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

[Jens Axboe <axboe@kernel.dk>]

On 4/12/22 8:32 PM, Eric Dumazet wrote:
> On Tue, Apr 12, 2022 at 7:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 4/12/22 8:19 PM, Eric Dumazet wrote:
>>> On Tue, Apr 12, 2022 at 7:12 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> 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.
>>>>
>>>>
>>>
>>> Sure. It would be nice that I have not received such a patch series
>>> the day I am sick.
>>
>> I'm sorry that you are sick - but if you are not in a state to reply,
>> then please just don't. It sets a bad example. It was sent to the list,
>> not to you personally.
> 
> I tried to be as constructive as possible, and Jakub pinged me about

Are you serious?! I don't think I've ever received less constructive
feedback in 20+ years of working on the kernel.

> this series,
> so I really thought Jakub was okay with it.
> 
> So I am a bit concerned.

I did show it to Jakub a week or so ago, probably that was why. But why
the concern?! It's just a patchseries proposed for discussion. Something
that happens every day.

>> Don't check email then, putting the blame on ME for posting a patchset
>> while you are sick is uncalled for and rude. If I had a crystal ball, I
>> would not be spending my time working on the kernel. You know what
>> would've been a better idea? Replying that you are sick and that you are
>> sorry for being an ass on the mailing list.
> 
> Wow.

Putting the blame on me for your emails, since I posted a patchset while
you're sick, is just rude.

-- 
Jens Axboe