Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 5/5] task_work: use TIF_NOTIFY_SIGNAL if available
To:     Thomas Gleixner <tglx@linutronix.de>,
        Oleg Nesterov <oleg@redhat.com>
Cc:     linux-kernel@vger.kernel.org, io-uring@vger.kernel.org,
        peterz@infradead.org, Roman Gershman <romger@amazon.com>
References: <20201015131701.511523-1-axboe@kernel.dk>
 <20201015131701.511523-6-axboe@kernel.dk> <20201015154953.GM24156@redhat.com>
 <e17cd91e-97b2-1eae-964b-fc90f8f9ef31@kernel.dk>
 <87a6wmv93v.fsf@nanos.tec.linutronix.de>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <c7da5280-f283-2c89-f6f2-be7d84c3675a@kernel.dk>
Date:   Fri, 16 Oct 2020 07:33:17 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <87a6wmv93v.fsf@nanos.tec.linutronix.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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