Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

On Fri, Sep 11, 2026 at 10:20:34PM -0500, Frank Li wrote:
> On Sat, Sep 12, 2026 at 03:21:08AM +0900, Koichiro Den wrote:
> > On Sat, Sep 12, 2026 at 03:04:21AM +0900, Koichiro Den wrote:
> > > On Fri, Sep 11, 2026 at 11:53:37AM -0500, Frank Li wrote:
> > > > On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> > > > > ntb_netdev_open() can call ntb_transport_link_up() while the transport
> > > > > worker is completing setup on another CPU. Concurrent transport setup
> > > > > and a client link-up request can both read the other's flag as false and
> > > > > leave QP link work unqueued. The QP then stays down until another link
> > > > > event or client link-up request.
> > > > >
> > > > > This is the store-buffering pattern described in
> > > > > tools/memory-model/Documentation/recipes.txt ("Store buffering").
> > > > >
> > > > > Add a full barrier between the store and load on each side, and
> > > > > mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
> > > > >
> > > > > Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > > > > Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@xxxxxxxxxxxxxxx/
> > > > > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > > > > ---
> > > > > Changes in v2:
> > > > >   - New patch (Sashiko)
> > > > >
> > > > >  drivers/ntb/ntb_transport.c | 13 +++++++++++--
> > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > > > > index 75d4a2e021f4..1332d53bcfe7 100644
> > > > > --- a/drivers/ntb/ntb_transport.c
> > > > > +++ b/drivers/ntb/ntb_transport.c
> > > > > @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
> > > > >  	/* Publish the link only after every QP has been set up. */
> > > > >  	smp_store_release(&nt->link_is_up, true);
> > > > >
> > > > > +	/*
> > > > > +	 * Prevent both sides from missing each other's flag. Pairs with
> > > > > +	 * the barrier in ntb_transport_link_up().
> > > > > +	 */
> > > > > +	smp_mb();
> > > > > +
> > > > >  	for (i = 0; i < nt->qp_count; i++) {
> > > > >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> > > > >
> > > > > -		if (qp->client_ready)
> > > > > +		if (READ_ONCE(qp->client_ready))
> > > >
> > > > I think it'd better change to use atomic variable for client_ready to avoid
> > > > manual handle smp sync.
> > >
> > > AFAICT converting client_ready to atomic_t and using atomic_set()/atomic_read()
> > > instead of WRITE_ONCE()/READ_ONCE() would not free us from the smp sync, as
> > > those operations are unordered.
> 
> there are acquire version for atomic
> 
> atomic_set_release()
> 
> atomic_read_acquire()
> 
> My key point is use existing higher level sync APIs to avoid consider
> barrier problem, which need more brain cell to think it.
> 
> this is just sync state, which don't impact performance.

I agree with the idea of changing both flags to atomic_t for cleaner code, but
I'm not sure if it frees us from "the smp sync" or the "barrier problem". We'd
still need both smp_mb()s and still have to think about *_release/*_acquire when
revisiting the code and maybe scratching our heads, right?

BTW, I once wondered whether it might be worth consolidating
link_work/link_cleanup into a single state machine worker per transport or QP.
That could simplify synchronization between setup and cleanup, which aren't
really performance-critical, as you say. However, that would possibly require a
larger rework, so for now I've put together this fix series as a small step. I
would be interested to hear what NTB maintainers think about the possibility of
such a rework.

Best regards,
Koichiro

> 
> Frank
> 
> > > So to illustrate this in litmus test like form:
> > >
> > >    # L = nt->link_is_up
> > >    # R = qp->client_ready
> > >    # Both initially false
> > >
> > >    Transport setup                  Client link-up
> > >    ---------------                  --------------
> > >     smp_store_release(&L, true);     WRITE_ONCE(R, true);
> > >     smp_mb(); /* added */            smp_mb(); /* added */
> > >     r0 = READ_ONCE(R);               r1 = smp_load_acquire(&L);
> > >
> > >    Both reads return false?
> > >     Before: allowed
> > >     After:  forbidden
> > >
> > > I might be misunderstanding your suggestion though. If you have something
> > > different in mind, please let me know.
> >
> > Just for the record, smp_store_release/smp_load_acquire depicted above, instead
> > of WRITE_ONCE()/READ_ONCE(), is intentional. They are for MP ordering to publish
> > the QP setup, which is needed by an earlier patch:
> > https://lore.kernel.org/r/20260910040836.3792333-5-den@xxxxxxxxxxxxx/
> >
> > Best regards,
> > Koichiro
> >
> > >
> > > Thanks for the review.
> > > Koichiro
> > >
> > > >
> > > > Frank
> > > >
> > > > >  			ntb_transport_schedule_qp_link(qp, 0);
> > > > >  	}
> > > > >
> > > > > @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
> > > > >  	if (!qp)
> > > > >  		return;
> > > > >
> > > > > -	qp->client_ready = true;
> > > > > +	WRITE_ONCE(qp->client_ready, true);
> > > > > +
> > > > > +	/* Pairs with the barrier in ntb_transport_link_work(). */
> > > > > +	smp_mb();
> > > > >
> > > > >  	ntb_transport_schedule_qp_link(qp, 0);
> > > > >  }
> > > > > --
> > > > > 2.51.0
> > > > >