Re: [PATCH] sfc: Use acquire/release for irq_soft_enabled

[Gui-Dong Han]

On Thu, Jun 4, 2026 at 7:52 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Wed, 3 Jun 2026 21:28:02 +0100 Edward Cree wrote:
> > On 03/06/2026 19:20, Jakub Kicinski wrote:
> > > On Thu, 28 May 2026 17:28:38 +0800 Gui-Dong Han wrote:
> > >> Use release stores when updating the gate and acquire loads in interrupt
> > >> handlers before touching channels. Keep the existing smp_wmb() after gate
> > >> updates, preserving the current ordering with event queue start and stop.
> > >
> > > Why are you keeping the smp_wmb() tho? You don't have to repeat what
> > > the patch does. The goal of the commit message is to explain the why.
> > >
> > > Ed, WDYT?
> >
> > I think... that my head hurts from trying to understand this.
> > What I particularly don't understand is why it wouldn't suffice to just
> >  put smp_rmb() after every read of irq_soft_enabled, pairing with the
> >  existing smp_wmb().
>
> Exactly..
>
> > And, conversely, why any of this works at all — what stops an interrupt
> >  handler passing the gate, because the write to irq_soft_enabled has
> >  propagated to its CPU, but the writes to channel pointers haven't?
> >  Naïvely I'd expect the smp_wmb() in efx_soft_enable_interrupts() to
> >  come _before_ the write to irq_soft_enabled.
> > (The smp_wmb() in efx_soft_disable_interrupts(), I'm not sure exactly
> >  what that pairs with, since the ordering that matters there is between
> >  the irq_soft_enabled=false write and the synchronize_irq().)

The issue I was trying to fix is the transition to true.  The interrupt
handler uses irq_soft_enabled as a gate before it touches channel state,
for example efx->channel[context->index].  So when the handler observes
irq_soft_enabled == true, it must also observe the channel state that was
set up before the gate was opened.

The existing barriers do not provide that ordering.  There is no read
barrier after the READ_ONCE() in the interrupt handlers.  Also, the
existing smp_wmb() in efx_soft_enable_interrupts() is after the
irq_soft_enabled=true store, so it cannot publish earlier channel state
before that store becomes visible.

A writer-side smp_wmb() before irq_soft_enabled=true plus a reader-side
smp_rmb() after reading true would provide the needed ordering.  I used
smp_store_release() and smp_load_acquire() instead because they express
that flag-based synchronization directly and are usually cheaper.

I kept the existing smp_wmb() because it seems to provide a different
ordering, between opening/closing the software IRQ gate and the later
event queue start/stop operations.  That barrier is not documented, so I
did not want to remove it in the same patch.

> >
> > Anyway, this code predates my involvement with sfc and I've never had
> >  to touch it, so frankly I don't know any more here than anyone else,
> >  and if a memory-models expert reviews it and says 'this is needed and
> >  correct', that would mean more than a review tag from me.
> > That's not a cop-out "I won't review this", rather I'm saying that if
> >  Gui-Dong can't convince me because I'm too dumb, but can convince you,
> >  then you don't need to wait for me to show up and add my tag.
>
> I haven't dug into the code too much but FWIW having a store_release
> barrier which flips something _both_ to true and false strikes me as
> highly suspicious. So the patch needs a much better commit description
> at the very least.

Agreed, the false transition does not need release ordering.  Handlers
that observe false do not touch channel state.  I changed that side in v1
only for helper consistency.

If this direction sounds OK, I can send a v2 that keeps WRITE_ONCE() for
the false transition, uses release only for the true transition, and
improves the commit message.

Thanks Jakub, Edward, and Simon for the careful review.  If there is anything
else you would like changed in v2, such as splitting the patch, please
let me know.

Thanks.