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
