linux-kernel - Re: [PATCH 5/5] task_work: use TIF_NOTIFY

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c7da5280-f283-2c89-f6f2-be7d84c3675a@kernel.dk>
Date:   Fri, 16 Oct 2020 07:33:17 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, io-uring@...r.kernel.org,
        peterz@...radead.org, Roman Gershman <romger@...zon.com>
Subject: Re: [PATCH 5/5] task_work: use TIF_NOTIFY_SIGNAL if available

On 10/16/20 3:00 AM, Thomas Gleixner wrote:
> On Thu, Oct 15 2020 at 12:39, Jens Axboe wrote:
>> On 10/15/20 9:49 AM, Oleg Nesterov wrote:
>>> You can simply nack the patch which adds TIF_NOTIFY_SIGNAL to
>>> arch/xxx/include/asm/thread_info.h.
> 
> As if that is going to change anything...
> 
>> This seems to be the biggest area of contention right now. Just to
>> summarize, we have two options:
>>
>> 1) We leave the CONFIG_GENERIC_ENTRY requirement, which means that the
>>    rest of the cleanups otherwise enabled by this series will not be
>>    able to move forward until the very last arch is converted to the
>>    generic entry code.
>>
>> 2) We go back to NOT having the CONFIG_GENERIC_ENTRY requirement, and
>>    archs can easily opt-in to TIF_NOTIFY_SIGNAL independently of
>>    switching to the generic entry code.
>>
>> I understand Thomas's reasoning in wanting to push archs towards the
>> generic entry code, and I fully support that. However, it does seem like
>> the road paved by #1 is long and potentially neverending, which would
>> leave us with never being able to kill the various bits of code that we
>> otherwise would be able to.
>>
>> Thomas, I do agree with Oleg on this one, I think we can make quicker
>> progress on cleanups with option #2. This isn't really going to hinder
>> any arch conversion to the generic entry code, as arch patches would be
>> funeled through the arch trees anyway.
> 
> I completely understand the desire to remove the jobctl mess and it
> looks like a valuable cleanup on it's own.
> 
> But I fundamentaly disagree with the wording of #2:
> 
>     'and archs can easily opt-in ....'
> 
> Just doing it on an opt-in base is not any different from making it
> dependent on CONFIG_GENERIC_ENTRY. It's just painted differently and
> makes it easy for you to bring the performance improvement to the less
> than a handful architectures which actually care about io_uring.

It's a lot easier for me to add support for archs for TIF_NOTIFY_SIGNAL,
than it is to convert them to CONFIG_GENERIC ENTRY. And in fact I
already _did_ convert all archs, in a separate series. Is it perfect
yet? No. arm needs a bit of love, powerpc should be cleaned up once the
5.10 merge happens for that arch (dropping a bit), and s390 I need
someone to verify. But apart from that, it is already done.

> So if you change #2 to:
> 
>    Drop the CONFIG_GENERIC_ENTRY dependency, make _all_ architectures
>    use TIF_NOTIFY_SIGNAL and clean up the jobctl and whatever related
>    mess.
> 
> and actually act apon it, then I'm fine with that approach.

Already did that too!

https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work.arch

It's sitting on top of this series. So the work is already done.

> Anything else is just proliferating the existing mess and yet another
> promise of great improvements which never materialize.
> 
> Just to prove my point:
> 
> e91b48162332 ("task_work: teach task_work_add() to do signal_wake_up()")
> 
> added TWA_SIGNAL in June with the following in the changelog:
> 
>     TODO: once this patch is merged we need to change all current users
>     of task_work_add(notify = true) to use TWA_RESUME.

Totally agree the ball was dropped on this one. I did actually write a
patch, just never had time to get it out.
 
> This features first and let others deal with the mess we create mindset
> has to stop. I'm dead tired of it.

I totally agree, and we're on the same page. I think you'll find that in
the past I always carry through, the task_work notification was somewhat
of a rush due to a hang related to it. For this particular case, the
cleanups and arch additions are pretty much ready to go.

-- 
Jens Axboe