[RFC 0/1] refactor all tasklet users into other APIs

[RFC 0/1] refactor all tasklet users into other APIs

[Allen Pais]

 Tasklet is an old API that will be eventually deprecated.
During the modernization of the tasklets API, there was a
request to entirely remove the API from the kernel. 
This series converts tasklets to simple work.

 Feedback on the series would be of great help as we are in the
process of dropping the usage of tasklets from the rest of the sub-systems.

 This is part of KSPP effort which is tracked at:
https://github.com/KSPP/linux/issues/94

This series replaces tasklets in drivers/dma/* with simple
workqueue. 

Allen Pais (1):
  drivers/dma/*: replace tasklets with workqueue

 drivers/dma/altera-msgdma.c                   | 15 ++++----
 drivers/dma/at_hdmac.c                        | 16 ++++-----
 drivers/dma/at_hdmac_regs.h                   |  6 ++--
 drivers/dma/at_xdmac.c                        | 14 ++++----
 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            |  4 +--
 drivers/dma/dw/core.c                         | 13 +++----
 drivers/dma/dw/regs.h                         |  2 +-
 drivers/dma/ep93xx_dma.c                      | 15 ++++----
 drivers/dma/fsl-edma-common.c                 |  2 +-
 drivers/dma/fsl-qdma.c                        |  2 +-
 drivers/dma/fsl_raid.c                        | 12 ++++---
 drivers/dma/fsl_raid.h                        |  2 +-
 drivers/dma/fsldma.c                          | 15 ++++----
 drivers/dma/fsldma.h                          |  2 +-
 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                        | 19 +++++-----
 drivers/dma/ioat/dma.h                        |  4 +--
 drivers/dma/ioat/init.c                       |  2 +-
 drivers/dma/iop-adma.c                        | 12 +++----
 drivers/dma/ipu/ipu_idmac.c                   | 22 ++++--------
 drivers/dma/ipu/ipu_intern.h                  |  2 +-
 drivers/dma/k3dma.c                           | 19 +++++-----
 drivers/dma/mediatek/mtk-cqdma.c              | 35 ++++++++++---------
 drivers/dma/mediatek/mtk-hsdma.c              |  4 +--
 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                          |  4 +--
 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                           | 32 +++++++++--------
 drivers/dma/plx_dma.c                         | 13 +++----
 drivers/dma/ppc4xx/adma.c                     | 17 ++++-----
 drivers/dma/ppc4xx/adma.h                     |  4 +--
 drivers/dma/ptdma/ptdma-dev.c                 |  2 +-
 drivers/dma/ptdma/ptdma.h                     |  4 +--
 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                      |  6 ++--
 drivers/dma/qcom/hidma_ll.c                   | 11 +++---
 drivers/dma/qcom/qcom_adm.c                   |  2 +-
 drivers/dma/s3c24xx-dma.c                     |  2 +-
 drivers/dma/sa11x0-dma.c                      | 27 +++++++-------
 drivers/dma/sf-pdma/sf-pdma.c                 | 24 +++++++------
 drivers/dma/sf-pdma/sf-pdma.h                 |  4 +--
 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/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                        | 30 ++++++++--------
 drivers/dma/txx9dmac.h                        |  4 +--
 drivers/dma/virt-dma.c                        |  9 ++---
 drivers/dma/virt-dma.h                        |  8 ++---
 drivers/dma/xgene-dma.c                       | 21 +++++------
 drivers/dma/xilinx/xilinx_dma.c               | 23 ++++++------
 drivers/dma/xilinx/xilinx_dpdma.c             | 19 +++++-----
 drivers/dma/xilinx/zynqmp_dma.c               | 21 +++++------
 include/linux/platform_data/dma-iop32x.h      |  4 +--
 80 files changed, 459 insertions(+), 429 deletions(-)

-- 
2.17.1

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Kees Cook]

On Tue, Apr 19, 2022 at 09:16:58PM +0000, Allen Pais wrote:
> The tasklet is an old API which will be deprecated, workqueue API
> cab be used instead of them.
> 
> This patch replaces the tasklet usage in drivers/dma/* with a
> simple work.

Thanks for working on this! Can you describe the process for the
replacement? For example, was it a 1:1 replacement from one set of
functions to another, or something more nuanced? Did you do this by hand,
or with the help of something like Coccinelle?

Including these details will help maintainers with review, and will help
others do other conversions in the future.

Thanks!

-Kees

-- 
Kees Cook

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

>> The tasklet is an old API which will be deprecated, workqueue API
>> cab be used instead of them.
>> 
>> This patch replaces the tasklet usage in drivers/dma/* with a
>> simple work.
> 
> Thanks for working on this! Can you describe the process for the
> replacement? For example, was it a 1:1 replacement from one set of
> functions to another, or something more nuanced? Did you do this by hand,
> or with the help of something like Coccinelle?

 For this series, I did the work manually as certain had the potential to be improved.
Also, there are drivers which use queue_work() instead of schedule_work(). 

> Including these details will help maintainers with review, and will help
> others do other conversions in the future.
> 

 I will wait for a couple of more days and send out the rest of the series with
More details.

Thanks.

> Thanks!
> 
> -Kees
> 
> -- 
> Kees Cook

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Linus Walleij]

On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:

> The tasklet is an old API which will be deprecated, workqueue API
> cab be used instead of them.
>
> This patch replaces the tasklet usage in drivers/dma/* with a
> simple work.
>
> Github: https://github.com/KSPP/linux/issues/94
>
> Signed-off-by: Allen Pais <apais@linux.microsoft.com>

Booted on:

>  drivers/dma/ste_dma40.c                       | 17 ++++-----

This DMA-controller with no regressions:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Also looks good so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Krzysztof Kozlowski]

On 19/04/2022 23:16, Allen Pais wrote:
> The tasklet is an old API which will be deprecated, workqueue API
> cab be used instead of them.
> 

Thank you for your patch. There is something to discuss/improve.

> This patch replaces the tasklet usage in drivers/dma/* with a
> simple work.

Minor nits:

1. Don't use "this patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
2. Use subject prefix matching subsystem (git log --oneline)

> 
> Github: https://github.com/KSPP/linux/issues/94

3. No external references to some issue management systems, change-ids
etc. Lore link could work, but it's not relevant here, I guess.

Best regards,
Krzysztof

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Gustavo A. R. Silva]

On 4/25/22 10:56, Krzysztof Kozlowski wrote:
> On 19/04/2022 23:16, Allen Pais wrote:
>> The tasklet is an old API which will be deprecated, workqueue API
>> cab be used instead of them.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> This patch replaces the tasklet usage in drivers/dma/* with a
>> simple work.
> 
> Minor nits:
> 
> 1. Don't use "this patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 2. Use subject prefix matching subsystem (git log --oneline)
> 
>>
>> Github: https://github.com/KSPP/linux/issues/94
> 
> 3. No external references to some issue management systems, change-ids
> etc. Lore link could work, but it's not relevant here, I guess.

I think the link to the KSPP issue tracker should stay. If something,
just changing 'Github:' to 'Link:'

The KSPP has been an active _upstream_ project for about 7 years now,
and the issue tracker is publicly available. :) So it's not like a random
link to a random project. This also help us to automatically keep track
of the patches sent out to address a particular issue that we want to
have resolved upstream. So please, keep the link in the changelog text,
it's useful. :)

Thanks
--
Gustavo

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

> On 25-Apr-2022, at 8:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:
> 
>> The tasklet is an old API which will be deprecated, workqueue API
>> cab be used instead of them.
>> 
>> This patch replaces the tasklet usage in drivers/dma/* with a
>> simple work.
>> 
>> Github: https://github.com/KSPP/linux/issues/94
>> 
>> Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> 
> Booted on:
> 
>> drivers/dma/ste_dma40.c                       | 17 ++++-----
> 
> This DMA-controller with no regressions:
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Also looks good so:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

 Thanks for review and testing.

> 
> Yours,
> Linus Walleij

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

>> The tasklet is an old API which will be deprecated, workqueue API
>> cab be used instead of them.
>> 
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> This patch replaces the tasklet usage in drivers/dma/* with a
>> simple work.
> 
> Minor nits:
> 
> 1. Don't use "this patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 2. Use subject prefix matching subsystem (git log --oneline)

 Thank you. Will have it updated.

> 
>> 
>> Github: https://github.com/KSPP/linux/issues/94
> 
> 3. No external references to some issue management systems, change-ids
> etc. Lore link could work, but it's not relevant here, I guess.

We have had other tasks in the past which have carried the links. Ideally, I would like to 
Keep it, I will leave it to the maintainers. 

Thanks.

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Vinod Koul]

On 19-04-22, 21:16, Allen Pais wrote:
> The tasklet is an old API which will be deprecated, workqueue API
> cab be used instead of them.

What is the reason for tasklet removal, I am not sure old is a reason to
remove an API...

> 
> This patch replaces the tasklet usage in drivers/dma/* with a
> simple work.

Dmaengines need very high throughput, one of the reasons in dmaengine
API design to use tasklet was higher priority given to them. Will the
workqueue allow that...?

-- 
~Vinod

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Dave Jiang]

On 4/26/2022 7:45 PM, Vinod Koul wrote:
> On 19-04-22, 21:16, Allen Pais wrote:
>> The tasklet is an old API which will be deprecated, workqueue API
>> cab be used instead of them.
> What is the reason for tasklet removal, I am not sure old is a reason to
> remove an API...
>
>> This patch replaces the tasklet usage in drivers/dma/* with a
>> simple work.
> Dmaengines need very high throughput, one of the reasons in dmaengine
> API design to use tasklet was higher priority given to them. Will the
> workqueue allow that...?

Wouldn't the logical move be to convert threaded irq IF tasklets are 
being deprecated rather than using workqueue as replacement?

Also, wouldn't all the spin_lock_bh() calls need to be changed to 
spin_lock_irqsave() now? Probably not as simple as just replace tasklet 
with workqueue.

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

>> The tasklet is an old API which will be deprecated, workqueue API
>> cab be used instead of them.
> 
> What is the reason for tasklet removal, I am not sure old is a reason to
> remove an API...

 While we moved to modernised version of tasklet API, there was
A request to entirely remove tasklets. The following patch is an attempt
Towards it. 

> 
>> 
>> This patch replaces the tasklet usage in drivers/dma/* with a
>> simple work.
> 
> Dmaengines need very high throughput, one of the reasons in dmaengine
> API design to use tasklet was higher priority given to them. Will the
> workqueue allow that...?

  It is interesting that you brought up this point. I haven’t had the opportunity
To test the changes by stressing the kernel, what tests/benchmarks would
You prefer to see run on with these changes.

Thanks.

> 
> -- 
> ~Vinod

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

>> On 19-04-22, 21:16, Allen Pais wrote:
>>> The tasklet is an old API which will be deprecated, workqueue API
>>> cab be used instead of them.
>> What is the reason for tasklet removal, I am not sure old is a reason to
>> remove an API...
>> 
>>> This patch replaces the tasklet usage in drivers/dma/* with a
>>> simple work.
>> Dmaengines need very high throughput, one of the reasons in dmaengine
>> API design to use tasklet was higher priority given to them. Will the
>> workqueue allow that...?
> 
> Wouldn't the logical move be to convert threaded irq IF tasklets are being deprecated rather than using workqueue as replacement?

  Logically yes. Would all tasklets need to be moved to threaded irq, that I am not sure. I think
Workqueues does the job.

> 
> Also, wouldn't all the spin_lock_bh() calls need to be changed to spin_lock_irqsave() now? Probably not as simple as just replace tasklet with workqueue.
> 

Yes, this was carefully looked at as we have moved from the interrupt/softirq context to process context.

Thanks.

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Andy Shevchenko]

On Mon, Apr 25, 2022 at 02:55:22PM -0500, Gustavo A. R. Silva wrote:
> On 4/25/22 10:56, Krzysztof Kozlowski wrote:
> > On 19/04/2022 23:16, Allen Pais wrote:

...

> > > Github: https://github.com/KSPP/linux/issues/94
> > 
> > 3. No external references to some issue management systems, change-ids
> > etc. Lore link could work, but it's not relevant here, I guess.
> 
> I think the link to the KSPP issue tracker should stay. If something,
> just changing 'Github:' to 'Link:'

BugLink: would be more explicit.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Linus Walleij]

On Fri, May 6, 2022 at 7:43 PM Allen Pais <apais@linux.microsoft.com> wrote:

>  - Concerns regarding throughput, would workqueues be as efficient as tasklets (Vinod)

You need to ask the scheduler people about this.

The workqueues goes deep into the scheduler and I can't make
out how they are prioritized, but they are certainly not treated
like any other task.

Yours,
Linus Walleij

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Vinod Koul]

On 12-05-22, 23:54, Linus Walleij wrote:
> On Fri, May 6, 2022 at 7:43 PM Allen Pais <apais@linux.microsoft.com> wrote:
> 
> >  - Concerns regarding throughput, would workqueues be as efficient as tasklets (Vinod)
> 
> You need to ask the scheduler people about this.
> 
> The workqueues goes deep into the scheduler and I can't make
> out how they are prioritized, but they are certainly not treated
> like any other task.

+1

-- 
~Vinod

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Linus Walleij]

On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:

> The tasklet is an old API which will be deprecated, workqueue API
> cab be used instead of them.
>
> This patch replaces the tasklet usage in drivers/dma/* with a
> simple work.
>
> Github: https://github.com/KSPP/linux/issues/94
>
> Signed-off-by: Allen Pais <apais@linux.microsoft.com>

Paging Vincent Guittot and Arnd Bergmann on the following question
on this patch set:

- Will replacing tasklets with workque like this negatively impact the
  performance on DMA engine bottom halves?

For reference:
https://lore.kernel.org/dmaengine/YoI4J8taHehMpjFj@matsya/

Yours,
Linus Walleij

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Vincent Guittot]

On Wed, 25 May 2022 at 11:24, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:
>
> > The tasklet is an old API which will be deprecated, workqueue API
> > cab be used instead of them.
> >
> > This patch replaces the tasklet usage in drivers/dma/* with a
> > simple work.
> >
> > Github: https://github.com/KSPP/linux/issues/94
> >
> > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
>
> Paging Vincent Guittot and Arnd Bergmann on the following question
> on this patch set:
>
> - Will replacing tasklets with workque like this negatively impact the
>   performance on DMA engine bottom halves?

workqueue uses cfs thread so they will be scheduled like any other
thread. Furthermore, schedule_work uses the default system workqueue
which is shared with a lot of other stuff and doesn't parallel work so
you can expect some performance impact.
If you really want to use workqueue, you should at least create your
own workqueue. But you should also have a look at irq_work; An example
of usage is kernel/sched/sched_cpufrequtil.c

>
> For reference:
> https://lore.kernel.org/dmaengine/YoI4J8taHehMpjFj@matsya/
>
> Yours,
> Linus Walleij

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Arnd Bergmann]

On Wed, May 25, 2022 at 11:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:
>
> > The tasklet is an old API which will be deprecated, workqueue API
> > cab be used instead of them.
> >
> > This patch replaces the tasklet usage in drivers/dma/* with a
> > simple work.
> >
> > Github: https://github.com/KSPP/linux/issues/94
> >
> > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
>
> Paging Vincent Guittot and Arnd Bergmann on the following question
> on this patch set:
>
> - Will replacing tasklets with workque like this negatively impact the
>   performance on DMA engine bottom halves?

I think it will in some cases but not others. The problem I see is that
the short patch description makes it sound like a trivial conversion of a
single subsystem, but in reality this interacts with all the drivers using
DMA engines, including tty/serial, sound, mmc and spi.

In many cases, the change is an improvement, but I can see a number
of ways this might go wrong:

- for audio, waiting to schedule the workqueue task may add enough
  latency to lead to audible skips

- for serial, transferring a few characters through DMA is probably
  more expensive now than using MMIO, which might mean that
  there may no longer be a point in using DMA in the first place.

- Some drivers such as dw_mmc schedule another tasklet from the
  callback. If the tasklet is turned into a workqueue, this becomes
  a bit pointless unless we change the called drivers first.

What might work better in the case of the dmaengine API would
be an approach like:

1. add helper functions to call the callback functions from a
    tasklet locally defined in drivers/dma/dmaengine.c to allow
    deferring it from hardirq context

2. Change all  tasklets that are not part of the callback
    mechanism to work queue functions, I only see
    xilinx_dpdma_chan_err_task in the patch, but there
    may be more

3. change all drivers to move their custom tasklets back into
    hardirq context and instead call the new helper for deferring
    the callback.

4. Extend the dmaengine callback API to let slave drivers
    pick hardirq, tasklet or task context for the callback.
    task context can mean either a workqueue, or a threaded
    IRQ here, with the default remaining the tasklet version.

5. Change slave drivers to pick either hardirq or task context
    depending on their requirements

6. Remove the tasklet version.

This is of course a lot more complicated than Allen's
approach, but I think the end result would be much better.

         Arnd

RE: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[David Laight]

From: Arnd Bergmann
> Sent: 25 May 2022 12:04
> 
> On Wed, May 25, 2022 at 11:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:
> >
> > > The tasklet is an old API which will be deprecated, workqueue API
> > > cab be used instead of them.
> > >
> > > This patch replaces the tasklet usage in drivers/dma/* with a
> > > simple work.
> > >
> > > Github: https://github.com/KSPP/linux/issues/94
> > >
> > > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> >
> > Paging Vincent Guittot and Arnd Bergmann on the following question
> > on this patch set:
> >
> > - Will replacing tasklets with workque like this negatively impact the
> >   performance on DMA engine bottom halves?
> 
> I think it will in some cases but not others. The problem I see is that
> the short patch description makes it sound like a trivial conversion of a
> single subsystem, but in reality this interacts with all the drivers using
> DMA engines, including tty/serial, sound, mmc and spi.
> 
> In many cases, the change is an improvement, but I can see a number
> of ways this might go wrong:

If the 'tasklet' API is based on the softint (or similar)
then changing to workqueue will cause serious grief in many cases
unless the workqueue process runs at a high priority.

Currently softint callbacks are usually higher priority than
any task/process.
So on a busy system they almost always run.
(They can get caught out by a need_resched() call and suddenly
be fighting with normal user processes for cpu time.)

As arnd said, I suspect this will break anything using tasklets
to chain together audio or video buffers.
Any process code doing that would need probbaly to run at a
'middling' RT priority.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Arnd Bergmann]

On Wed, May 25, 2022 at 6:56 PM Allen Pais <apais@linux.microsoft.com> wrote:
>
> Thank you Linus, Arnd, Vincent and David for the feedback.
>
> This is a lot more than a just conversion of API’s. I am in the process
> Of replacing tasklets with threaded irq’s and hopefully that should be
> A better solution than using workqueues.

I don't think that is much better for the case of the DMA engine
callbacks than the work queue, the problem I pointed out here
is scheduling into task context, which may be too expensive
in some cases, but whether it is or not depends on the slave
driver, not the dmaengine driver.

Even if all the slave drivers are better off using task context
(threaded irq or workqueue), you need to look at the slave
drivers first before you can convert the dmaengine drivers.

        Arnd

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

>> 
>> Thank you Linus, Arnd, Vincent and David for the feedback.
>> 
>> This is a lot more than a just conversion of API’s. I am in the process
>> Of replacing tasklets with threaded irq’s and hopefully that should be
>> A better solution than using workqueues.
> 
> I don't think that is much better for the case of the DMA engine
> callbacks than the work queue, the problem I pointed out here
> is scheduling into task context, which may be too expensive
> in some cases, but whether it is or not depends on the slave
> driver, not the dmaengine driver.

Fair point. Deferring all callbacks to task context is not the ideal
Way forward. I will work on the approach you shared,


>1. add helper functions to call the callback functions from a
>tasklet locally defined in drivers/dma/dmaengine.c to allow
>deferring it from hardirq context

>2. Change all tasklets that are not part of the callback
>mechanism to work queue functions, I only see
>xilinx_dpdma_chan_err_task in the patch, but there
>may be more

>3. change all drivers to move their custom tasklets back into
>hardirq context and instead call the new helper for deferring
>the callback.

>4. Extend the dmaengine callback API to let slave drivers
>pick hardirq, tasklet or task context for the callback.
>task context can mean either a workqueue, or a threaded
>IRQ here, with the default remaining the tasklet version.

>5. Change slave drivers to pick either hardirq or task context
>depending on their requirements

>6. Remove the tasklet version.

Thanks.

> 
> Even if all the slave drivers are better off using task context
> (threaded irq or workqueue), you need to look at the slave
> drivers first before you can convert the dmaengine drivers.
> 

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Vinod Koul]

On 25-05-22, 13:03, Arnd Bergmann wrote:
> On Wed, May 25, 2022 at 11:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Apr 19, 2022 at 11:17 PM Allen Pais <apais@linux.microsoft.com> wrote:
> >
> > > The tasklet is an old API which will be deprecated, workqueue API
> > > cab be used instead of them.
> > >
> > > This patch replaces the tasklet usage in drivers/dma/* with a
> > > simple work.
> > >
> > > Github: https://github.com/KSPP/linux/issues/94
> > >
> > > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> >
> > Paging Vincent Guittot and Arnd Bergmann on the following question
> > on this patch set:
> >
> > - Will replacing tasklets with workque like this negatively impact the
> >   performance on DMA engine bottom halves?
> 
> I think it will in some cases but not others. The problem I see is that
> the short patch description makes it sound like a trivial conversion of a
> single subsystem, but in reality this interacts with all the drivers using
> DMA engines, including tty/serial, sound, mmc and spi.
> 
> In many cases, the change is an improvement, but I can see a number
> of ways this might go wrong:
> 
> - for audio, waiting to schedule the workqueue task may add enough
>   latency to lead to audible skips
> 
> - for serial, transferring a few characters through DMA is probably
>   more expensive now than using MMIO, which might mean that
>   there may no longer be a point in using DMA in the first place.
> 
> - Some drivers such as dw_mmc schedule another tasklet from the
>   callback. If the tasklet is turned into a workqueue, this becomes
>   a bit pointless unless we change the called drivers first.

Yes and there are assumptions in the peripheral drivers about the
context of callback which right now is tasklet, that needs to be updated
as well..

> What might work better in the case of the dmaengine API would
> be an approach like:
> 
> 1. add helper functions to call the callback functions from a
>     tasklet locally defined in drivers/dma/dmaengine.c to allow
>     deferring it from hardirq context
> 
> 2. Change all  tasklets that are not part of the callback
>     mechanism to work queue functions, I only see
>     xilinx_dpdma_chan_err_task in the patch, but there
>     may be more
> 
> 3. change all drivers to move their custom tasklets back into
>     hardirq context and instead call the new helper for deferring
>     the callback.
> 
> 4. Extend the dmaengine callback API to let slave drivers
>     pick hardirq, tasklet or task context for the callback.
>     task context can mean either a workqueue, or a threaded
>     IRQ here, with the default remaining the tasklet version.

That does sound a good idea, but I dont know who will use the workqueue
or a threaded context here, it might be that most would default to
hardirq or tasklet context for obvious reasons...

> 
> 5. Change slave drivers to pick either hardirq or task context
>     depending on their requirements
> 
> 6. Remove the tasklet version.
> 
> This is of course a lot more complicated than Allen's
> approach, but I think the end result would be much better.
> 
>          Arnd

-- 
~Vinod

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Arnd Bergmann]

On Fri, May 27, 2022 at 10:06 AM Vinod Koul <vkoul@kernel.org> wrote:
> On 25-05-22, 13:03, Arnd Bergmann wrote:
> > What might work better in the case of the dmaengine API would
> > be an approach like:
> >
> > 1. add helper functions to call the callback functions from a
> >     tasklet locally defined in drivers/dma/dmaengine.c to allow
> >     deferring it from hardirq context
> >
> > 2. Change all  tasklets that are not part of the callback
> >     mechanism to work queue functions, I only see
> >     xilinx_dpdma_chan_err_task in the patch, but there
> >     may be more
> >
> > 3. change all drivers to move their custom tasklets back into
> >     hardirq context and instead call the new helper for deferring
> >     the callback.
> >
> > 4. Extend the dmaengine callback API to let slave drivers
> >     pick hardirq, tasklet or task context for the callback.
> >     task context can mean either a workqueue, or a threaded
> >     IRQ here, with the default remaining the tasklet version.
>
> That does sound a good idea, but I dont know who will use the workqueue
> or a threaded context here, it might be that most would default to
> hardirq or tasklet context for obvious reasons...

If the idea is to remove tasklets from the kernel for good, then the
choice is only between workqueue and hardirq at this point. The
workqueue version is the one that would make sense for any driver
that just defers execution from the callback down into task context.
If that gets called in task context already, the driver can be simpler.

I took a brief look at the roughly 150 slave drivers, and it does
seem like very few of them actually want task context:

* Over Half the drivers just do a complete(), which could
  probably be pulled into the dmaengine layer and done from
  hardirq, avoiding the callback entirely

* A lot of the remaining drivers have interrupts disabled for
  the entire callback, which means they might as well use
  hardirqs, regardless of what they want

* drivers/crypto/* and drivers/mmc/* tend to call another tasklet
  to do the real work.

* drivers/ata/sata_dwc_460ex.c and drivers/ntb/ntb_transport.c
   probably want task context

* Some drivers like sound/soc/sh/siu_pcm.c start a new DMA
  from the callback. Is that allowed from hardirq?

If we do the first three steps above, and then add a 'struct
completion' pointer to dma_async_tx_descriptor as an alternative
to the callback, that would already reduce the number of drivers
that end up in a tasklet significantly and should be completely
safe.

Unfortunately we can't just move the rest into hardirq
context because that breaks anything using spin_lock_bh
to protect against concurrent execution of the tasklet.

A possible alternative might be to then replace the global
dmaengine tasklet with a custom softirq. Obviously those
are not so hot either,  but dmaengine could be considered
special enough to fit in the same category as net_rx/tx
and block with their global softirqs.

       Arnd

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Vinod Koul]

On 27-05-22, 12:59, Arnd Bergmann wrote:
> On Fri, May 27, 2022 at 10:06 AM Vinod Koul <vkoul@kernel.org> wrote:
> > On 25-05-22, 13:03, Arnd Bergmann wrote:
> > > What might work better in the case of the dmaengine API would
> > > be an approach like:
> > >
> > > 1. add helper functions to call the callback functions from a
> > >     tasklet locally defined in drivers/dma/dmaengine.c to allow
> > >     deferring it from hardirq context
> > >
> > > 2. Change all  tasklets that are not part of the callback
> > >     mechanism to work queue functions, I only see
> > >     xilinx_dpdma_chan_err_task in the patch, but there
> > >     may be more
> > >
> > > 3. change all drivers to move their custom tasklets back into
> > >     hardirq context and instead call the new helper for deferring
> > >     the callback.
> > >
> > > 4. Extend the dmaengine callback API to let slave drivers
> > >     pick hardirq, tasklet or task context for the callback.
> > >     task context can mean either a workqueue, or a threaded
> > >     IRQ here, with the default remaining the tasklet version.
> >
> > That does sound a good idea, but I dont know who will use the workqueue
> > or a threaded context here, it might be that most would default to
> > hardirq or tasklet context for obvious reasons...
> 
> If the idea is to remove tasklets from the kernel for good, then the
> choice is only between workqueue and hardirq at this point. The
> workqueue version is the one that would make sense for any driver
> that just defers execution from the callback down into task context.
> If that gets called in task context already, the driver can be simpler.
> 
> I took a brief look at the roughly 150 slave drivers, and it does
> seem like very few of them actually want task context:
> 
> * Over Half the drivers just do a complete(), which could
>   probably be pulled into the dmaengine layer and done from
>   hardirq, avoiding the callback entirely
> 
> * A lot of the remaining drivers have interrupts disabled for
>   the entire callback, which means they might as well use
>   hardirqs, regardless of what they want
> 
> * drivers/crypto/* and drivers/mmc/* tend to call another tasklet
>   to do the real work.
> 
> * drivers/ata/sata_dwc_460ex.c and drivers/ntb/ntb_transport.c
>    probably want task context
> 
> * Some drivers like sound/soc/sh/siu_pcm.c start a new DMA
>   from the callback. Is that allowed from hardirq?
> 
> If we do the first three steps above, and then add a 'struct
> completion' pointer to dma_async_tx_descriptor as an alternative
> to the callback, that would already reduce the number of drivers
> that end up in a tasklet significantly and should be completely
> safe.

That is a good idea, lot of drivers are waiting for completion which can
be signalled from hardirq, this would also reduce the hops we have and
help improve latency a bit. On the downside, some controllers provide
error information, which would need to be dealt with.

I will prototype this on Qcom boards I have...

> 
> Unfortunately we can't just move the rest into hardirq
> context because that breaks anything using spin_lock_bh
> to protect against concurrent execution of the tasklet.
> 
> A possible alternative might be to then replace the global
> dmaengine tasklet with a custom softirq. Obviously those
> are not so hot either,  but dmaengine could be considered
> special enough to fit in the same category as net_rx/tx
> and block with their global softirqs.

Yes that would be a very reasonable mechanism, thanks for the
suggestions.

-- 
~Vinod

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Arnd Bergmann]

On Tue, May 31, 2022 at 7:02 PM Allen Pais <apais@linux.microsoft.com> wrote:

[note: something is wrong with your email client, your previous reply appears to
be in HTML]

> > That is a good idea, lot of drivers are waiting for completion which can
> > be signalled from hardirq, this would also reduce the hops we have and
> > help improve latency a bit. On the downside, some controllers provide
> > error information, which would need to be dealt with.
>
>
>    I am not an expert in dma subsystem, but by using completion from
> Hardirq context be a concern? Especially with latency.

I don't see how: to the task waiting for the completion, there should
be no difference, and for the irq handler sending it, it just avoids
a few cycles going into softirq context.

> > Yes that would be a very reasonable mechanism, thanks for the
> > suggestions.
>
>   I have started working on the idea of global softirq. A RFC should be ready
> For review soon.

Ok, thanks!

       Arnd

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Allen Pais]

> 
> [note: something is wrong with your email client, your previous reply appears to
> be in HTML]
> 
  Sorry, fixed it.

>>> That is a good idea, lot of drivers are waiting for completion which can
>>> be signalled from hardirq, this would also reduce the hops we have and
>>> help improve latency a bit. On the downside, some controllers provide
>>> error information, which would need to be dealt with.
>> 
>> 
>>   I am not an expert in dma subsystem, but by using completion from
>> Hardirq context be a concern? Especially with latency.
> 
> I don't see how: to the task waiting for the completion, there should
> be no difference, and for the irq handler sending it, it just avoids
> a few cycles going into softirq context.

  Thanks for clarification. 

  If I have understood it correctly, your suggestion is to move the current
Callback mechanism out to dmaengine as a generic helper function
And introduce completion in dma_async_tx_descriptor to handle what
Tasklets currently do.

Thanks.

> 
>>> Yes that would be a very reasonable mechanism, thanks for the
>>> suggestions.
>> 
>>  I have started working on the idea of global softirq. A RFC should be ready
>> For review soon.
> 
> Ok, thanks!
> 
>       Arnd

Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue

[Arnd Bergmann]

On Tue, May 31, 2022 at 8:19 PM Allen Pais <apais@linux.microsoft.com> wrote:
> >>> That is a good idea, lot of drivers are waiting for completion which can
> >>> be signalled from hardirq, this would also reduce the hops we have and
> >>> help improve latency a bit. On the downside, some controllers provide
> >>> error information, which would need to be dealt with.
> >>
> >>
> >>   I am not an expert in dma subsystem, but by using completion from
> >> Hardirq context be a concern? Especially with latency.
> >
> > I don't see how: to the task waiting for the completion, there should
> > be no difference, and for the irq handler sending it, it just avoids
> > a few cycles going into softirq context.
>
>   Thanks for clarification.
>
>   If I have understood it correctly, your suggestion is to move the current
> Callback mechanism out to dmaengine as a generic helper function
> And introduce completion in dma_async_tx_descriptor to handle what
> Tasklets currently do.

Right: around half the callbacks are a trivial version that does nothing
other than calling complete(), so these will benefit from skipping the
softirq, while the others still need the callback.

       Arnd