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

[Dave Jiang <dave.jiang@intel.com>]

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@vger.kernel.org
>>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>>>>> Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
>>>>>> Signed-off-by: Koichiro Den <den@valinux.co.jp>
>>>>>> ---
>>>>>> 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.

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@valinux.co.jp/
>>>
>>> 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
>>>>>>