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

[Frank Li]

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.

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
> > > >