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

[Koichiro Den]

On Fri, Sep 18, 2026 at 07:43:26AM -0700, Dave Jiang wrote:
> 
> 
> On 9/12/26 7:52 AM, Koichiro Den wrote:
> > 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.
> 
> I'm fine with it if it doesn't break any existing code and reduces complexity with simpler code.

Alright, I'll look into it when I have some bandwidth to see if it's really
worth doing for mainline.

Thanks for the review!
And thanks to Logan and Frank as well for taking the time to look at this.
(Saying it here to save ML traffic :))

Best regards,
Koichiro

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