Re: [PATCHSET 0/4] Add support for no-lock sockets

[Jens Axboe <axboe@kernel.dk>]

On 4/12/22 8:05 PM, Eric Dumazet wrote:
> On Tue, Apr 12, 2022 at 7:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 4/12/22 7:54 PM, Eric Dumazet wrote:
>>> On Tue, Apr 12, 2022 at 6:26 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 4/12/22 6:40 PM, Eric Dumazet wrote:
>>>>>
>>>>> On 4/12/22 13:26, Jens Axboe wrote:
>>>>>> Hi,
>>>>>>
>>>>>> If we accept a connection directly, eg without installing a file
>>>>>> descriptor for it, or if we use IORING_OP_SOCKET in direct mode, then
>>>>>> we have a socket for recv/send that we can fully serialize access to.
>>>>>>
>>>>>> With that in mind, we can feasibly skip locking on the socket for TCP
>>>>>> in that case. Some of the testing I've done has shown as much as 15%
>>>>>> of overhead in the lock_sock/release_sock part, with this change then
>>>>>> we see none.
>>>>>>
>>>>>> Comments welcome!
>>>>>>
>>>>> How BH handlers (including TCP timers) and io_uring are going to run
>>>>> safely ? Even if a tcp socket had one user, (private fd opened by a
>>>>> non multi-threaded program), we would still to use the spinlock.
>>>>
>>>> But we don't even hold the spinlock over lock_sock() and release_sock(),
>>>> just the mutex. And we do check for running eg the backlog on release,
>>>> which I believe is done safely and similarly in other places too.
>>>
>>> So lets say TCP stack receives a packet in BH handler... it proceeds
>>> using many tcp sock fields.
>>>
>>> Then io_uring wants to read/write stuff from another cpu, while BH
>>> handler(s) is(are) not done yet,
>>> and will happily read/change many of the same fields
>>
>> But how is that currently protected?
> 
> It is protected by current code.
> 
> What you wrote would break TCP stack quite badly.

No offense, but your explanations are severely lacking. By "current
code"? So what you're saying is that it's protected by how the code
currently works? From how that it currently is? Yeah, that surely
explains it.

> I suggest you setup/run a syzbot server/farm, then you will have a
> hundred reports quite easily.

Nowhere am I claiming this is currently perfect, and it should have had
an RFC on it. Was hoping for some constructive criticism on how to move
this forward, as high frequency TCP currently _sucks_ in the stack.
Instead I get useless replies, not very encouraging.

I've run this quite extensively on just basic send/receive over sockets,
so it's not like it hasn't been run at all. And it's been fine so far,
no ill effects observed. If we need to tighten down the locking, perhaps
a valid use would be to simply skip the mutex and retain the bh lock for
setting owner. As far as I can tell, should still be safe to skip on
release, except if we need to process the backlog. And it'd serialize
the owner setting with the BH, which seems to be your main objection in.
Mostly guessing here, based on the in-depth replies.

But it'd be nice if we could have a more constructive dialogue about
this, rather than the weird dismisiveness.

-- 
Jens Axboe