[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c05ac7b9-d71b-4069-ac73-19a082eea559@suse.de>
Date: Wed, 8 Oct 2025 08:28:09 +0200
From: Hannes Reinecke <hare@...e.de>
To: Wilfred Mallawa <wilfred.opensource@...il.com>,
linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Cc: Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...nel.dk>,
Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
John Fastabend <john.fastabend@...il.com>, Jakub Kicinski <kuba@...nel.org>,
Sabrina Dubroca <sd@...asysnail.net>, "David S . Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH] nvme/tcp: handle tls partially sent records in
write_space()
On 10/8/25 04:11, Wilfred Mallawa wrote:
> On Tue, 2025-10-07 at 11:51 +0200, Hannes Reinecke wrote:
>> On 10/7/25 11:24, Wilfred Mallawa wrote:
>>> On Tue, 2025-10-07 at 07:19 +0200, Hannes Reinecke wrote:
>>>> On 10/7/25 02:46, Wilfred Mallawa wrote:
>>>>> From: Wilfred Mallawa <wilfred.mallawa@....com>
>>>>>
>>>>
>>> [...]
>>>> I wonder: Do we really need to check for a partially assembled
>>>> record,
>>>> or wouldn't it be easier to call queue->write_space() every time
>>>> here?
>>>> We sure would end up with executing the callback more often, but
>>>> if
>>>> no
>>>> data is present it shouldn't do any harm.
>>>>
>>>> IE just use
>>>>
>>>> if (nvme_tcp_queue_tls(queue)
>>>> queue->write_space(sk);
>>>
>>> Hey Hannes,
>>>
>>> This was my initial approach, but I figured using
>>> tls_is_partially_sent_record() might be slightly more efficient.
>>> But if
>>> we think that's negligible, happy to go with this approach
>>> (omitting
>>> the partial record check).
>>>
>> Please do.
>> Performance testing on NVMe-TCP is notoriously tricky, so for now we
>> really should not assume anything here.
>> And it's making the patch _vastly_ simpler, _and_ we don't have to
>> involve the networking folks here.
>
> Okay, will send a V2 with this approach.
>
>> We have a similar patch for the data_ready() function in nvmet_tcp(),
>> and that seemed to work, too.
>> Nit: we don't unset the 'NOSPACE' flag there. Can you check if that's
>> really required?
>> And, if it is, fixup nvmet_tcp() to unset it?
>> Or, if not, modify your patch to not clear it?
>
> I don't see why we would need to clear the NOSPACE flag in
> data_ready()? My understanding is that this flag is used when the send
> buffer is full.
>
> I would think the clear_bit() is necessary in write_space() since it
> would typically get done in something like sk_stream_write_space()?
> However, running some quick FIOs with the clear_bit() removed, things
> seem to work. Not sure if removing it has any further implications
> though...
>
I am not sure, either. Code analysis suggests that we don't need to
do that, but then we're the first ever to explore that area.
So I would think we don't need to worry (as nvmet-tcp doesn't do that,
either). Sounds like a question for LPC.
So let's drop the 'NOSPACE' flag handling to get the
partial records fixed, and address the NOSPACE issue separately.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@...e.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Powered by blists - more mailing lists