mbox series

[0/9] Convert Tasklets to BH Workqueues

Message ID 20240327160314.9982-1-apais@linux.microsoft.com
Headers show
Series Convert Tasklets to BH Workqueues | expand

Message

Allen Pais March 27, 2024, 4:03 p.m. UTC
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(-)

Comments

Greg KH March 27, 2024, 4:55 p.m. UTC | #1
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
Alan Stern March 27, 2024, 5:55 p.m. UTC | #2
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.
> 
> 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?

Alan Stern
Allen Pais March 28, 2024, 5:52 p.m. UTC | #3
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
> >
> >
>
Allen Pais March 28, 2024, 5:54 p.m. UTC | #4
> >
> > 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
>
Corey Minyard March 28, 2024, 7:23 p.m. UTC | #5
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
>
Allen Pais March 28, 2024, 7:41 p.m. UTC | #6
> > > 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
Corey Minyard March 28, 2024, 7:52 p.m. UTC | #7
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
>
Allen Pais March 28, 2024, 7:58 p.m. UTC | #8
> > > >
> > > >   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