[5/8] usb: core: hcd: Convert from tasklet to BH workqueue
Commit Message
The only generic interface to execute asynchronously in the BH context is
tasklet; however, it's marked deprecated and has some design flaws. To
replace tasklets, BH workqueue support was recently added. A BH workqueue
behaves similarly to regular workqueues except that the queued work items
are executed in the BH context.
This patch converts usb hcd from tasklet to BH workqueue.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
---
drivers/usb/core/hcd.c | 23 ++++++++++++-----------
include/linux/usb/hcd.h | 2 +-
2 files changed, 13 insertions(+), 12 deletions(-)
Comments
On Mon, Jan 29, 2024 at 11:11:52PM -1000, Tejun Heo wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts usb hcd from tasklet to BH workqueue.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> ---
> drivers/usb/core/hcd.c | 23 ++++++++++++-----------
> include/linux/usb/hcd.h | 2 +-
> 2 files changed, 13 insertions(+), 12 deletions(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Mon, 29 Jan 2024, Tejun Heo wrote:
>The only generic interface to execute asynchronously in the BH context is
>tasklet; however, it's marked deprecated and has some design flaws. To
>replace tasklets, BH workqueue support was recently added. A BH workqueue
>behaves similarly to regular workqueues except that the queued work items
>are executed in the BH context.
>
>This patch converts usb hcd from tasklet to BH workqueue.
In the past this tasklet removal was held up by Mauro's device not properly
streaming - hopefully this no longer the case. Cc'ing.
https://lore.kernel.org/all/20180216170450.yl5owfphuvltstnt@breakpoint.cc/
>
>Signed-off-by: Tejun Heo <tj@kernel.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Alan Stern <stern@rowland.harvard.edu>
>Cc: linux-usb@vger.kernel.org
Acked-by: Davidlohr Bueso <dave@stgolabs.net>
>---
> drivers/usb/core/hcd.c | 23 ++++++++++++-----------
> include/linux/usb/hcd.h | 2 +-
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>index 12b6dfeaf658..edf74458474a 100644
>--- a/drivers/usb/core/hcd.c
>+++ b/drivers/usb/core/hcd.c
>@@ -1664,9 +1664,10 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> usb_put_urb(urb);
> }
>
>-static void usb_giveback_urb_bh(struct tasklet_struct *t)
>+static void usb_giveback_urb_bh(struct work_struct *work)
> {
>- struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
>+ struct giveback_urb_bh *bh =
>+ container_of(work, struct giveback_urb_bh, bh);
> struct list_head local_list;
>
> spin_lock_irq(&bh->lock);
>@@ -1691,9 +1692,9 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
> spin_lock_irq(&bh->lock);
> if (!list_empty(&bh->head)) {
> if (bh->high_prio)
>- tasklet_hi_schedule(&bh->bh);
>+ queue_work(system_bh_highpri_wq, &bh->bh);
> else
>- tasklet_schedule(&bh->bh);
>+ queue_work(system_bh_wq, &bh->bh);
> }
> bh->running = false;
> spin_unlock_irq(&bh->lock);
>@@ -1706,7 +1707,7 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
> * @status: completion status code for the URB.
> *
> * Context: atomic. The completion callback is invoked in caller's context.
>- * For HCDs with HCD_BH flag set, the completion callback is invoked in tasklet
>+ * For HCDs with HCD_BH flag set, the completion callback is invoked in BH
> * context (except for URBs submitted to the root hub which always complete in
> * caller's context).
> *
>@@ -1725,7 +1726,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
> struct giveback_urb_bh *bh;
> bool running;
>
>- /* pass status to tasklet via unlinked */
>+ /* pass status to BH via unlinked */
> if (likely(!urb->unlinked))
> urb->unlinked = status;
>
>@@ -1747,9 +1748,9 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
> if (running)
> ;
> else if (bh->high_prio)
>- tasklet_hi_schedule(&bh->bh);
>+ queue_work(system_bh_highpri_wq, &bh->bh);
> else
>- tasklet_schedule(&bh->bh);
>+ queue_work(system_bh_wq, &bh->bh);
> }
> EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
>
>@@ -2540,7 +2541,7 @@ static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
>
> spin_lock_init(&bh->lock);
> INIT_LIST_HEAD(&bh->head);
>- tasklet_setup(&bh->bh, usb_giveback_urb_bh);
>+ INIT_WORK(&bh->bh, usb_giveback_urb_bh);
> }
>
> struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
>@@ -2926,7 +2927,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
> && device_can_wakeup(&hcd->self.root_hub->dev))
> dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>
>- /* initialize tasklets */
>+ /* initialize BHs */
> init_giveback_urb_bh(&hcd->high_prio_bh);
> hcd->high_prio_bh.high_prio = true;
> init_giveback_urb_bh(&hcd->low_prio_bh);
>@@ -3036,7 +3037,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
> mutex_unlock(&usb_bus_idr_lock);
>
> /*
>- * tasklet_kill() isn't needed here because:
>+ * flush_work() isn't needed here because:
> * - driver's disconnect() called from usb_disconnect() should
> * make sure its URBs are completed during the disconnect()
> * callback
>diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
>index 00724b4f6e12..f698aac71de3 100644
>--- a/include/linux/usb/hcd.h
>+++ b/include/linux/usb/hcd.h
>@@ -55,7 +55,7 @@ struct giveback_urb_bh {
> bool high_prio;
> spinlock_t lock;
> struct list_head head;
>- struct tasklet_struct bh;
>+ struct work_struct bh;
> struct usb_host_endpoint *completing_ep;
> };
>
>--
>2.43.0
>
On Tue, 20 Feb 2024 at 09:25, Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> In the past this tasklet removal was held up by Mauro's device not properly
> streaming - hopefully this no longer the case. Cc'ing.
>
> https://lore.kernel.org/all/20180216170450.yl5owfphuvltstnt@breakpoint.cc/
Oh, lovely - an actual use-case where the old tasklet code has known
requirements.
Mauro - the BH workqueue should provide the same kind of latency as
the tasklets, and it would be good to validate early that yes, this
workqueue conversion works well in practice. Since you have an actual
real-life test-case, could you give it a try?
You can find the work in
git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git
refs/heads/for-6.9-bh-conversions
although it's possible that Tejun has a newer version in some other
branch. Tejun - maybe point Mauro at something he can try out if you
have updated the conversion since?
Linus
Hello,
On Tue, Feb 20, 2024 at 09:55:30AM -0800, Linus Torvalds wrote:
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git
> refs/heads/for-6.9-bh-conversions
>
> although it's possible that Tejun has a newer version in some other
> branch. Tejun - maybe point Mauro at something he can try out if you
> have updated the conversion since?
Just pushed out the following branch for testing.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.9-bh-conversions-test
It's the same branch but combined with the current linus#master to avoid the
rc1 wonkiness.
Thanks.
On Tue, 20 Feb 2024, Linus Torvalds wrote:
>Mauro - the BH workqueue should provide the same kind of latency as
>the tasklets, and it would be good to validate early that yes, this
>workqueue conversion works well in practice. Since you have an actual
>real-life test-case, could you give it a try?
In general I think it's worth pointing out that future conversions should
still aim for an equivalent in task context, and now with disable/enable_work
a lot opens up for regular wq conversions. If users/maintainers shout about
latency, then use BH wq.
Thanks,
Davidlohr
@@ -1664,9 +1664,10 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
usb_put_urb(urb);
}
-static void usb_giveback_urb_bh(struct tasklet_struct *t)
+static void usb_giveback_urb_bh(struct work_struct *work)
{
- struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
+ struct giveback_urb_bh *bh =
+ container_of(work, struct giveback_urb_bh, bh);
struct list_head local_list;
spin_lock_irq(&bh->lock);
@@ -1691,9 +1692,9 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
spin_lock_irq(&bh->lock);
if (!list_empty(&bh->head)) {
if (bh->high_prio)
- tasklet_hi_schedule(&bh->bh);
+ queue_work(system_bh_highpri_wq, &bh->bh);
else
- tasklet_schedule(&bh->bh);
+ queue_work(system_bh_wq, &bh->bh);
}
bh->running = false;
spin_unlock_irq(&bh->lock);
@@ -1706,7 +1707,7 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
* @status: completion status code for the URB.
*
* Context: atomic. The completion callback is invoked in caller's context.
- * For HCDs with HCD_BH flag set, the completion callback is invoked in tasklet
+ * For HCDs with HCD_BH flag set, the completion callback is invoked in BH
* context (except for URBs submitted to the root hub which always complete in
* caller's context).
*
@@ -1725,7 +1726,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
struct giveback_urb_bh *bh;
bool running;
- /* pass status to tasklet via unlinked */
+ /* pass status to BH via unlinked */
if (likely(!urb->unlinked))
urb->unlinked = status;
@@ -1747,9 +1748,9 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
if (running)
;
else if (bh->high_prio)
- tasklet_hi_schedule(&bh->bh);
+ queue_work(system_bh_highpri_wq, &bh->bh);
else
- tasklet_schedule(&bh->bh);
+ queue_work(system_bh_wq, &bh->bh);
}
EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
@@ -2540,7 +2541,7 @@ static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
spin_lock_init(&bh->lock);
INIT_LIST_HEAD(&bh->head);
- tasklet_setup(&bh->bh, usb_giveback_urb_bh);
+ INIT_WORK(&bh->bh, usb_giveback_urb_bh);
}
struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
@@ -2926,7 +2927,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
&& device_can_wakeup(&hcd->self.root_hub->dev))
dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
- /* initialize tasklets */
+ /* initialize BHs */
init_giveback_urb_bh(&hcd->high_prio_bh);
hcd->high_prio_bh.high_prio = true;
init_giveback_urb_bh(&hcd->low_prio_bh);
@@ -3036,7 +3037,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
mutex_unlock(&usb_bus_idr_lock);
/*
- * tasklet_kill() isn't needed here because:
+ * flush_work() isn't needed here because:
* - driver's disconnect() called from usb_disconnect() should
* make sure its URBs are completed during the disconnect()
* callback
@@ -55,7 +55,7 @@ struct giveback_urb_bh {
bool high_prio;
spinlock_t lock;
struct list_head head;
- struct tasklet_struct bh;
+ struct work_struct bh;
struct usb_host_endpoint *completing_ep;
};