Re: [PATCH 06/18] nvme-tcp: call 'queue->data_ready()' in nvme_tcp_data_ready()

[Hannes Reinecke <hare@suse.de>]

On 3/22/23 09:08, Sagi Grimberg wrote:
> 
> 
> On 3/21/23 16:09, Hannes Reinecke wrote:
>> On 3/21/23 14:44, Sagi Grimberg wrote:
>>>
>>>> Call the original data_ready() callback in nvme_tcp_data_ready()
>>>> to avoid a receive stall.
>>>
>>> Can you please improve the description to include what is the stall?
>>> For example, does the stall exist today? If it is, I would like to
>>> separate such patches from this set and include them asap.
>>>
>> That is actually particular to the TLS implementation, as it uses the 
>> 'data_ready' callback to produce the data which can be read by eg 
>> recvmsg().
>>
>> Without this call there's no data to peruse for recvmsg().
>>
>> But I'm not _that_ deep into networking details to know whether this 
>> is TLS specific or an issue with any data_ready callback.
>> I assume the latter, but then again, who knows.
> 
> Seems that this is only relevant when nvme_tcp runs on top of a ulp,
> so a code comment would probably make sense here.
> 
>>
>> Hence the slightly vague description.
>>
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>   drivers/nvme/host/tcp.c | 8 +++++---
>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>> index 0e14b1b90855..0512eb289dcf 100644
>>>> --- a/drivers/nvme/host/tcp.c
>>>> +++ b/drivers/nvme/host/tcp.c
>>>> @@ -936,12 +936,14 @@ static void nvme_tcp_data_ready(struct sock *sk)
>>>>       trace_sk_data_ready(sk);
>>>> -    read_lock_bh(&sk->sk_callback_lock);
>>>> -    queue = sk->sk_user_data;
>>>> +    rcu_read_lock_bh();
>>>
>>> Now I understand your comment from a previous patch.
>>> Can you explain why is this convention needed?
>>>
>>> I would prefer to have it as a separate patch with an
>>> explanation to why it is needed.
>>>
>> This is the slightly odd socket callback handling.
>> Any driver is free to set the socket callbacks, but it has to be aware 
>> that it might not be the only one in the stack doing so.
>> So one has to be prepared that the callbacks are set already, so we 
>> should be calling them prior to our callback.
> 
> I meant the change from read_lock_bh to rcu_read_lock_bh and the
> same for rcu_dereference_sk_user_data. It needs to be in a
> separate patch with explanation to why it is needed.
> 
That is primarily for sanity checking.

Both us _and_ the tls code are setting the sk_user_data pointer,
so I wanted to make sure that we do get things correct.
But seems to work now, so I guess I can drop the rcu_dereference
modifications.

Let's see.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman