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

[Sagi Grimberg <sagi@grimberg.me>]

On 3/22/23 08:59, Hannes Reinecke wrote:
> On 3/22/23 01:18, Chris Leech wrote:
>> On Tue, Mar 21, 2023 at 03:09:06PM +0100, 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.
>>>
>>> Hence the slightly vague description.
>>
>> This looks like the socket callbacks end up hooked in the wrong order.
>> Ideally it would be tcp -> tls -> nvme_tcp, while this currently looks
>> like tcp -> nvme_tcp and then this call back to tls for decryption.
>>
> Well, problem is that I need not one but two sets the callbacks.
> One callback is for waking up userspace (took me weeks to figure that 
> out), and needs to added before calling the userspace helper.
> The other is the 'normal' nvme-tcp callback:
> 
> tcp->nvme-upcall->tls->nvme-tcp
> 
> So really the problem is not so much an inversion, but rather the fact
> that the nvme-upcall callback is really only needed for the duration
> of the handshake. And hence I thought that we should remove the callback
> once we're done with the upcall.

What do you mean remove the callback? data_ready? I'm not sure I'm
following.

> Turns out that we can't, and the best we can do is to disable the 
> functionality, leaving the callback itself in place.
> 
>> I'm not quite sure how to untangle this; nvme_tcp can't just set it's
>> own callbacks before initializing kTLS, becuse that's being done by
>> tlshd which is going to need the userspace socket API callbacks working.
>>
> Correct.
> So for now I'll leave the callbacks in place, even though they are 
> pointless after the upcall.

Does it make any difference now that callbacks setting moved to
nvme_tcp_start_queue?

Again, I'm not sure I understand what callback is pointless?