| Message ID | 20240327160314.9982-1-apais@linux.microsoft.com |
|---|---|
| Headers | show |
| Series | Convert Tasklets to BH Workqueues | expand |
Hi Allen, the usbatm bits look very reasonable to me. Unfortunately I don't
have the hardware to test any more. Still, for what it's worth:
Signed-off-by: Duncan Sands <duncan.sands@free.fr>
On Wed, Mar 27, 2024 at 04:03:09PM +0000, Allen Pais 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 drivers/infiniband/* from tasklet to BH workqueue. No it does not, I think your changelog is wrong :( > > Based on the work done by Tejun Heo <tj@kernel.org> > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > Signed-off-by: Allen Pais <allen.lkml@gmail.com> > --- > drivers/usb/atm/usbatm.c | 55 +++++++++++++++-------------- > drivers/usb/atm/usbatm.h | 3 +- > drivers/usb/core/hcd.c | 22 ++++++------ > drivers/usb/gadget/udc/fsl_qe_udc.c | 21 +++++------ > drivers/usb/gadget/udc/fsl_qe_udc.h | 4 +-- > drivers/usb/host/ehci-sched.c | 2 +- > drivers/usb/host/fhci-hcd.c | 3 +- > drivers/usb/host/fhci-sched.c | 10 +++--- > drivers/usb/host/fhci.h | 5 +-- > drivers/usb/host/xhci-dbgcap.h | 3 +- > drivers/usb/host/xhci-dbgtty.c | 15 ++++---- > include/linux/usb/cdc_ncm.h | 2 +- > include/linux/usb/usbnet.h | 2 +- > 13 files changed, 76 insertions(+), 71 deletions(-) > > diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c > index 2da6615fbb6f..74849f24e52e 100644 > --- a/drivers/usb/atm/usbatm.c > +++ b/drivers/usb/atm/usbatm.c > @@ -17,7 +17,7 @@ > * - Removed the limit on the number of devices > * - Module now autoloads on device plugin > * - Merged relevant parts of sarlib > - * - Replaced the kernel thread with a tasklet > + * - Replaced the kernel thread with a work a "work"? > * - New packet transmission code > * - Changed proc file contents > * - Fixed all known SMP races > @@ -68,6 +68,7 @@ > #include <linux/wait.h> > #include <linux/kthread.h> > #include <linux/ratelimit.h> > +#include <linux/workqueue.h> > > #ifdef VERBOSE_DEBUG > static int usbatm_print_packet(struct usbatm_data *instance, const unsigned char *data, int len); > @@ -249,7 +250,7 @@ static void usbatm_complete(struct urb *urb) > /* vdbg("%s: urb 0x%p, status %d, actual_length %d", > __func__, urb, status, urb->actual_length); */ > > - /* Can be invoked from task context, protect against interrupts */ > + /* Can be invoked from work context, protect against interrupts */ "workqueue"? This too seems wrong. Same for other comment changes in this patch. thanks, greg k-h
On Wed, Mar 27, 2024 at 04:03:11PM +0000, Allen Pais 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 drivers/infiniband/* from tasklet to BH workqueue. I think you mean drivers/char/ipmi/* here. I believe that work queues items are execute single-threaded for a work queue, so this should be good. I need to test this, though. It may be that an IPMI device can have its own work queue; it may not be important to run it in bh context. -corey > > Based on the work done by Tejun Heo <tj@kernel.org> > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > Signed-off-by: Allen Pais <allen.lkml@gmail.com> > --- > drivers/char/ipmi/ipmi_msghandler.c | 30 ++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c > index b0eedc4595b3..fce2a2dbdc82 100644 > --- a/drivers/char/ipmi/ipmi_msghandler.c > +++ b/drivers/char/ipmi/ipmi_msghandler.c > @@ -36,12 +36,13 @@ > #include <linux/nospec.h> > #include <linux/vmalloc.h> > #include <linux/delay.h> > +#include <linux/workqueue.h> > > #define IPMI_DRIVER_VERSION "39.2" > > static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void); > static int ipmi_init_msghandler(void); > -static void smi_recv_tasklet(struct tasklet_struct *t); > +static void smi_recv_work(struct work_struct *t); > static void handle_new_recv_msgs(struct ipmi_smi *intf); > static void need_waiter(struct ipmi_smi *intf); > static int handle_one_recv_msg(struct ipmi_smi *intf, > @@ -498,13 +499,13 @@ struct ipmi_smi { > /* > * Messages queued for delivery. If delivery fails (out of memory > * for instance), They will stay in here to be processed later in a > - * periodic timer interrupt. The tasklet is for handling received > + * periodic timer interrupt. The work is for handling received > * messages directly from the handler. > */ > spinlock_t waiting_rcv_msgs_lock; > struct list_head waiting_rcv_msgs; > atomic_t watchdog_pretimeouts_to_deliver; > - struct tasklet_struct recv_tasklet; > + struct work_struct recv_work; > > spinlock_t xmit_msgs_lock; > struct list_head xmit_msgs; > @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi *intf) > struct cmd_rcvr *rcvr, *rcvr2; > struct list_head list; > > - tasklet_kill(&intf->recv_tasklet); > + cancel_work_sync(&intf->recv_work); > > free_smi_msg_list(&intf->waiting_rcv_msgs); > free_recv_msg_list(&intf->waiting_events); > @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref) > { > struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount); > > - /* SRCU cleanup must happen in task context. */ > + /* SRCU cleanup must happen in work context. */ > queue_work(remove_work_wq, &user->remove_work); > } > > @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner, > intf->curr_seq = 0; > spin_lock_init(&intf->waiting_rcv_msgs_lock); > INIT_LIST_HEAD(&intf->waiting_rcv_msgs); > - tasklet_setup(&intf->recv_tasklet, > - smi_recv_tasklet); > + INIT_WORK(&intf->recv_work, smi_recv_work); > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 0); > spin_lock_init(&intf->xmit_msgs_lock); > INIT_LIST_HEAD(&intf->xmit_msgs); > @@ -4779,7 +4779,7 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) > * To preserve message order, quit if we > * can't handle a message. Add the message > * back at the head, this is safe because this > - * tasklet is the only thing that pulls the > + * work is the only thing that pulls the > * messages. > */ > list_add(&smi_msg->link, &intf->waiting_rcv_msgs); > @@ -4812,10 +4812,10 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) > } > } > > -static void smi_recv_tasklet(struct tasklet_struct *t) > +static void smi_recv_work(struct work_struct *t) > { > unsigned long flags = 0; /* keep us warning-free. */ > - struct ipmi_smi *intf = from_tasklet(intf, t, recv_tasklet); > + struct ipmi_smi *intf = from_work(intf, t, recv_work); > int run_to_completion = intf->run_to_completion; > struct ipmi_smi_msg *newmsg = NULL; > > @@ -4866,7 +4866,7 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, > > /* > * To preserve message order, we keep a queue and deliver from > - * a tasklet. > + * a work. > */ > if (!run_to_completion) > spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); > @@ -4887,9 +4887,9 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, > spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags); > > if (run_to_completion) > - smi_recv_tasklet(&intf->recv_tasklet); > + smi_recv_work(&intf->recv_work); > else > - tasklet_schedule(&intf->recv_tasklet); > + queue_work(system_bh_wq, &intf->recv_work); > } > EXPORT_SYMBOL(ipmi_smi_msg_received); > > @@ -4899,7 +4899,7 @@ void ipmi_smi_watchdog_pretimeout(struct ipmi_smi *intf) > return; > > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 1); > - tasklet_schedule(&intf->recv_tasklet); > + queue_work(system_bh_wq, &intf->recv_work); > } > EXPORT_SYMBOL(ipmi_smi_watchdog_pretimeout); > > @@ -5068,7 +5068,7 @@ static bool ipmi_timeout_handler(struct ipmi_smi *intf, > flags); > } > > - tasklet_schedule(&intf->recv_tasklet); > + queue_work(system_bh_wq, &intf->recv_work); > > return need_timer; > } > -- > 2.17.1 > >
On Wed, Mar 27, 2024 at 11:05 AM Corey Minyard <minyard@acm.org> wrote: > > On Wed, Mar 27, 2024 at 04:03:11PM +0000, Allen Pais 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 drivers/infiniband/* from tasklet to BH workqueue. > > I think you mean drivers/char/ipmi/* here. My apologies, my scripts messed up the commit messages for this series. Will have it fixed in v2. > > I believe that work queues items are execute single-threaded for a work > queue, so this should be good. I need to test this, though. It may be > that an IPMI device can have its own work queue; it may not be important > to run it in bh context. Fair point. Could you please let me know once you have had a chance to test these changes. Meanwhile, I will work on RFC wherein IPMI will have its own workqueue. Thanks for taking time out to review. - Allen > > -corey > > > > > Based on the work done by Tejun Heo <tj@kernel.org> > > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > > > Signed-off-by: Allen Pais <allen.lkml@gmail.com> > > --- > > drivers/char/ipmi/ipmi_msghandler.c | 30 ++++++++++++++--------------- > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c > > index b0eedc4595b3..fce2a2dbdc82 100644 > > --- a/drivers/char/ipmi/ipmi_msghandler.c > > +++ b/drivers/char/ipmi/ipmi_msghandler.c > > @@ -36,12 +36,13 @@ > > #include <linux/nospec.h> > > #include <linux/vmalloc.h> > > #include <linux/delay.h> > > +#include <linux/workqueue.h> > > > > #define IPMI_DRIVER_VERSION "39.2" > > > > static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void); > > static int ipmi_init_msghandler(void); > > -static void smi_recv_tasklet(struct tasklet_struct *t); > > +static void smi_recv_work(struct work_struct *t); > > static void handle_new_recv_msgs(struct ipmi_smi *intf); > > static void need_waiter(struct ipmi_smi *intf); > > static int handle_one_recv_msg(struct ipmi_smi *intf, > > @@ -498,13 +499,13 @@ struct ipmi_smi { > > /* > > * Messages queued for delivery. If delivery fails (out of memory > > * for instance), They will stay in here to be processed later in a > > - * periodic timer interrupt. The tasklet is for handling received > > + * periodic timer interrupt. The work is for handling received > > * messages directly from the handler. > > */ > > spinlock_t waiting_rcv_msgs_lock; > > struct list_head waiting_rcv_msgs; > > atomic_t watchdog_pretimeouts_to_deliver; > > - struct tasklet_struct recv_tasklet; > > + struct work_struct recv_work; > > > > spinlock_t xmit_msgs_lock; > > struct list_head xmit_msgs; > > @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi *intf) > > struct cmd_rcvr *rcvr, *rcvr2; > > struct list_head list; > > > > - tasklet_kill(&intf->recv_tasklet); > > + cancel_work_sync(&intf->recv_work); > > > > free_smi_msg_list(&intf->waiting_rcv_msgs); > > free_recv_msg_list(&intf->waiting_events); > > @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref) > > { > > struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount); > > > > - /* SRCU cleanup must happen in task context. */ > > + /* SRCU cleanup must happen in work context. */ > > queue_work(remove_work_wq, &user->remove_work); > > } > > > > @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner, > > intf->curr_seq = 0; > > spin_lock_init(&intf->waiting_rcv_msgs_lock); > > INIT_LIST_HEAD(&intf->waiting_rcv_msgs); > > - tasklet_setup(&intf->recv_tasklet, > > - smi_recv_tasklet); > > + INIT_WORK(&intf->recv_work, smi_recv_work); > > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 0); > > spin_lock_init(&intf->xmit_msgs_lock); > > INIT_LIST_HEAD(&intf->xmit_msgs); > > @@ -4779,7 +4779,7 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) > > * To preserve message order, quit if we > > * can't handle a message. Add the message > > * back at the head, this is safe because this > > - * tasklet is the only thing that pulls the > > + * work is the only thing that pulls the > > * messages. > > */ > > list_add(&smi_msg->link, &intf->waiting_rcv_msgs); > > @@ -4812,10 +4812,10 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) > > } > > } > > > > -static void smi_recv_tasklet(struct tasklet_struct *t) > > +static void smi_recv_work(struct work_struct *t) > > { > > unsigned long flags = 0; /* keep us warning-free. */ > > - struct ipmi_smi *intf = from_tasklet(intf, t, recv_tasklet); > > + struct ipmi_smi *intf = from_work(intf, t, recv_work); > > int run_to_completion = intf->run_to_completion; > > struct ipmi_smi_msg *newmsg = NULL; > > > > @@ -4866,7 +4866,7 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, > > > > /* > > * To preserve message order, we keep a queue and deliver from > > - * a tasklet. > > + * a work. > > */ > > if (!run_to_completion) > > spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); > > @@ -4887,9 +4887,9 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, > > spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags); > > > > if (run_to_completion) > > - smi_recv_tasklet(&intf->recv_tasklet); > > + smi_recv_work(&intf->recv_work); > > else > > - tasklet_schedule(&intf->recv_tasklet); > > + queue_work(system_bh_wq, &intf->recv_work); > > } > > EXPORT_SYMBOL(ipmi_smi_msg_received); > > > > @@ -4899,7 +4899,7 @@ void ipmi_smi_watchdog_pretimeout(struct ipmi_smi *intf) > > return; > > > > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 1); > > - tasklet_schedule(&intf->recv_tasklet); > > + queue_work(system_bh_wq, &intf->recv_work); > > } > > EXPORT_SYMBOL(ipmi_smi_watchdog_pretimeout); > > > > @@ -5068,7 +5068,7 @@ static bool ipmi_timeout_handler(struct ipmi_smi *intf, > > flags); > > } > > > > - tasklet_schedule(&intf->recv_tasklet); > > + queue_work(system_bh_wq, &intf->recv_work); > > > > return need_timer; > > } > > -- > > 2.17.1 > > > > >
> > > > This patch converts drivers/infiniband/* from tasklet to BH workqueue. > > > > Based on the work done by Tejun Heo <tj@kernel.org> > > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > > > Signed-off-by: Allen Pais <allen.lkml@gmail.com> > > --- > > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > > index c0e005670d67..88d8e1c366cd 100644 > > --- a/drivers/usb/core/hcd.c > > +++ b/drivers/usb/core/hcd.c > > > @@ -1662,10 +1663,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > > usb_put_urb(urb); > > } > > > > -static void usb_giveback_urb_bh(struct work_struct *work) > > +static void usb_giveback_urb_bh(struct work_struct *t) > > { > > - struct giveback_urb_bh *bh = > > - container_of(work, struct giveback_urb_bh, bh); > > + struct giveback_urb_bh *bh = from_work(bh, t, bh); > > struct list_head local_list; > > > > spin_lock_irq(&bh->lock); > > Is there any reason for this apparently pointless change of a local > variable's name? No, it was done just to keep things consistent across the kernel. I can revert it back to *work if you'd prefer. Thanks. > > Alan Stern >
On Thu, Mar 28, 2024 at 10:52:16AM -0700, Allen wrote: > On Wed, Mar 27, 2024 at 11:05 AM Corey Minyard <minyard@acm.org> wrote: > > > > I believe that work queues items are execute single-threaded for a work > > queue, so this should be good. I need to test this, though. It may be > > that an IPMI device can have its own work queue; it may not be important > > to run it in bh context. > > Fair point. Could you please let me know once you have had a chance to test > these changes. Meanwhile, I will work on RFC wherein IPMI will have its own > workqueue. > > Thanks for taking time out to review. After looking and thinking about it a bit, a BH context is still probably the best for this. I have tested this patch under load and various scenarios and it seems to work ok. So: Tested-by: Corey Minyard <cminyard@mvista.com> Acked-by: Corey Minyard <cminyard@mvista.com> Or I can take this into my tree. -corey > > - Allen > > > > > -corey > > > > > > > > Based on the work done by Tejun Heo <tj@kernel.org> > > > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-610 > > > > > > Signed-off-by: Allen Pais <allen.lkml@gmail.com> > > > --- > > > drivers/char/ipmi/ipmi_msghandler.c | 30 ++++++++++++++--------------- > > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c > > > index b0eedc4595b3..fce2a2dbdc82 100644 > > > --- a/drivers/char/ipmi/ipmi_msghandler.c > > > +++ b/drivers/char/ipmi/ipmi_msghandler.c > > > @@ -36,12 +36,13 @@ > > > #include <linux/nospec.h> > > > #include <linux/vmalloc.h> > > > #include <linux/delay.h> > > > +#include <linux/workqueue.h> > > > > > > #define IPMI_DRIVER_VERSION "39.2" > > > > > > static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void); > > > static int ipmi_init_msghandler(void); > > > -static void smi_recv_tasklet(struct tasklet_struct *t); > > > +static void smi_recv_work(struct work_struct *t); > > > static void handle_new_recv_msgs(struct ipmi_smi *intf); > > > static void need_waiter(struct ipmi_smi *intf); > > > static int handle_one_recv_msg(struct ipmi_smi *intf, > > > @@ -498,13 +499,13 @@ struct ipmi_smi { > > > /* > > > * Messages queued for delivery. If delivery fails (out of memory > > > * for instance), They will stay in here to be processed later in a > > > - * periodic timer interrupt. The tasklet is for handling received > > > + * periodic timer interrupt. The work is for handling received > > > * messages directly from the handler. > > > */ > > > spinlock_t waiting_rcv_msgs_lock; > > > struct list_head waiting_rcv_msgs; > > > atomic_t watchdog_pretimeouts_to_deliver; > > > - struct tasklet_struct recv_tasklet; > > > + struct work_struct recv_work; > > > > > > spinlock_t xmit_msgs_lock; > > > struct list_head xmit_msgs; > > > @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi *intf) > > > struct cmd_rcvr *rcvr, *rcvr2; > > > struct list_head list; > > > > > > - tasklet_kill(&intf->recv_tasklet); > > > + cancel_work_sync(&intf->recv_work); > > > > > > free_smi_msg_list(&intf->waiting_rcv_msgs); > > > free_recv_msg_list(&intf->waiting_events); > > > @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref) > > > { > > > struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount); > > > > > > - /* SRCU cleanup must happen in task context. */ > > > + /* SRCU cleanup must happen in work context. */ > > > queue_work(remove_work_wq, &user->remove_work); > > > } > > > > > > @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner, > > > intf->curr_seq = 0; > > > spin_lock_init(&intf->waiting_rcv_msgs_lock); > > > INIT_LIST_HEAD(&intf->waiting_rcv_msgs); > > > - tasklet_setup(&intf->recv_tasklet, > > > - smi_recv_tasklet); > > > + INIT_WORK(&intf->recv_work, smi_recv_work); > > > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 0); > > > spin_lock_init(&intf->xmit_msgs_lock); > > > INIT_LIST_HEAD(&intf->xmit_msgs); > > > @@ -4779,7 +4779,7 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) > > > * To preserve message order, quit if we > > > * can't handle a message. Add the message > > > * back at the head, this is safe because this > > > - * tasklet is the only thing that pulls the > > > + * work is the only thing that pulls the > > > * messages. > > > */ > > > list_add(&smi_msg->link, &intf->waiting_rcv_msgs); > > > @@ -4812,10 +4812,10 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf) > > > } > > > } > > > > > > -static void smi_recv_tasklet(struct tasklet_struct *t) > > > +static void smi_recv_work(struct work_struct *t) > > > { > > > unsigned long flags = 0; /* keep us warning-free. */ > > > - struct ipmi_smi *intf = from_tasklet(intf, t, recv_tasklet); > > > + struct ipmi_smi *intf = from_work(intf, t, recv_work); > > > int run_to_completion = intf->run_to_completion; > > > struct ipmi_smi_msg *newmsg = NULL; > > > > > > @@ -4866,7 +4866,7 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, > > > > > > /* > > > * To preserve message order, we keep a queue and deliver from > > > - * a tasklet. > > > + * a work. > > > */ > > > if (!run_to_completion) > > > spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); > > > @@ -4887,9 +4887,9 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf, > > > spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags); > > > > > > if (run_to_completion) > > > - smi_recv_tasklet(&intf->recv_tasklet); > > > + smi_recv_work(&intf->recv_work); > > > else > > > - tasklet_schedule(&intf->recv_tasklet); > > > + queue_work(system_bh_wq, &intf->recv_work); > > > } > > > EXPORT_SYMBOL(ipmi_smi_msg_received); > > > > > > @@ -4899,7 +4899,7 @@ void ipmi_smi_watchdog_pretimeout(struct ipmi_smi *intf) > > > return; > > > > > > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 1); > > > - tasklet_schedule(&intf->recv_tasklet); > > > + queue_work(system_bh_wq, &intf->recv_work); > > > } > > > EXPORT_SYMBOL(ipmi_smi_watchdog_pretimeout); > > > > > > @@ -5068,7 +5068,7 @@ static bool ipmi_timeout_handler(struct ipmi_smi *intf, > > > flags); > > > } > > > > > > - tasklet_schedule(&intf->recv_tasklet); > > > + queue_work(system_bh_wq, &intf->recv_work); > > > > > > return need_timer; > > > } > > > -- > > > 2.17.1 > > > > > > > > > > > -- > - Allen >
> > > I believe that work queues items are execute single-threaded for a work > > > queue, so this should be good. I need to test this, though. It may be > > > that an IPMI device can have its own work queue; it may not be important > > > to run it in bh context. > > > > Fair point. Could you please let me know once you have had a chance to test > > these changes. Meanwhile, I will work on RFC wherein IPMI will have its own > > workqueue. > > > > Thanks for taking time out to review. > > After looking and thinking about it a bit, a BH context is still > probably the best for this. > > I have tested this patch under load and various scenarios and it seems > to work ok. So: > > Tested-by: Corey Minyard <cminyard@mvista.com> > Acked-by: Corey Minyard <cminyard@mvista.com> > > Or I can take this into my tree. > > -corey Thank you very much. I think it should be okay for you to carry it into your tree. - Allen
On Thu, Mar 28, 2024 at 12:41:22PM -0700, Allen wrote: > > > > I believe that work queues items are execute single-threaded for a work > > > > queue, so this should be good. I need to test this, though. It may be > > > > that an IPMI device can have its own work queue; it may not be important > > > > to run it in bh context. > > > > > > Fair point. Could you please let me know once you have had a chance to test > > > these changes. Meanwhile, I will work on RFC wherein IPMI will have its own > > > workqueue. > > > > > > Thanks for taking time out to review. > > > > After looking and thinking about it a bit, a BH context is still > > probably the best for this. > > > > I have tested this patch under load and various scenarios and it seems > > to work ok. So: > > > > Tested-by: Corey Minyard <cminyard@mvista.com> > > Acked-by: Corey Minyard <cminyard@mvista.com> > > > > Or I can take this into my tree. > > > > -corey > > Thank you very much. I think it should be okay for you to carry it into > your tree. Ok, it's in my for-next tree. I fixed the directory reference, and I changed all the comments where you changed "tasklet" to "work" to instead say "workqueue". -corey > > - Allen >
> > > > > > > > Fair point. Could you please let me know once you have had a chance to test > > > > these changes. Meanwhile, I will work on RFC wherein IPMI will have its own > > > > workqueue. > > > > > > > > Thanks for taking time out to review. > > > > > > After looking and thinking about it a bit, a BH context is still > > > probably the best for this. > > > > > > I have tested this patch under load and various scenarios and it seems > > > to work ok. So: > > > > > > Tested-by: Corey Minyard <cminyard@mvista.com> > > > Acked-by: Corey Minyard <cminyard@mvista.com> > > > > > > Or I can take this into my tree. > > > > > > -corey > > > > Thank you very much. I think it should be okay for you to carry it into > > your tree. > > Ok, it's in my for-next tree. I fixed the directory reference, and I > changed all the comments where you changed "tasklet" to "work" to > instead say "workqueue". > Thank you very much for fixing it. - Allen
This patch series represents a significant shift in how asynchronous execution in the bottom half (BH) context is handled within the kernel. Traditionally, tasklets have been the go-to mechanism for such operations. This series introduces the conversion of existing tasklet implementations to the newly supported BH workqueues, marking a pivotal enhancement in how asynchronous tasks are managed and executed. Background and Motivation: Tasklets have served as the kernel's lightweight mechanism for scheduling bottom-half processing, providing a simple interface for deferring work from interrupt context. There have been increasing requests and motivations to deprecate and eventually remove tasklets in favor of more modern and flexible mechanisms. Introduction of BH Workqueues: BH workqueues are designed to behave similarly to regular workqueues with the added benefit of execution in the BH context. Conversion Details: The conversion process involved identifying all instances where tasklets were used within the kernel and replacing them with BH workqueue implementations. This patch series is a first step toward broader adoption of BH workqueues across the kernel, and soon other subsystems using tasklets will undergo a similar transition. The groundwork laid here could serve as a blueprint for such future conversions. Testing Request: In addition to a thorough review of these changes, I kindly request that the reviwers engage in both functional and performance testing of this patch series. Specifically, benchmarks that measure interrupt handling efficiency, latency, and throughput. I welcome your feedback, suggestions, and any further discussion on this patch series. Additional Info: Based on the work done by Tejun Heo <tj@kernel.org> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 Allen Pais (9): hyperv: Convert from tasklet to BH workqueue dma: Convert from tasklet to BH workqueue IB: Convert from tasklet to BH workqueue USB: Convert from tasklet to BH workqueue mailbox: Convert from tasklet to BH workqueue ipmi: Convert from tasklet to BH workqueue s390: Convert from tasklet to BH workqueue drivers/media/*: Convert from tasklet to BH workqueue mmc: Convert from tasklet to BH workqueue drivers/char/ipmi/ipmi_msghandler.c | 30 ++++---- drivers/dma/altera-msgdma.c | 15 ++-- drivers/dma/apple-admac.c | 15 ++-- drivers/dma/at_hdmac.c | 2 +- drivers/dma/at_xdmac.c | 15 ++-- drivers/dma/bcm2835-dma.c | 2 +- drivers/dma/dma-axi-dmac.c | 2 +- drivers/dma/dma-jz4780.c | 2 +- .../dma/dw-axi-dmac/dw-axi-dmac-platform.c | 2 +- drivers/dma/dw-edma/dw-edma-core.c | 2 +- drivers/dma/dw/core.c | 13 ++-- drivers/dma/dw/regs.h | 3 +- drivers/dma/ep93xx_dma.c | 15 ++-- drivers/dma/fsl-edma-common.c | 2 +- drivers/dma/fsl-qdma.c | 2 +- drivers/dma/fsl_raid.c | 11 +-- drivers/dma/fsl_raid.h | 2 +- drivers/dma/fsldma.c | 15 ++-- drivers/dma/fsldma.h | 3 +- drivers/dma/hisi_dma.c | 2 +- drivers/dma/hsu/hsu.c | 2 +- drivers/dma/idma64.c | 4 +- drivers/dma/img-mdc-dma.c | 2 +- drivers/dma/imx-dma.c | 27 +++---- drivers/dma/imx-sdma.c | 6 +- drivers/dma/ioat/dma.c | 17 +++-- drivers/dma/ioat/dma.h | 5 +- drivers/dma/ioat/init.c | 2 +- drivers/dma/k3dma.c | 19 ++--- drivers/dma/mediatek/mtk-cqdma.c | 35 ++++----- drivers/dma/mediatek/mtk-hsdma.c | 2 +- drivers/dma/mediatek/mtk-uart-apdma.c | 4 +- drivers/dma/mmp_pdma.c | 13 ++-- drivers/dma/mmp_tdma.c | 11 +-- drivers/dma/mpc512x_dma.c | 17 +++-- drivers/dma/mv_xor.c | 13 ++-- drivers/dma/mv_xor.h | 5 +- drivers/dma/mv_xor_v2.c | 23 +++--- drivers/dma/mxs-dma.c | 13 ++-- drivers/dma/nbpfaxi.c | 15 ++-- drivers/dma/owl-dma.c | 2 +- drivers/dma/pch_dma.c | 17 +++-- drivers/dma/pl330.c | 31 ++++---- drivers/dma/plx_dma.c | 13 ++-- drivers/dma/ppc4xx/adma.c | 17 +++-- drivers/dma/ppc4xx/adma.h | 5 +- drivers/dma/pxa_dma.c | 2 +- drivers/dma/qcom/bam_dma.c | 35 ++++----- drivers/dma/qcom/gpi.c | 18 ++--- drivers/dma/qcom/hidma.c | 11 +-- drivers/dma/qcom/hidma.h | 5 +- drivers/dma/qcom/hidma_ll.c | 11 +-- drivers/dma/qcom/qcom_adm.c | 2 +- drivers/dma/sa11x0-dma.c | 27 +++---- drivers/dma/sf-pdma/sf-pdma.c | 23 +++--- drivers/dma/sf-pdma/sf-pdma.h | 5 +- drivers/dma/sprd-dma.c | 2 +- drivers/dma/st_fdma.c | 2 +- drivers/dma/ste_dma40.c | 17 +++-- drivers/dma/sun6i-dma.c | 33 ++++---- drivers/dma/tegra186-gpc-dma.c | 2 +- drivers/dma/tegra20-apb-dma.c | 19 ++--- drivers/dma/tegra210-adma.c | 2 +- drivers/dma/ti/edma.c | 2 +- drivers/dma/ti/k3-udma.c | 11 +-- drivers/dma/ti/omap-dma.c | 2 +- drivers/dma/timb_dma.c | 23 +++--- drivers/dma/txx9dmac.c | 29 +++---- drivers/dma/txx9dmac.h | 5 +- drivers/dma/virt-dma.c | 9 ++- drivers/dma/virt-dma.h | 9 ++- drivers/dma/xgene-dma.c | 21 +++--- drivers/dma/xilinx/xilinx_dma.c | 23 +++--- drivers/dma/xilinx/xilinx_dpdma.c | 21 +++--- drivers/dma/xilinx/zynqmp_dma.c | 21 +++--- drivers/hv/channel.c | 8 +- drivers/hv/channel_mgmt.c | 5 +- drivers/hv/connection.c | 9 ++- drivers/hv/hv.c | 3 +- drivers/hv/hv_balloon.c | 4 +- drivers/hv/hv_fcopy.c | 8 +- drivers/hv/hv_kvp.c | 8 +- drivers/hv/hv_snapshot.c | 8 +- drivers/hv/hyperv_vmbus.h | 9 ++- drivers/hv/vmbus_drv.c | 19 ++--- drivers/infiniband/hw/bnxt_re/bnxt_re.h | 3 +- drivers/infiniband/hw/bnxt_re/qplib_fp.c | 21 +++--- drivers/infiniband/hw/bnxt_re/qplib_fp.h | 2 +- drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 25 ++++--- drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 2 +- drivers/infiniband/hw/erdma/erdma.h | 3 +- drivers/infiniband/hw/erdma/erdma_eq.c | 11 +-- drivers/infiniband/hw/hfi1/rc.c | 2 +- drivers/infiniband/hw/hfi1/sdma.c | 37 ++++----- drivers/infiniband/hw/hfi1/sdma.h | 9 ++- drivers/infiniband/hw/hfi1/tid_rdma.c | 6 +- drivers/infiniband/hw/irdma/ctrl.c | 2 +- drivers/infiniband/hw/irdma/hw.c | 24 +++--- drivers/infiniband/hw/irdma/main.h | 5 +- drivers/infiniband/hw/qib/qib.h | 7 +- drivers/infiniband/hw/qib/qib_iba7322.c | 9 ++- drivers/infiniband/hw/qib/qib_rc.c | 16 ++-- drivers/infiniband/hw/qib/qib_ruc.c | 4 +- drivers/infiniband/hw/qib/qib_sdma.c | 11 +-- drivers/infiniband/sw/rdmavt/qp.c | 2 +- drivers/mailbox/bcm-pdc-mailbox.c | 21 +++--- drivers/mailbox/imx-mailbox.c | 16 ++-- drivers/media/pci/bt8xx/bt878.c | 8 +- drivers/media/pci/bt8xx/bt878.h | 3 +- drivers/media/pci/bt8xx/dvb-bt8xx.c | 9 ++- drivers/media/pci/ddbridge/ddbridge.h | 3 +- drivers/media/pci/mantis/hopper_cards.c | 2 +- drivers/media/pci/mantis/mantis_cards.c | 2 +- drivers/media/pci/mantis/mantis_common.h | 3 +- drivers/media/pci/mantis/mantis_dma.c | 5 +- drivers/media/pci/mantis/mantis_dma.h | 2 +- drivers/media/pci/mantis/mantis_dvb.c | 12 +-- drivers/media/pci/ngene/ngene-core.c | 23 +++--- drivers/media/pci/ngene/ngene.h | 5 +- drivers/media/pci/smipcie/smipcie-main.c | 18 ++--- drivers/media/pci/smipcie/smipcie.h | 3 +- drivers/media/pci/ttpci/budget-av.c | 3 +- drivers/media/pci/ttpci/budget-ci.c | 27 +++---- drivers/media/pci/ttpci/budget-core.c | 10 +-- drivers/media/pci/ttpci/budget.h | 5 +- drivers/media/pci/tw5864/tw5864-core.c | 2 +- drivers/media/pci/tw5864/tw5864-video.c | 13 ++-- drivers/media/pci/tw5864/tw5864.h | 7 +- drivers/media/platform/intel/pxa_camera.c | 15 ++-- drivers/media/platform/marvell/mcam-core.c | 11 +-- drivers/media/platform/marvell/mcam-core.h | 3 +- .../st/sti/c8sectpfe/c8sectpfe-core.c | 15 ++-- .../st/sti/c8sectpfe/c8sectpfe-core.h | 2 +- drivers/media/radio/wl128x/fmdrv.h | 7 +- drivers/media/radio/wl128x/fmdrv_common.c | 41 +++++----- drivers/media/rc/mceusb.c | 2 +- drivers/media/usb/ttusb-dec/ttusb_dec.c | 21 +++--- drivers/mmc/host/atmel-mci.c | 35 ++++----- drivers/mmc/host/au1xmmc.c | 37 ++++----- drivers/mmc/host/cb710-mmc.c | 15 ++-- drivers/mmc/host/cb710-mmc.h | 3 +- drivers/mmc/host/dw_mmc.c | 25 ++++--- drivers/mmc/host/dw_mmc.h | 9 ++- drivers/mmc/host/omap.c | 17 +++-- drivers/mmc/host/renesas_sdhi.h | 3 +- drivers/mmc/host/renesas_sdhi_internal_dmac.c | 24 +++--- drivers/mmc/host/renesas_sdhi_sys_dmac.c | 9 +-- drivers/mmc/host/sdhci-bcm-kona.c | 2 +- drivers/mmc/host/tifm_sd.c | 15 ++-- drivers/mmc/host/tmio_mmc.h | 3 +- drivers/mmc/host/tmio_mmc_core.c | 4 +- drivers/mmc/host/uniphier-sd.c | 13 ++-- drivers/mmc/host/via-sdmmc.c | 25 ++++--- drivers/mmc/host/wbsd.c | 75 ++++++++++--------- drivers/mmc/host/wbsd.h | 10 +-- drivers/s390/block/dasd.c | 42 +++++------ drivers/s390/block/dasd_int.h | 10 +-- drivers/s390/char/con3270.c | 27 ++++--- drivers/s390/crypto/ap_bus.c | 24 +++--- drivers/s390/crypto/ap_bus.h | 2 +- drivers/s390/crypto/zcrypt_msgtype50.c | 2 +- drivers/s390/crypto/zcrypt_msgtype6.c | 4 +- drivers/s390/net/ctcm_fsms.c | 4 +- drivers/s390/net/ctcm_main.c | 15 ++-- drivers/s390/net/ctcm_main.h | 5 +- drivers/s390/net/ctcm_mpc.c | 12 +-- drivers/s390/net/ctcm_mpc.h | 7 +- drivers/s390/net/lcs.c | 26 +++---- drivers/s390/net/lcs.h | 2 +- drivers/s390/net/qeth_core_main.c | 2 +- drivers/s390/scsi/zfcp_qdio.c | 45 +++++------ drivers/s390/scsi/zfcp_qdio.h | 9 ++- drivers/usb/atm/usbatm.c | 55 +++++++------- drivers/usb/atm/usbatm.h | 3 +- drivers/usb/core/hcd.c | 22 +++--- drivers/usb/gadget/udc/fsl_qe_udc.c | 21 +++--- drivers/usb/gadget/udc/fsl_qe_udc.h | 4 +- drivers/usb/host/ehci-sched.c | 2 +- drivers/usb/host/fhci-hcd.c | 3 +- drivers/usb/host/fhci-sched.c | 10 +-- drivers/usb/host/fhci.h | 5 +- drivers/usb/host/xhci-dbgcap.h | 3 +- drivers/usb/host/xhci-dbgtty.c | 15 ++-- include/linux/hyperv.h | 2 +- include/linux/usb/cdc_ncm.h | 2 +- include/linux/usb/usbnet.h | 2 +- 186 files changed, 1135 insertions(+), 1044 deletions(-)