linux-kernel - Re: [PATCH RFC] kernel: decouple TASK_WORK TWA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8a98b921-c1c6-c1b7-e0ff-e2179badda55@kernel.dk>
Date:   Thu, 1 Oct 2020 11:17:44 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Thomas Gleixner <tglx@...utronix.de>,
        io-uring <io-uring@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH RFC] kernel: decouple TASK_WORK TWA_SIGNAL handling from
 signals

On 10/1/20 9:49 AM, Thomas Gleixner wrote:
>>> This is really a hack. TWA_SIGNAL is a misnomer with the new
>>> functionality and combined with the above
>>>
>>>          if (!ret && !notify)
>>>   		wake_up_process(tsk);
>>>
>>> there is not really a big difference between TWA_RESUME and TWA_SIGNAL
>>> anymore. Just the delivery mode and the syscall restart magic.
>>
>> Agree, maybe it'd make more sense to rename TWA_SIGNAL to TWA_RESTART or
>> something like that. The only user of this is io_uring, so it's not like
>> it's a lot of churn to do so.
> 
> I really hate that extra TIF flag just for this. We have way too many
> already and there is work in progress already to address that. I told
> other people already that new TIF flags are not going to happen unless
> the mess is cleaned up. There is work in progress to do so.

I'm open to alternatives, but it does seem like the best match for
something like this...

>>> This needs a lot more thoughts.
>>
>> Definitely, which is why I'm posting it as an RFC. It fixes a real
>> performance regression, and there's no reliable way to use TWA_RESUME
>> that I can tell.
> 
> It's not a performance regression simply because the stuff you had in
> the first place which had more performance was broken. We are not
> measuring broken vs. correct, really.
> 
> You are looking for a way to make stuff perform better and that's
> something totally different and does not need to be rushed. Especially
> rushing stuff into sensible areas like the entry code is not going to
> happen just because you screwed up your initial design.

Nobody is rushing anything - I noticed that I messed up the syscall
restart for task_work && signal, so I fixed it. I'm quite happy taking
my time getting this done the right way.

>> What kind of restart behavior do we need? Before this change, everytime
>> _TIF_SIGPENDING is set and we don't deliver a signal in the loop, we go
>> through the syscall restart code. After this change, we only do so at
>> the end. I'm assuming that's your objection?
> 
> No. That should work by some definition of work, but doing a restart
> while delivering a signal cannot work at all.

Right, this is what v2 fixes, and why I sent it out.

>> For _TIF_TASKWORK, we'll always want to restat the system call, if we
>> were currently doing one. For signals, only if we didn't deliver a
>> signal. So we'll want to retain the restart inside signal delivery?
> 
> No. This needs more thoughts about how restart handling is supposed to
> work in the bigger picture and I'm not going to look at new versions of
> this which are rushed out every half an hour unless there is a proper
> analysis of how all this should play together in a way which does not
> make an utter mess of everything.

Again, this is an RFC, I'm soliciting comments on how we can make this
work. I'd appreciate any hints and help in that regard of course.

Thanks,
-- 
Jens Axboe