Message-ID: <CAOMdWSJ7ut3n-nryTYSyPD37YwN7UqyZ1VcgZ9nmBcRF_jxH=w@mail.gmail.com>
Date: Mon, 12 Jan 2026 14:20:47 -0800
From: Allen <allen.lkml@...il.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Vinod Koul <vkoul@...nel.org>, Kees Cook <kees@...nel.org>, dmaengine@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/1] dmaengine: introduce dmaengine_bh_wq and bh helpers

> >    Thanks for the feedback. My intent with WQ_BH was to keep callbacks in
> >   softirq/BH context, but I agree the scheduling overhead and existing tasklet
> >   assumptions are valid concerns.
>
> Hi Allen,
>
> Sorry about missing the bit about WQ_BH, I forgot about the details
> of or previous discussion and this is pretty much what I had
> suggested last time,
>
Hi Arnd,

  Sorry I missed the WQ_BH point in my reply, I’d forgotten the details of our
  earlier discussion, and I should have followed up on this sooner.

> >   I can re-spin the RFC to drop the workqueue entirely and keep
> > tasklet semantics,
> >   while still abstracting tasklet handling into dmaengine helpers so drivers no
> >   longer directly manipulate tasklets. That keeps
> > dmaengine_desc_callback_invoke()
> >   in tasklet context and avoids breaking DMA users that rely on that behavior.
>
> It's probably fine to do both, but a series of two patches (first introduce
> the per-channel API, then move it over to WQ_BH) may be slightly
> clearer here.
>
> I'm not sure why the dmaengine_*_bh_wq() functions are exported
> interfaces, as far as I can tell, you use them only internally
> in the dma_chan_*_bh() functions, so making them static would
> let the compiler inline them where possible.
>
> There are of course dmaengine drivers that use tasklets for other
> purposes than the channel callback. Was your idea here to use
> the same workqueue for these? I would perhaps hold off on that for
> the moment and see if there is a better alternative for those,
> possibly hardirq context, threaded irq or a regular workqueue
> depending on the driver.
>
> >   A hardirq callback path feels like a larger API change, so I’d
> > prefer to handle that as a separate follow‑up (e.g. explicit hardirq
> >  callback/flag + user migration where safe). Thoughts?
>
> Yes, definitely keep that separate. I still think that this is
> what we want eventually for a bigger improvement, but your
> patch seems valuable on its own as well.
>

Thanks for the detailed feedback. I’ll respin along these lines:

  - Split the series into two patches: (1) introduce the per‑channel BH API,
    (2) switch the vchan implementation over to the WQ_BH backend if we decide
    to keep that step. This should make the progression clearer.

  - The dmaengine_*_bh_wq() helpers will be made static; only dma_chan_*_bh()
    stays exported.

  - I won’t try to move the other per‑driver tasklets onto the shared queue in
    this series. That feels like a separate discussion, and the right context
    (hardirq/threaded/workqueue) may vary by driver.

  - I’ll keep the hardirq callback path separate. For that follow‑up, I can plan
    to add an explicit “hardirq safe” request bit (e.g. a new dma_ctrl_flags)
    and update vchan_cookie_complete() to invoke the callback directly when
    requested; otherwise it stays on the tasklet path.

  If you’d prefer I drop the WQ_BH conversion entirely for now and keep only the
  tasklet‑based per‑channel API, I can do that too.

Thanks,
Allen


> Some more thoughts on where that later change could take us:
>
> >    /*
> >   - * This tasklet handles the completion of a DMA descriptor by
> >   - * calling its callback and freeing it.
> >   + * This bottom-half handler completes a DMA descriptor by invoking its
> >   + * callback and freeing it.
> >     */
> >   -static void vchan_complete(struct tasklet_struct *t)
> >   +static void vchan_complete(struct dma_chan *chan)
> >    {
> >   -    struct virt_dma_chan *vc = from_tasklet(vc, t, task);
> >   +    struct virt_dma_chan *vc = to_virt_chan(chan);
> >        struct virt_dma_desc *vd, *_vd;
> >        struct dmaengine_desc_callback cb;
> >        LIST_HEAD(head);
> >   @@ -131,7 +131,7 @@ void vchan_init(struct virt_dma_chan *vc, struct
> > dma_device *dmadev)
> >        INIT_LIST_HEAD(&vc->desc_completed);
> >        INIT_LIST_HEAD(&vc->desc_terminated);
> >
> >   -    tasklet_setup(&vc->task, vchan_complete);
> >   +    dma_chan_init_bh(&vc->chan, vchan_complete);
>
> This is where I think it makes sense to start, again for
> the vchan imlmenentation. What the dmaengine drivers have
> is a per-driver tasklet (or WQ_BH) with a single complete()
> function that directly calls into the client drivers for
> each completion that has happened.
>
> Since the context we want depends on the type of client
> driver, I think a good approach would be to start
> by modifying vchan_cookie_complete() to allow it to
> call the callback function directly from hardirq context
> when the client asks for that, and avoid the round trip
> through the tasklet where possible.
>
> A new bit in the dma_ctrl_flags word could be used
> to ask for hardirq vs softirq context, and the existing
> drivers just fall back to using a tasklet for softirq
> context.
>
>       Arnd



-- 
       - Allen

Powered by blists - more mailing lists