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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Jens Axboe <axboe@kernel.dk>, io-uring <io-uring@vger.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH RFC] kernel: decouple TASK_WORK TWA_SIGNAL handling from signals
In-Reply-To: <3eafe8ec-7d31-bd46-8641-2d26aca5420d@kernel.dk>
References: <0b5336a7-c975-a8f8-e988-e983e2340d99@kernel.dk> <875z7uezys.fsf@nanos.tec.linutronix.de> <3eafe8ec-7d31-bd46-8641-2d26aca5420d@kernel.dk>
Date:   Thu, 01 Oct 2020 17:49:05 +0200
Message-ID: <87362yeyku.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Thu, Oct 01 2020 at 09:26, Jens Axboe wrote:
> On 10/1/20 9:19 AM, Thomas Gleixner wrote:
>>>  	ret = task_work_add(tsk, cb, notify);
>>> -	if (!ret)
>>> +	if (!ret && !notify)
>> 
>> !notify assumes that TWA_RESUME == 0. Fun to debug if that ever changes.
>
> Agree, I'll make that
>
> 	if (!ret && notify != TWA_SIGNAL)
>
> instead, that's more sane.

It's not more sane. It's just more correct.

>> 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.

>> 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.

> 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.

> 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.

Thanks,

        tglx