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

[Vinod Koul <vkoul@kernel.org>]

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