Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
> On Jun 3, 2024, at 5:38 AM, Aubin Constans > wrote: > > On 27/03/2024 17:03, Allen Pais wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> 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 >> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 >> Signed-off-by: Allen Pais >> --- >> drivers/mmc/host/atmel-mci.c | 35 - > [...] > > For atmel-mci, judging from a few simple tests, performance is preserved. > E.g. writing to a SD Card on the SAMA5D3-Xplained board: > time dd if=/dev/zero of=/opt/_del_me bs=4k count=64k > > Base 6.9.0 : 0.07user 5.05system 0:18.92elapsed 27%CPU > Patched 6.9.0+: 0.12user 4.92system 0:18.76elapsed 26%CPU > > However, please resolve what checkpatch is complaining about: > scripts/checkpatch.pl --strict > PATCH-9-9-mmc-Convert-from-tasklet-to-BH-workqueue.mbox > > WARNING: please, no space before tabs > #72: FILE: drivers/mmc/host/atmel-mci.c:367: > +^Istruct work_struct ^Iwork;$ > > Same as discussions on the USB patch[1] and others in this series, I am also > in favour of "workqueue" or similar in the comments, rather than just "work". Will send out a new version. Thank you very much for testing and providing your review. - Allen > > Apart from that: > Tested-by: Aubin Constans > Acked-by: Aubin Constans > > Thanks. > > [1]: > https://lore.kernel.org/linux-mmc/caomdwslippfm3oztpjzz4uf4m+e_8qaotemckbxawlnktqx...@mail.gmail.com/ ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On 27/03/2024 17:03, Allen Pais wrote: EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe 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 Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 Signed-off-by: Allen Pais --- drivers/mmc/host/atmel-mci.c | 35 - [...] For atmel-mci, judging from a few simple tests, performance is preserved. E.g. writing to a SD Card on the SAMA5D3-Xplained board: time dd if=/dev/zero of=/opt/_del_me bs=4k count=64k Base 6.9.0 : 0.07user 5.05system 0:18.92elapsed 27%CPU Patched 6.9.0+: 0.12user 4.92system 0:18.76elapsed 26%CPU However, please resolve what checkpatch is complaining about: scripts/checkpatch.pl --strict PATCH-9-9-mmc-Convert-from-tasklet-to-BH-workqueue.mbox WARNING: please, no space before tabs #72: FILE: drivers/mmc/host/atmel-mci.c:367: +^Istruct work_struct ^Iwork;$ Same as discussions on the USB patch[1] and others in this series, I am also in favour of "workqueue" or similar in the comments, rather than just "work". Apart from that: Tested-by: Aubin Constans Acked-by: Aubin Constans Thanks. [1]: https://lore.kernel.org/linux-mmc/caomdwslippfm3oztpjzz4uf4m+e_8qaotemckbxawlnktqx...@mail.gmail.com/ ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:14PM +, 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 > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > Signed-off-by: Allen Pais > --- [...] > drivers/mmc/host/cb710-mmc.c | 15 ++-- > drivers/mmc/host/cb710-mmc.h | 3 +- [...] Acked-by: Michał Mirosław ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On Thu, 28 Mar 2024 at 17:21, Tejun Heo wrote: > > Hello, > > On Thu, Mar 28, 2024 at 01:53:25PM +0100, Ulf Hansson wrote: > > At this point we have suggested to drivers to switch to use threaded > > irq handlers (and regular work queues if needed too). That said, > > what's the benefit of using the BH work queue? > > BH workqueues should behave about the same as tasklets which have more > limited interface and is subtly broken in an expensive-to-fix way (around > freeing in-flight work item), so the plan is to replace tasklets with BH > workqueues and remove tasklets from the kernel. Seems like a good approach! > > The [dis]advantages of BH workqueues over threaded IRQs or regular threaded > workqueues are the same as when you compare them to tasklets. No thread > switching overhead, so latencies will be a bit tighter. Wheteher that > actually matters really depends on the use case. Here, the biggest advantage > is that it's mostly interchangeable with tasklets and can thus be swapped > easily. Right, thanks for clarifying! However, the main question is then - if/when it makes sense to use the BH workqueue for an mmc host driver. Unless there are some HW limitations, a threaded irq handler should be sufficient, I think. That said, moving to threaded irq handlers is a different topic and doesn't prevent us from moving to BH workqueues as it seems like a step in the right direction. Kind regards Uffe ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On 27/03/2024 16:03, 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. s/infiniband/mmc > > Based on the work done by Tejun Heo > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > Signed-off-by: Allen Pais > --- > 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 ++- For dw_mmc: Performance numbers look good FWIW. for i in $(seq 0 5); do echo performance > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor; done for i in $(seq 0 4); do fio --name=test --rw=randread --bs=4k --runtime=30 --time_based --filename=/dev/mmcblk1 --minimal --numjobs=6 --iodepth=32 --group_reporting | awk -F ";" '{print $8}'; sleep 30; done Baseline: 1758 1773 1619 1835 1639 to: 1743 1643 1860 1638 1872 (I'd call that equivalent). This is on a RK3399. I would prefer most of the naming to change from "work" to "workqueue" in the driver code. Apart from that: Reviewed-by: Christian Loehle Tested-by: Christian Loehle > drivers/mmc/host/omap.c | 17 +++-- > drivers/mmc/host/renesas_sdhi.h | 3 +- > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 24 +++--- See inline > 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 +-- > 18 files changed, 167 insertions(+), 157 deletions(-) > > diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c > index dba826db739a..0a92a7fd020f 100644 > --- a/drivers/mmc/host/atmel-mci.c > +++ b/drivers/mmc/host/atmel-mci.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -284,12 +285,12 @@ struct atmel_mci_dma { > * EVENT_DATA_ERROR is pending. > * @stop_cmdr: Value to be loaded into CMDR when the stop command is > * to be sent. > - * @tasklet: Tasklet running the request state machine. > + * @work: Work running the request state machine. > * @pending_events: Bitmask of events flagged by the interrupt handler > - * to be processed by the tasklet. > + * to be processed by the work. > * @completed_events: Bitmask of events which the state machine has > * processed. > - * @state: Tasklet state. > + * @state: Work state. > * @queue: List of slots waiting for access to the controller. > * @need_clock_update: Update the clock rate before the next request. > * @need_reset: Reset controller before next request. > @@ -363,7 +364,7 @@ struct atmel_mci { > u32 data_status; > u32 stop_cmdr; > > - struct tasklet_struct tasklet; > + struct work_struct work; > unsigned long pending_events; > unsigned long completed_events; > enum atmel_mci_statestate; > @@ -761,7 +762,7 @@ static void atmci_timeout_timer(struct timer_list *t) > host->need_reset = 1; > host->state = STATE_END_REQUEST; > smp_wmb(); > - tasklet_schedule(>tasklet); > + queue_work(system_bh_wq, >work); > } > > static inline unsigned int atmci_ns_to_clocks(struct atmel_mci *host, > @@ -983,7 +984,7 @@ static void atmci_pdc_complete(struct atmel_mci *host) > > dev_dbg(>pdev->dev, "(%s) set pending xfer complete\n", __func__); > atmci_set_pending(host, EVENT_XFER_COMPLETE); > - tasklet_schedule(>tasklet); > + queue_work(system_bh_wq, >work); > } > > static void atmci_dma_cleanup(struct atmel_mci *host) > @@ -997,7 +998,7 @@ static void atmci_dma_cleanup(struct atmel_mci *host) > } > > /* > - * This function is called by the DMA driver from tasklet context. > + * This function is called by the DMA driver from work context. > */ > static void atmci_dma_complete(void *arg) > { > @@ -1020,7 +1021,7 @@ static void atmci_dma_complete(void *arg) > dev_dbg(>pdev->dev, > "(%s) set pending xfer complete\n", __func__); >
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On Thu, Mar 28, 2024 at 3:16 AM Christian Loehle wrote: > > On 27/03/2024 16:03, 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. > s/infiniband/mmc Will fix it in v2. > > > > Based on the work done by Tejun Heo > > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > > > Signed-off-by: Allen Pais > > --- > > 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 ++- > For dw_mmc: > Performance numbers look good FWIW. > for i in $(seq 0 5); do echo performance > > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor; done > for i in $(seq 0 4); do fio --name=test --rw=randread --bs=4k --runtime=30 > --time_based --filename=/dev/mmcblk1 --minimal --numjobs=6 --iodepth=32 > --group_reporting | awk -F ";" '{print $8}'; sleep 30; done > Baseline: > 1758 > 1773 > 1619 > 1835 > 1639 > to: > 1743 > 1643 > 1860 > 1638 > 1872 > (I'd call that equivalent). > This is on a RK3399. > I would prefer most of the naming to change from "work" to "workqueue" in the > driver > code. > Apart from that: > Reviewed-by: Christian Loehle > Tested-by: Christian Loehle Thank you very much for testing and the review. Will have your concerns addressed in v2. - Allen ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
Hello, On Thu, Mar 28, 2024 at 01:53:25PM +0100, Ulf Hansson wrote: > At this point we have suggested to drivers to switch to use threaded > irq handlers (and regular work queues if needed too). That said, > what's the benefit of using the BH work queue? BH workqueues should behave about the same as tasklets which have more limited interface and is subtly broken in an expensive-to-fix way (around freeing in-flight work item), so the plan is to replace tasklets with BH workqueues and remove tasklets from the kernel. The [dis]advantages of BH workqueues over threaded IRQs or regular threaded workqueues are the same as when you compare them to tasklets. No thread switching overhead, so latencies will be a bit tighter. Wheteher that actually matters really depends on the use case. Here, the biggest advantage is that it's mostly interchangeable with tasklets and can thus be swapped easily. Thanks. -- tejun ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On Wed, 27 Mar 2024 at 17:03, 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 > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > Signed-off-by: Allen Pais Overall, this makes sense to me. However, just to make things clear, an mmc host driver shouldn't really need the tasklet. As I understand it, the few remaining users are there for legacy reasons. At this point we have suggested to drivers to switch to use threaded irq handlers (and regular work queues if needed too). That said, what's the benefit of using the BH work queue? Kind regards Uffe > --- > 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 +-- > 18 files changed, 167 insertions(+), 157 deletions(-) > > diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c > index dba826db739a..0a92a7fd020f 100644 > --- a/drivers/mmc/host/atmel-mci.c > +++ b/drivers/mmc/host/atmel-mci.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -284,12 +285,12 @@ struct atmel_mci_dma { > * EVENT_DATA_ERROR is pending. > * @stop_cmdr: Value to be loaded into CMDR when the stop command is > * to be sent. > - * @tasklet: Tasklet running the request state machine. > + * @work: Work running the request state machine. > * @pending_events: Bitmask of events flagged by the interrupt handler > - * to be processed by the tasklet. > + * to be processed by the work. > * @completed_events: Bitmask of events which the state machine has > * processed. > - * @state: Tasklet state. > + * @state: Work state. > * @queue: List of slots waiting for access to the controller. > * @need_clock_update: Update the clock rate before the next request. > * @need_reset: Reset controller before next request. > @@ -363,7 +364,7 @@ struct atmel_mci { > u32 data_status; > u32 stop_cmdr; > > - struct tasklet_struct tasklet; > + struct work_struct work; > unsigned long pending_events; > unsigned long completed_events; > enum atmel_mci_statestate; > @@ -761,7 +762,7 @@ static void atmci_timeout_timer(struct timer_list *t) > host->need_reset = 1; > host->state = STATE_END_REQUEST; > smp_wmb(); > - tasklet_schedule(>tasklet); > + queue_work(system_bh_wq, >work); > } > > static inline unsigned int atmci_ns_to_clocks(struct atmel_mci *host, > @@ -983,7 +984,7 @@ static void atmci_pdc_complete(struct atmel_mci *host) > > dev_dbg(>pdev->dev, "(%s) set pending xfer complete\n", > __func__); > atmci_set_pending(host, EVENT_XFER_COMPLETE); > - tasklet_schedule(>tasklet); > + queue_work(system_bh_wq, >work); > } > > static void atmci_dma_cleanup(struct atmel_mci *host) > @@ -997,7 +998,7 @@ static void atmci_dma_cleanup(struct atmel_mci *host) > } > > /* > - * This function is called by the DMA driver from tasklet context. > + * This function is called by the DMA driver from work context. > */ > static void atmci_dma_complete(void *arg) > { > @@ -1020,7 +1021,7 @@ static void atmci_dma_complete(void *arg) > dev_dbg(>pdev->dev, > "(%s) set pending xfer complete\n", __func__); > atmci_set_pending(host, EVENT_XFER_COMPLETE); > - tasklet_schedule(>tasklet); > + queue_work(system_bh_wq, >work); > > /* > * Regardless of what the documentation
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
On Thu, Mar 28, 2024 at 1:54 PM Ulf Hansson wrote: > At this point we have suggested to drivers to switch to use threaded > irq handlers (and regular work queues if needed too). That said, > what's the benefit of using the BH work queue? Context: https://lwn.net/Articles/960041/ "Tasklets, in particular, remain because they offer lower latency than workqueues which, since they must go through the CPU scheduler, can take longer to execute a deferred-work item." The BH WQ is controlled by a software IRQ and quicker than an ordinary work item. I don't know if this little latency could actually affect any MMC device, I doubt it. The other benefit IIUC is that it is easy to mechanically rewrite tasklets to BH workqueues and be sure that it is as fast as the tasklet, if you want to switch to threaded IRQ handlers or proper work, you need to write a lot of elaborate code and test it (preferably on real hardware). Yours, Linus Walleij ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 9/9] mmc: Convert from tasklet to BH workqueue
Dne sreda, 27. marec 2024 ob 17:03:14 CET je Allen Pais napisal(a): > 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. infiniband -> mmc Best regards, Jernej > > Based on the work done by Tejun Heo > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10 > > Signed-off-by: Allen Pais ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer