Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 01.04.21 um 18:24 schrieb Linus Torvalds:
> On Thu, Apr 1, 2021 at 9:00 AM Stefan Metzmacher  wrote:
>>
>> I haven't tried it, but it seems gdb tries to use PTRACE_PEEKUSR
>> against the last thread tid listed under /proc//tasks/ in order to
>> get the architecture for the userspace application
> 
> Christ, what an odd hack. Why wouldn't it just do it on the initial
> thread you actually attached to?
> 
> Are you sure it's not simply because your test-case was to attach to
> the io_uring thread? Because the io_uring thread might as well be
> considered to be 64-bit.

  │   └─io_uring-cp,1396 Makefile file
  │   ├─{iou-mgr-1396},1397
  │   ├─{iou-wrk-1396},1398
  │   └─{iou-wrk-1396},1399

strace -ttT -o strace-uring-fail.txt gdb --pid 1396
(note strace -f would deadlock gdb with SIGSTOP)

The full file can be found here:
https://www.samba.org/~metze/strace-uring-fail.txt
(I guess there was a race and the workers 1398 and 1399 exited in between,
that's it using 1397):

18:46:35.429498 ptrace(PTRACE_PEEKUSER, 1397, 8*CS, [NULL]) = 0 <0.22>

>> so my naive assumption
>> would be that it wouldn't allow the detection of a 32-bit application
>> using a 64-bit kernel.
> 
> I'm not entirely convinced we want to care about a confused gdb
> implementation and somebody debugging a case that I don't believe
> happens in practice.
> 
> 32-bit user space is legacy. And legacy isn't io_uring. If somebody
> insists on doing odd things, they can live with the odd results.

Ok, I'd agree for 32-bit applications, but what about libraries?
E.g. distributions ship libraries like libsmbclient or nss modules
as 64-bit and 32-bit version, in order to support legacy applications
to run. Why shouldn't the 32-bit library builds not support io_uring?
(Note libsmbclient don't use io_uring yet, but I guess it will be in future).

Any ideas regarding similar problems for other architectures?

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Linus Torvalds]

On Thu, Apr 1, 2021 at 9:00 AM Stefan Metzmacher  wrote:
>
> I haven't tried it, but it seems gdb tries to use PTRACE_PEEKUSR
> against the last thread tid listed under /proc//tasks/ in order to
> get the architecture for the userspace application

Christ, what an odd hack. Why wouldn't it just do it on the initial
thread you actually attached to?

Are you sure it's not simply because your test-case was to attach to
the io_uring thread? Because the io_uring thread might as well be
considered to be 64-bit.

> so my naive assumption
> would be that it wouldn't allow the detection of a 32-bit application
> using a 64-bit kernel.

I'm not entirely convinced we want to care about a confused gdb
implementation and somebody debugging a case that I don't believe
happens in practice.

32-bit user space is legacy. And legacy isn't io_uring. If somebody
insists on doing odd things, they can live with the odd results.

  Linus

Re: [PATCH 0/6] Allow signals for IO threads

[Linus Torvalds]

On Thu, Apr 1, 2021 at 7:58 AM Stefan Metzmacher  wrote:
>
> >
> > Ok, the following makes gdb happy again:
> >
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -163,6 +163,8 @@ int copy_thread(unsigned long clone_flags, unsigned 
> > long sp, unsigned long arg,
> > /* Kernel thread ? */
> > if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
> > memset(childregs, 0, sizeof(struct pt_regs));
> > +   if (p->flags & PF_IO_WORKER)
> > +   childregs->cs = current_pt_regs()->cs;
> > kthread_frame_init(frame, sp, arg);
> > return 0;
> > }
>
> Would it be possible to fix this remaining problem before 5.12 final?

Please not that way.

But doing something like

childregs->cs = __USER_CS;
childregs->ss = __USER_DS;
childregs->ds = __USER_DS;
childregs->es = __USER_DS;

might make sense (just do it unconditionally, rather than making it
special to PF_IO_WORKER).

Does that make gdb happy too?

   Linus

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 01.04.21 um 17:39 schrieb Linus Torvalds:
> On Thu, Apr 1, 2021 at 7:58 AM Stefan Metzmacher  wrote:
>>
>>>
>>> Ok, the following makes gdb happy again:
>>>
>>> --- a/arch/x86/kernel/process.c
>>> +++ b/arch/x86/kernel/process.c
>>> @@ -163,6 +163,8 @@ int copy_thread(unsigned long clone_flags, unsigned 
>>> long sp, unsigned long arg,
>>> /* Kernel thread ? */
>>> if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
>>> memset(childregs, 0, sizeof(struct pt_regs));
>>> +   if (p->flags & PF_IO_WORKER)
>>> +   childregs->cs = current_pt_regs()->cs;
>>> kthread_frame_init(frame, sp, arg);
>>> return 0;
>>> }
>>
>> Would it be possible to fix this remaining problem before 5.12 final?
> 
> Please not that way.
> 
> But doing something like
> 
> childregs->cs = __USER_CS;
> childregs->ss = __USER_DS;
> childregs->ds = __USER_DS;
> childregs->es = __USER_DS;
> 
> might make sense (just do it unconditionally, rather than making it
> special to PF_IO_WORKER).
> 
> Does that make gdb happy too?

I haven't tried it, but it seems gdb tries to use PTRACE_PEEKUSR
against the last thread tid listed under /proc//tasks/ in order to
get the architecture for the userspace application, so my naive assumption
would be that it wouldn't allow the detection of a 32-bit application
using a 64-bit kernel.

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Hi Jens,

>> For help, type "help".
>> Type "apropos word" to search for commands related to "word".
>> Attaching to process 1320
>> [New LWP 1321]
>> [New LWP 1322]
>>
>> warning: Selected architecture i386:x86-64 is not compatible with reported 
>> target architecture i386
>>
>> warning: Architecture rejected target-supplied description
>> syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
>> 38  ../sysdeps/unix/sysv/linux/x86_64/syscall.S: No such file or 
>> directory.
>> (gdb)
> 
> Ok, the following makes gdb happy again:
> 
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -163,6 +163,8 @@ int copy_thread(unsigned long clone_flags, unsigned long 
> sp, unsigned long arg,
> /* Kernel thread ? */
> if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
> memset(childregs, 0, sizeof(struct pt_regs));
> +   if (p->flags & PF_IO_WORKER)
> +   childregs->cs = current_pt_regs()->cs;
> kthread_frame_init(frame, sp, arg);
> return 0;
> }
> 
> I'm wondering if we should decouple the PF_KTHREAD and PF_IO_WORKER cases 
> even more
> and keep as much of a userspace-like copy_thread as possible.

Would it be possible to fix this remaining problem before 5.12 final?
(I don't think my change would be the correct fix actually
and other architectures may have similar problems).

Thanks!
metze

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 7:46 PM, Stefan Metzmacher wrote:
> 
> Hi Jens,
> 
>> root@ub1704-166:~# LANG=C gdb --pid 1320
>> GNU gdb (Ubuntu 9.2-0ubuntu1~20.04) 9.2
>> Copyright (C) 2020 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later 
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.
>> Type "show copying" and "show warranty" for details.
>> This GDB was configured as "x86_64-linux-gnu".
>> Type "show configuration" for configuration details.
>> For bug reporting instructions, please see:
>> .
>> Find the GDB manual and other documentation resources online at:
>> .
>>
>> For help, type "help".
>> Type "apropos word" to search for commands related to "word".
>> Attaching to process 1320
>> [New LWP 1321]
>> [New LWP 1322]
>>
>> warning: Selected architecture i386:x86-64 is not compatible with reported 
>> target architecture i386
>>
>> warning: Architecture rejected target-supplied description
>> syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
>> 38  ../sysdeps/unix/sysv/linux/x86_64/syscall.S: No such file or 
>> directory.
>> (gdb)
> 
> Ok, the following makes gdb happy again:
> 
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -163,6 +163,8 @@ int copy_thread(unsigned long clone_flags, unsigned long 
> sp, unsigned long arg,
> /* Kernel thread ? */
> if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
> memset(childregs, 0, sizeof(struct pt_regs));
> +   if (p->flags & PF_IO_WORKER)
> +   childregs->cs = current_pt_regs()->cs;
> kthread_frame_init(frame, sp, arg);
> return 0;
> }

Confirmed, it stops complaining about the arch at that point.

> I'm wondering if we should decouple the PF_KTHREAD and PF_IO_WORKER
> cases even more and keep as much of a userspace-like copy_thread as
> possible.

Probably makes sense, the only thing they really share is the func+arg
setup. Hence PF_IO_WORKER threads likely just use the rest of the init,
where it doesn't conflict with the frame setup.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Hi Jens,

> root@ub1704-166:~# LANG=C gdb --pid 1320
> GNU gdb (Ubuntu 9.2-0ubuntu1~20.04) 9.2
> Copyright (C) 2020 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
> Type "show copying" and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> .
> Find the GDB manual and other documentation resources online at:
> .
> 
> For help, type "help".
> Type "apropos word" to search for commands related to "word".
> Attaching to process 1320
> [New LWP 1321]
> [New LWP 1322]
> 
> warning: Selected architecture i386:x86-64 is not compatible with reported 
> target architecture i386
> 
> warning: Architecture rejected target-supplied description
> syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
> 38  ../sysdeps/unix/sysv/linux/x86_64/syscall.S: No such file or 
> directory.
> (gdb)

Ok, the following makes gdb happy again:

--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -163,6 +163,8 @@ int copy_thread(unsigned long clone_flags, unsigned long 
sp, unsigned long arg,
/* Kernel thread ? */
if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
memset(childregs, 0, sizeof(struct pt_regs));
+   if (p->flags & PF_IO_WORKER)
+   childregs->cs = current_pt_regs()->cs;
kthread_frame_init(frame, sp, arg);
return 0;
}

I'm wondering if we should decouple the PF_KTHREAD and PF_IO_WORKER cases even 
more
and keep as much of a userspace-like copy_thread as possible.

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 9:11 AM, Stefan Metzmacher wrote:
> Am 26.03.21 um 16:10 schrieb Jens Axboe:
>> On 3/26/21 9:08 AM, Stefan Metzmacher wrote:
>>> Am 26.03.21 um 15:55 schrieb Jens Axboe:
 On 3/26/21 8:53 AM, Jens Axboe wrote:
> On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
>> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
>>> Am 26.03.21 um 15:38 schrieb Jens Axboe:
 On 3/26/21 7:59 AM, Jens Axboe wrote:
> On 3/26/21 7:54 AM, Jens Axboe wrote:
>>> The KILL after STOP deadlock still exists.
>>
>> In which tree? Sounds like you're still on the old one with that
>> incremental you sent, which wasn't complete.
>>
>>> Does io_wq_manager() exits without cleaning up on SIGKILL?
>>
>> No, it should kill up in all cases. I'll try your stop + kill, I just
>> tested both of them separately and didn't observe anything. I also 
>> ran
>> your io_uring-cp example (and found a bug in the example, fixed and
>> pushed), fwiw.
>
> I can reproduce this one! I'll take a closer look.

 OK, that one is actually pretty straight forward - we rely on cleaning
 up on exit, but for fatal cases, get_signal() will call do_exit() for 
 us
 and never return. So we might need a special case in there to deal with
 that, or some other way of ensuring that fatal signal gets processed
 correctly for IO threads.
>>>
>>> And if (fatal_signal_pending(current)) doesn't prevent get_signal() 
>>> from being called?
>>
>> Ah, we're still in the first get_signal() from SIGSTOP, correct?
>
> Yes exactly, we're waiting in there being stopped. So we either need to
> check to something ala:
>
> relock:
> + if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
> + return false;
>
> to catch it upfront and from the relock case, or add:
>
>   fatal:
> + if (current->flags & PF_IO_WORKER)
> + return false;
>
> to catch it in the fatal section.

 Can you try this? Not crazy about adding a special case, but I don't
 think there's any way around this one. And should be pretty cheap, as
 we're already pulling in ->flags right above anyway.

 diff --git a/kernel/signal.c b/kernel/signal.c
 index 5ad8566534e7..5b75fbe3d2d6 100644
 --- a/kernel/signal.c
 +++ b/kernel/signal.c
 @@ -2752,6 +2752,15 @@ bool get_signal(struct ksignal *ksig)
 */
current->flags |= PF_SIGNALED;
  
 +  /*
 +   * PF_IO_WORKER threads will catch and exit on fatal signals
 +   * themselves. They have cleanup that must be performed, so
 +   * we cannot call do_exit() on their behalf. coredumps also
 +   * do not apply to them.
 +   */
 +  if (current->flags & PF_IO_WORKER)
 +  return false;
 +
if (sig_kernel_coredump(signr)) {
if (print_fatal_signals)
print_fatal_signal(ksig->info.si_signo);

>>>
>>> I guess not before next week, but if it resolves the problem for you,
>>> I guess it would be good to get this into rc5.
>>
>> It does, I pushed out a new branch. I'll send out a v2 series in a bit.
> 
> Great, thanks!
> 
> Any chance to get the "cmdline" hiding included?

I'll take a look at your response there, haven't yet. Wanted to get this
one sorted first.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 26.03.21 um 16:10 schrieb Jens Axboe:
> On 3/26/21 9:08 AM, Stefan Metzmacher wrote:
>> Am 26.03.21 um 15:55 schrieb Jens Axboe:
>>> On 3/26/21 8:53 AM, Jens Axboe wrote:
 On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
>> Am 26.03.21 um 15:38 schrieb Jens Axboe:
>>> On 3/26/21 7:59 AM, Jens Axboe wrote:
 On 3/26/21 7:54 AM, Jens Axboe wrote:
>> The KILL after STOP deadlock still exists.
>
> In which tree? Sounds like you're still on the old one with that
> incremental you sent, which wasn't complete.
>
>> Does io_wq_manager() exits without cleaning up on SIGKILL?
>
> No, it should kill up in all cases. I'll try your stop + kill, I just
> tested both of them separately and didn't observe anything. I also ran
> your io_uring-cp example (and found a bug in the example, fixed and
> pushed), fwiw.

 I can reproduce this one! I'll take a closer look.
>>>
>>> OK, that one is actually pretty straight forward - we rely on cleaning
>>> up on exit, but for fatal cases, get_signal() will call do_exit() for us
>>> and never return. So we might need a special case in there to deal with
>>> that, or some other way of ensuring that fatal signal gets processed
>>> correctly for IO threads.
>>
>> And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
>> being called?
>
> Ah, we're still in the first get_signal() from SIGSTOP, correct?

 Yes exactly, we're waiting in there being stopped. So we either need to
 check to something ala:

 relock:
 +  if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
 +  return false;

 to catch it upfront and from the relock case, or add:

fatal:
 +  if (current->flags & PF_IO_WORKER)
 +  return false;

 to catch it in the fatal section.
>>>
>>> Can you try this? Not crazy about adding a special case, but I don't
>>> think there's any way around this one. And should be pretty cheap, as
>>> we're already pulling in ->flags right above anyway.
>>>
>>> diff --git a/kernel/signal.c b/kernel/signal.c
>>> index 5ad8566534e7..5b75fbe3d2d6 100644
>>> --- a/kernel/signal.c
>>> +++ b/kernel/signal.c
>>> @@ -2752,6 +2752,15 @@ bool get_signal(struct ksignal *ksig)
>>>  */
>>> current->flags |= PF_SIGNALED;
>>>  
>>> +   /*
>>> +* PF_IO_WORKER threads will catch and exit on fatal signals
>>> +* themselves. They have cleanup that must be performed, so
>>> +* we cannot call do_exit() on their behalf. coredumps also
>>> +* do not apply to them.
>>> +*/
>>> +   if (current->flags & PF_IO_WORKER)
>>> +   return false;
>>> +
>>> if (sig_kernel_coredump(signr)) {
>>> if (print_fatal_signals)
>>> print_fatal_signal(ksig->info.si_signo);
>>>
>>
>> I guess not before next week, but if it resolves the problem for you,
>> I guess it would be good to get this into rc5.
> 
> It does, I pushed out a new branch. I'll send out a v2 series in a bit.

Great, thanks!

Any chance to get the "cmdline" hiding included?

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 9:08 AM, Stefan Metzmacher wrote:
> Am 26.03.21 um 15:55 schrieb Jens Axboe:
>> On 3/26/21 8:53 AM, Jens Axboe wrote:
>>> On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
 Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
> Am 26.03.21 um 15:38 schrieb Jens Axboe:
>> On 3/26/21 7:59 AM, Jens Axboe wrote:
>>> On 3/26/21 7:54 AM, Jens Axboe wrote:
> The KILL after STOP deadlock still exists.

 In which tree? Sounds like you're still on the old one with that
 incremental you sent, which wasn't complete.

> Does io_wq_manager() exits without cleaning up on SIGKILL?

 No, it should kill up in all cases. I'll try your stop + kill, I just
 tested both of them separately and didn't observe anything. I also ran
 your io_uring-cp example (and found a bug in the example, fixed and
 pushed), fwiw.
>>>
>>> I can reproduce this one! I'll take a closer look.
>>
>> OK, that one is actually pretty straight forward - we rely on cleaning
>> up on exit, but for fatal cases, get_signal() will call do_exit() for us
>> and never return. So we might need a special case in there to deal with
>> that, or some other way of ensuring that fatal signal gets processed
>> correctly for IO threads.
>
> And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
> being called?

 Ah, we're still in the first get_signal() from SIGSTOP, correct?
>>>
>>> Yes exactly, we're waiting in there being stopped. So we either need to
>>> check to something ala:
>>>
>>> relock:
>>> +   if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
>>> +   return false;
>>>
>>> to catch it upfront and from the relock case, or add:
>>>
>>> fatal:
>>> +   if (current->flags & PF_IO_WORKER)
>>> +   return false;
>>>
>>> to catch it in the fatal section.
>>
>> Can you try this? Not crazy about adding a special case, but I don't
>> think there's any way around this one. And should be pretty cheap, as
>> we're already pulling in ->flags right above anyway.
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index 5ad8566534e7..5b75fbe3d2d6 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2752,6 +2752,15 @@ bool get_signal(struct ksignal *ksig)
>>   */
>>  current->flags |= PF_SIGNALED;
>>  
>> +/*
>> + * PF_IO_WORKER threads will catch and exit on fatal signals
>> + * themselves. They have cleanup that must be performed, so
>> + * we cannot call do_exit() on their behalf. coredumps also
>> + * do not apply to them.
>> + */
>> +if (current->flags & PF_IO_WORKER)
>> +return false;
>> +
>>  if (sig_kernel_coredump(signr)) {
>>  if (print_fatal_signals)
>>  print_fatal_signal(ksig->info.si_signo);
>>
> 
> I guess not before next week, but if it resolves the problem for you,
> I guess it would be good to get this into rc5.

It does, I pushed out a new branch. I'll send out a v2 series in a bit.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 9:04 AM, Stefan Metzmacher wrote:
> 
> Am 26.03.21 um 15:53 schrieb Jens Axboe:
>> On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
>>> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
 Am 26.03.21 um 15:38 schrieb Jens Axboe:
> On 3/26/21 7:59 AM, Jens Axboe wrote:
>> On 3/26/21 7:54 AM, Jens Axboe wrote:
 The KILL after STOP deadlock still exists.
>>>
>>> In which tree? Sounds like you're still on the old one with that
>>> incremental you sent, which wasn't complete.
>>>
 Does io_wq_manager() exits without cleaning up on SIGKILL?
>>>
>>> No, it should kill up in all cases. I'll try your stop + kill, I just
>>> tested both of them separately and didn't observe anything. I also ran
>>> your io_uring-cp example (and found a bug in the example, fixed and
>>> pushed), fwiw.
>>
>> I can reproduce this one! I'll take a closer look.
>
> OK, that one is actually pretty straight forward - we rely on cleaning
> up on exit, but for fatal cases, get_signal() will call do_exit() for us
> and never return. So we might need a special case in there to deal with
> that, or some other way of ensuring that fatal signal gets processed
> correctly for IO threads.

 And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
 being called?
>>>
>>> Ah, we're still in the first get_signal() from SIGSTOP, correct?
>>
>> Yes exactly, we're waiting in there being stopped. So we either need to
>> check to something ala:
>>
>> relock:
>> +if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
>> +return false;
>>
>> to catch it upfront and from the relock case, or add:
>>
>>  fatal:
>> +if (current->flags & PF_IO_WORKER)
>> +return false;
>>
>> to catch it in the fatal section.
>>
> 
> Or something like io_uring_files_cancel()
> 
> Maybe change current->pf_io_worker with a generic current->io_thread
> structure which, has exit hooks, as well as
> io_wq_worker_sleeping() and io_wq_worker_running().
> 
> Maybe create_io_thread would take such an structure
> as argument instead of a single function pointer.
> 
> struct io_thread_description {
>   const char *name;
>   int (*thread_fn)(struct io_thread_description *);
>   void (*sleeping_fn)((struct io_thread_description *);
>   void (*running_fn)((struct io_thread_description *);
>   void (*exit_fn)((struct io_thread_description *);
> };
> 
> And then
> struct io_wq_manager {
>   struct io_thread_description description;
>   ... manager specific stuff...
> };

I did consider something like that, but seems a bit over-engineered
just for catching this case. And any kind of logic for PF_EXITING
ends up being a bit tricky for cancelations.

We can look into doing that for 5.13 potentially.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 26.03.21 um 15:55 schrieb Jens Axboe:
> On 3/26/21 8:53 AM, Jens Axboe wrote:
>> On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
>>> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
 Am 26.03.21 um 15:38 schrieb Jens Axboe:
> On 3/26/21 7:59 AM, Jens Axboe wrote:
>> On 3/26/21 7:54 AM, Jens Axboe wrote:
 The KILL after STOP deadlock still exists.
>>>
>>> In which tree? Sounds like you're still on the old one with that
>>> incremental you sent, which wasn't complete.
>>>
 Does io_wq_manager() exits without cleaning up on SIGKILL?
>>>
>>> No, it should kill up in all cases. I'll try your stop + kill, I just
>>> tested both of them separately and didn't observe anything. I also ran
>>> your io_uring-cp example (and found a bug in the example, fixed and
>>> pushed), fwiw.
>>
>> I can reproduce this one! I'll take a closer look.
>
> OK, that one is actually pretty straight forward - we rely on cleaning
> up on exit, but for fatal cases, get_signal() will call do_exit() for us
> and never return. So we might need a special case in there to deal with
> that, or some other way of ensuring that fatal signal gets processed
> correctly for IO threads.

 And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
 being called?
>>>
>>> Ah, we're still in the first get_signal() from SIGSTOP, correct?
>>
>> Yes exactly, we're waiting in there being stopped. So we either need to
>> check to something ala:
>>
>> relock:
>> +if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
>> +return false;
>>
>> to catch it upfront and from the relock case, or add:
>>
>>  fatal:
>> +if (current->flags & PF_IO_WORKER)
>> +return false;
>>
>> to catch it in the fatal section.
> 
> Can you try this? Not crazy about adding a special case, but I don't
> think there's any way around this one. And should be pretty cheap, as
> we're already pulling in ->flags right above anyway.
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5ad8566534e7..5b75fbe3d2d6 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2752,6 +2752,15 @@ bool get_signal(struct ksignal *ksig)
>*/
>   current->flags |= PF_SIGNALED;
>  
> + /*
> +  * PF_IO_WORKER threads will catch and exit on fatal signals
> +  * themselves. They have cleanup that must be performed, so
> +  * we cannot call do_exit() on their behalf. coredumps also
> +  * do not apply to them.
> +  */
> + if (current->flags & PF_IO_WORKER)
> + return false;
> +
>   if (sig_kernel_coredump(signr)) {
>   if (print_fatal_signals)
>   print_fatal_signal(ksig->info.si_signo);
> 

I guess not before next week, but if it resolves the problem for you,
I guess it would be good to get this into rc5.

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 26.03.21 um 15:53 schrieb Jens Axboe:
> On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
>> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
>>> Am 26.03.21 um 15:38 schrieb Jens Axboe:
 On 3/26/21 7:59 AM, Jens Axboe wrote:
> On 3/26/21 7:54 AM, Jens Axboe wrote:
>>> The KILL after STOP deadlock still exists.
>>
>> In which tree? Sounds like you're still on the old one with that
>> incremental you sent, which wasn't complete.
>>
>>> Does io_wq_manager() exits without cleaning up on SIGKILL?
>>
>> No, it should kill up in all cases. I'll try your stop + kill, I just
>> tested both of them separately and didn't observe anything. I also ran
>> your io_uring-cp example (and found a bug in the example, fixed and
>> pushed), fwiw.
>
> I can reproduce this one! I'll take a closer look.

 OK, that one is actually pretty straight forward - we rely on cleaning
 up on exit, but for fatal cases, get_signal() will call do_exit() for us
 and never return. So we might need a special case in there to deal with
 that, or some other way of ensuring that fatal signal gets processed
 correctly for IO threads.
>>>
>>> And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
>>> being called?
>>
>> Ah, we're still in the first get_signal() from SIGSTOP, correct?
> 
> Yes exactly, we're waiting in there being stopped. So we either need to
> check to something ala:
> 
> relock:
> + if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
> + return false;
> 
> to catch it upfront and from the relock case, or add:
> 
>   fatal:
> + if (current->flags & PF_IO_WORKER)
> + return false;
> 
> to catch it in the fatal section.
> 

Or something like io_uring_files_cancel()

Maybe change current->pf_io_worker with a generic current->io_thread
structure which, has exit hooks, as well as
io_wq_worker_sleeping() and io_wq_worker_running().

Maybe create_io_thread would take such an structure
as argument instead of a single function pointer.

struct io_thread_description {
const char *name;
int (*thread_fn)(struct io_thread_description *);
void (*sleeping_fn)((struct io_thread_description *);
void (*running_fn)((struct io_thread_description *);
void (*exit_fn)((struct io_thread_description *);
};

And then
struct io_wq_manager {
struct io_thread_description description;
... manager specific stuff...
};

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 8:53 AM, Jens Axboe wrote:
> On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
>> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
>>> Am 26.03.21 um 15:38 schrieb Jens Axboe:
 On 3/26/21 7:59 AM, Jens Axboe wrote:
> On 3/26/21 7:54 AM, Jens Axboe wrote:
>>> The KILL after STOP deadlock still exists.
>>
>> In which tree? Sounds like you're still on the old one with that
>> incremental you sent, which wasn't complete.
>>
>>> Does io_wq_manager() exits without cleaning up on SIGKILL?
>>
>> No, it should kill up in all cases. I'll try your stop + kill, I just
>> tested both of them separately and didn't observe anything. I also ran
>> your io_uring-cp example (and found a bug in the example, fixed and
>> pushed), fwiw.
>
> I can reproduce this one! I'll take a closer look.

 OK, that one is actually pretty straight forward - we rely on cleaning
 up on exit, but for fatal cases, get_signal() will call do_exit() for us
 and never return. So we might need a special case in there to deal with
 that, or some other way of ensuring that fatal signal gets processed
 correctly for IO threads.
>>>
>>> And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
>>> being called?
>>
>> Ah, we're still in the first get_signal() from SIGSTOP, correct?
> 
> Yes exactly, we're waiting in there being stopped. So we either need to
> check to something ala:
> 
> relock:
> + if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
> + return false;
> 
> to catch it upfront and from the relock case, or add:
> 
>   fatal:
> + if (current->flags & PF_IO_WORKER)
> + return false;
> 
> to catch it in the fatal section.

Can you try this? Not crazy about adding a special case, but I don't
think there's any way around this one. And should be pretty cheap, as
we're already pulling in ->flags right above anyway.

diff --git a/kernel/signal.c b/kernel/signal.c
index 5ad8566534e7..5b75fbe3d2d6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2752,6 +2752,15 @@ bool get_signal(struct ksignal *ksig)
 */
current->flags |= PF_SIGNALED;
 
+   /*
+* PF_IO_WORKER threads will catch and exit on fatal signals
+* themselves. They have cleanup that must be performed, so
+* we cannot call do_exit() on their behalf. coredumps also
+* do not apply to them.
+*/
+   if (current->flags & PF_IO_WORKER)
+   return false;
+
if (sig_kernel_coredump(signr)) {
if (print_fatal_signals)
print_fatal_signal(ksig->info.si_signo);

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 8:45 AM, Stefan Metzmacher wrote:
> Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
>> Am 26.03.21 um 15:38 schrieb Jens Axboe:
>>> On 3/26/21 7:59 AM, Jens Axboe wrote:
 On 3/26/21 7:54 AM, Jens Axboe wrote:
>> The KILL after STOP deadlock still exists.
>
> In which tree? Sounds like you're still on the old one with that
> incremental you sent, which wasn't complete.
>
>> Does io_wq_manager() exits without cleaning up on SIGKILL?
>
> No, it should kill up in all cases. I'll try your stop + kill, I just
> tested both of them separately and didn't observe anything. I also ran
> your io_uring-cp example (and found a bug in the example, fixed and
> pushed), fwiw.

 I can reproduce this one! I'll take a closer look.
>>>
>>> OK, that one is actually pretty straight forward - we rely on cleaning
>>> up on exit, but for fatal cases, get_signal() will call do_exit() for us
>>> and never return. So we might need a special case in there to deal with
>>> that, or some other way of ensuring that fatal signal gets processed
>>> correctly for IO threads.
>>
>> And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
>> being called?
> 
> Ah, we're still in the first get_signal() from SIGSTOP, correct?

Yes exactly, we're waiting in there being stopped. So we either need to
check to something ala:

relock:
+   if (current->flags & PF_IO_WORKER && fatal_signal_pending(current))
+   return false;

to catch it upfront and from the relock case, or add:

fatal:
+   if (current->flags & PF_IO_WORKER)
+   return false;

to catch it in the fatal section.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 8:43 AM, Stefan Metzmacher wrote:
> Am 26.03.21 um 15:38 schrieb Jens Axboe:
>> On 3/26/21 7:59 AM, Jens Axboe wrote:
>>> On 3/26/21 7:54 AM, Jens Axboe wrote:
> The KILL after STOP deadlock still exists.

 In which tree? Sounds like you're still on the old one with that
 incremental you sent, which wasn't complete.

> Does io_wq_manager() exits without cleaning up on SIGKILL?

 No, it should kill up in all cases. I'll try your stop + kill, I just
 tested both of them separately and didn't observe anything. I also ran
 your io_uring-cp example (and found a bug in the example, fixed and
 pushed), fwiw.
>>>
>>> I can reproduce this one! I'll take a closer look.
>>
>> OK, that one is actually pretty straight forward - we rely on cleaning
>> up on exit, but for fatal cases, get_signal() will call do_exit() for us
>> and never return. So we might need a special case in there to deal with
>> that, or some other way of ensuring that fatal signal gets processed
>> correctly for IO threads.
> 
> And if (fatal_signal_pending(current)) doesn't prevent get_signal()
> from being called?

Usually yes, but this case is first doing SIGSTOP, so we're waiting in
get_signal() -> do_signal_stop() when the SIGKILL arrives. Hence there's
no way to catch it in the worker themselves.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 26.03.21 um 15:43 schrieb Stefan Metzmacher:
> Am 26.03.21 um 15:38 schrieb Jens Axboe:
>> On 3/26/21 7:59 AM, Jens Axboe wrote:
>>> On 3/26/21 7:54 AM, Jens Axboe wrote:
> The KILL after STOP deadlock still exists.

 In which tree? Sounds like you're still on the old one with that
 incremental you sent, which wasn't complete.

> Does io_wq_manager() exits without cleaning up on SIGKILL?

 No, it should kill up in all cases. I'll try your stop + kill, I just
 tested both of them separately and didn't observe anything. I also ran
 your io_uring-cp example (and found a bug in the example, fixed and
 pushed), fwiw.
>>>
>>> I can reproduce this one! I'll take a closer look.
>>
>> OK, that one is actually pretty straight forward - we rely on cleaning
>> up on exit, but for fatal cases, get_signal() will call do_exit() for us
>> and never return. So we might need a special case in there to deal with
>> that, or some other way of ensuring that fatal signal gets processed
>> correctly for IO threads.
> 
> And if (fatal_signal_pending(current)) doesn't prevent get_signal() from 
> being called?

Ah, we're still in the first get_signal() from SIGSTOP, correct?

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 26.03.21 um 15:38 schrieb Jens Axboe:
> On 3/26/21 7:59 AM, Jens Axboe wrote:
>> On 3/26/21 7:54 AM, Jens Axboe wrote:
 The KILL after STOP deadlock still exists.
>>>
>>> In which tree? Sounds like you're still on the old one with that
>>> incremental you sent, which wasn't complete.
>>>
 Does io_wq_manager() exits without cleaning up on SIGKILL?
>>>
>>> No, it should kill up in all cases. I'll try your stop + kill, I just
>>> tested both of them separately and didn't observe anything. I also ran
>>> your io_uring-cp example (and found a bug in the example, fixed and
>>> pushed), fwiw.
>>
>> I can reproduce this one! I'll take a closer look.
> 
> OK, that one is actually pretty straight forward - we rely on cleaning
> up on exit, but for fatal cases, get_signal() will call do_exit() for us
> and never return. So we might need a special case in there to deal with
> that, or some other way of ensuring that fatal signal gets processed
> correctly for IO threads.

And if (fatal_signal_pending(current)) doesn't prevent get_signal() from being 
called?

metze

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 7:59 AM, Jens Axboe wrote:
> On 3/26/21 7:54 AM, Jens Axboe wrote:
>>> The KILL after STOP deadlock still exists.
>>
>> In which tree? Sounds like you're still on the old one with that
>> incremental you sent, which wasn't complete.
>>
>>> Does io_wq_manager() exits without cleaning up on SIGKILL?
>>
>> No, it should kill up in all cases. I'll try your stop + kill, I just
>> tested both of them separately and didn't observe anything. I also ran
>> your io_uring-cp example (and found a bug in the example, fixed and
>> pushed), fwiw.
> 
> I can reproduce this one! I'll take a closer look.

OK, that one is actually pretty straight forward - we rely on cleaning
up on exit, but for fatal cases, get_signal() will call do_exit() for us
and never return. So we might need a special case in there to deal with
that, or some other way of ensuring that fatal signal gets processed
correctly for IO threads.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 7:54 AM, Jens Axboe wrote:
>> The KILL after STOP deadlock still exists.
> 
> In which tree? Sounds like you're still on the old one with that
> incremental you sent, which wasn't complete.
> 
>> Does io_wq_manager() exits without cleaning up on SIGKILL?
> 
> No, it should kill up in all cases. I'll try your stop + kill, I just
> tested both of them separately and didn't observe anything. I also ran
> your io_uring-cp example (and found a bug in the example, fixed and
> pushed), fwiw.

I can reproduce this one! I'll take a closer look.

-- 
Jens Axboe

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 7:31 AM, Stefan Metzmacher wrote:
> Am 26.03.21 um 13:56 schrieb Jens Axboe:
>> On 3/26/21 5:48 AM, Stefan Metzmacher wrote:
>>>
>>> Am 26.03.21 um 01:39 schrieb Jens Axboe:
 Hi,

 As discussed in a previous thread today, the seemingly much saner approach
 is just to allow signals (including SIGSTOP) for the PF_IO_WORKER IO
 threads. If we just have the threads call get_signal() for
 signal_pending(), then everything just falls out naturally with how
 we receive and handle signals.

 Patch 1 adds support for checking and calling get_signal() from the
 regular IO workers, the manager, and the SQPOLL thread. Patch 2 unblocks
 SIGSTOP from the default IO thread blocked mask, and the rest just revert
 special cases that were put in place for PF_IO_WORKER threads.

 With this done, only two special cases remain for PF_IO_WORKER, and they
 aren't related to signals so not part of this patchset. But both of them
 can go away as well now that we have "real" threads as IO workers, and
 then we'll have zero special cases for PF_IO_WORKER.

 This passes the usual regression testing, my other usual 24h run has been
 kicked off. But I wanted to send this out early.

 Thanks to Linus for the suggestion. As with most other good ideas, it's
 obvious once you hear it. The fact that we end up with _zero_ special
 cases with this is a clear sign that this is the right way to do it
 indeed. The fact that this series is 2/3rds revert further drives that
 point home. Also thanks to Eric for diligent review on the signal side
 of things for the past changes (and hopefully ditto on this series :-))
>>>
>>> Ok, I'm testing a8ff6a3b20bd16d071ef66824ae4428529d114f9 from
>>> your io_uring-5.12 branch.
>>>
>>> And using this patch:
>>> diff --git a/examples/io_uring-cp.c b/examples/io_uring-cp.c
>>> index cc7a227a5ec7..6e26a4214015 100644
>>> --- a/examples/io_uring-cp.c
>>> +++ b/examples/io_uring-cp.c
>>> @@ -116,13 +116,16 @@ static void queue_write(struct io_uring *ring, struct 
>>> io_data *data)
>>> io_uring_submit(ring);
>>>  }
>>>
>>> -static int copy_file(struct io_uring *ring, off_t insize)
>>> +static int copy_file(struct io_uring *ring, off_t _insize)
>>>  {
>>> +   off_t insize = _insize;
>>> unsigned long reads, writes;
>>> struct io_uring_cqe *cqe;
>>> off_t write_left, offset;
>>> int ret;
>>>
>>> +again:
>>> +   insize = _insize;
>>> write_left = insize;
>>> writes = reads = offset = 0;
>>>
>>> @@ -221,6 +224,12 @@ static int copy_file(struct io_uring *ring, off_t 
>>> insize)
>>> }
>>> }
>>>
>>> +   {
>>> +   struct timespec ts = { .tv_nsec = 99, };
>>> +   nanosleep(, NULL);
>>> +   goto again;
>>> +   }
>>> +
>>> return 0;
>>>  }
>>>
>>> Running ./io_uring-cp ~/linux-image-5.12.0-rc2+-dbg_5.12.0-rc2+-5_amd64.deb 
>>> file
>>> What I see is this:
>>>
>>> kill -SIGSTOP to any thread I used a worker with pid 2061 here, results in
>>>
>>> root@ub1704-166:~# head /proc/2061/status
>>> Name:   iou-wrk-2041
>>> Umask:  0022
>>> State:  R (running)
>>> Tgid:   2041
>>> Ngid:   0
>>> Pid:2061
>>> PPid:   1857
>>> TracerPid:  0
>>> Uid:0   0   0   0
>>> Gid:0   0   0   0
>>> root@ub1704-166:~# head /proc/2041/status
>>> Name:   io_uring-cp
>>> Umask:  0022
>>> State:  T (stopped)
>>> Tgid:   2041
>>> Ngid:   0
>>> Pid:2041
>>> PPid:   1857
>>> TracerPid:  0
>>> Uid:0   0   0   0
>>> Gid:0   0   0   0
>>> root@ub1704-166:~# head /proc/2042/status
>>> Name:   iou-mgr-2041
>>> Umask:  0022
>>> State:  T (stopped)
>>> Tgid:   2041
>>> Ngid:   0
>>> Pid:2042
>>> PPid:   1857
>>> TracerPid:  0
>>> Uid:0   0   0   0
>>> Gid:0   0   0   0
>>>
>>> So userspace and iou-mgr-2041 stop, but the workers don't.
>>> 49 workers burn cpu as much as possible.
>>>
>>> kill -KILL 2061
>>> results in this:
>>> - all workers are gone
>>> - iou-mgr-2041 is gone
>>> - io_uring-cp waits in status D forever
>>>
>>> root@ub1704-166:~# head /proc/2041/status
>>> Name:   io_uring-cp
>>> Umask:  0022
>>> State:  D (disk sleep)
>>> Tgid:   2041
>>> Ngid:   0
>>> Pid:2041
>>> PPid:   1857
>>> TracerPid:  0
>>> Uid:0   0   0   0
>>> Gid:0   0   0   0
>>> root@ub1704-166:~# cat /proc/2041/stack
>>> [<0>] io_wq_destroy_manager+0x36/0xa0
>>> [<0>] io_wq_put_and_exit+0x2b/0x40
>>> [<0>] io_uring_clean_tctx+0xc5/0x110
>>> [<0>] __io_uring_files_cancel+0x336/0x4e0
>>> [<0>] do_exit+0x16b/0x13b0
>>> [<0>] do_group_exit+0x8b/0x140
>>> [<0>] get_signal+0x219/0xc90
>>> [<0>] arch_do_signal_or_restart+0x1eb/0xeb0
>>> [<0>] exit_to_user_mode_prepare+0x115/0x1a0
>>> [<0>] syscall_exit_to_user_mode+0x27/0x50
>>> [<0>] do_syscall_64+0x45/0x90
>>> [<0>] 

Re: [PATCH 0/6] Allow signals for IO threads

[Stefan Metzmacher]

Am 26.03.21 um 13:56 schrieb Jens Axboe:
> On 3/26/21 5:48 AM, Stefan Metzmacher wrote:
>>
>> Am 26.03.21 um 01:39 schrieb Jens Axboe:
>>> Hi,
>>>
>>> As discussed in a previous thread today, the seemingly much saner approach
>>> is just to allow signals (including SIGSTOP) for the PF_IO_WORKER IO
>>> threads. If we just have the threads call get_signal() for
>>> signal_pending(), then everything just falls out naturally with how
>>> we receive and handle signals.
>>>
>>> Patch 1 adds support for checking and calling get_signal() from the
>>> regular IO workers, the manager, and the SQPOLL thread. Patch 2 unblocks
>>> SIGSTOP from the default IO thread blocked mask, and the rest just revert
>>> special cases that were put in place for PF_IO_WORKER threads.
>>>
>>> With this done, only two special cases remain for PF_IO_WORKER, and they
>>> aren't related to signals so not part of this patchset. But both of them
>>> can go away as well now that we have "real" threads as IO workers, and
>>> then we'll have zero special cases for PF_IO_WORKER.
>>>
>>> This passes the usual regression testing, my other usual 24h run has been
>>> kicked off. But I wanted to send this out early.
>>>
>>> Thanks to Linus for the suggestion. As with most other good ideas, it's
>>> obvious once you hear it. The fact that we end up with _zero_ special
>>> cases with this is a clear sign that this is the right way to do it
>>> indeed. The fact that this series is 2/3rds revert further drives that
>>> point home. Also thanks to Eric for diligent review on the signal side
>>> of things for the past changes (and hopefully ditto on this series :-))
>>
>> Ok, I'm testing a8ff6a3b20bd16d071ef66824ae4428529d114f9 from
>> your io_uring-5.12 branch.
>>
>> And using this patch:
>> diff --git a/examples/io_uring-cp.c b/examples/io_uring-cp.c
>> index cc7a227a5ec7..6e26a4214015 100644
>> --- a/examples/io_uring-cp.c
>> +++ b/examples/io_uring-cp.c
>> @@ -116,13 +116,16 @@ static void queue_write(struct io_uring *ring, struct 
>> io_data *data)
>> io_uring_submit(ring);
>>  }
>>
>> -static int copy_file(struct io_uring *ring, off_t insize)
>> +static int copy_file(struct io_uring *ring, off_t _insize)
>>  {
>> +   off_t insize = _insize;
>> unsigned long reads, writes;
>> struct io_uring_cqe *cqe;
>> off_t write_left, offset;
>> int ret;
>>
>> +again:
>> +   insize = _insize;
>> write_left = insize;
>> writes = reads = offset = 0;
>>
>> @@ -221,6 +224,12 @@ static int copy_file(struct io_uring *ring, off_t 
>> insize)
>> }
>> }
>>
>> +   {
>> +   struct timespec ts = { .tv_nsec = 99, };
>> +   nanosleep(, NULL);
>> +   goto again;
>> +   }
>> +
>> return 0;
>>  }
>>
>> Running ./io_uring-cp ~/linux-image-5.12.0-rc2+-dbg_5.12.0-rc2+-5_amd64.deb 
>> file
>> What I see is this:
>>
>> kill -SIGSTOP to any thread I used a worker with pid 2061 here, results in
>>
>> root@ub1704-166:~# head /proc/2061/status
>> Name:   iou-wrk-2041
>> Umask:  0022
>> State:  R (running)
>> Tgid:   2041
>> Ngid:   0
>> Pid:2061
>> PPid:   1857
>> TracerPid:  0
>> Uid:0   0   0   0
>> Gid:0   0   0   0
>> root@ub1704-166:~# head /proc/2041/status
>> Name:   io_uring-cp
>> Umask:  0022
>> State:  T (stopped)
>> Tgid:   2041
>> Ngid:   0
>> Pid:2041
>> PPid:   1857
>> TracerPid:  0
>> Uid:0   0   0   0
>> Gid:0   0   0   0
>> root@ub1704-166:~# head /proc/2042/status
>> Name:   iou-mgr-2041
>> Umask:  0022
>> State:  T (stopped)
>> Tgid:   2041
>> Ngid:   0
>> Pid:2042
>> PPid:   1857
>> TracerPid:  0
>> Uid:0   0   0   0
>> Gid:0   0   0   0
>>
>> So userspace and iou-mgr-2041 stop, but the workers don't.
>> 49 workers burn cpu as much as possible.
>>
>> kill -KILL 2061
>> results in this:
>> - all workers are gone
>> - iou-mgr-2041 is gone
>> - io_uring-cp waits in status D forever
>>
>> root@ub1704-166:~# head /proc/2041/status
>> Name:   io_uring-cp
>> Umask:  0022
>> State:  D (disk sleep)
>> Tgid:   2041
>> Ngid:   0
>> Pid:2041
>> PPid:   1857
>> TracerPid:  0
>> Uid:0   0   0   0
>> Gid:0   0   0   0
>> root@ub1704-166:~# cat /proc/2041/stack
>> [<0>] io_wq_destroy_manager+0x36/0xa0
>> [<0>] io_wq_put_and_exit+0x2b/0x40
>> [<0>] io_uring_clean_tctx+0xc5/0x110
>> [<0>] __io_uring_files_cancel+0x336/0x4e0
>> [<0>] do_exit+0x16b/0x13b0
>> [<0>] do_group_exit+0x8b/0x140
>> [<0>] get_signal+0x219/0xc90
>> [<0>] arch_do_signal_or_restart+0x1eb/0xeb0
>> [<0>] exit_to_user_mode_prepare+0x115/0x1a0
>> [<0>] syscall_exit_to_user_mode+0x27/0x50
>> [<0>] do_syscall_64+0x45/0x90
>> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> The 3rd problem is that gdb in a ubuntu 20.04 userspace vm hangs forever:
>>
>> root@ub1704-166:~/samba.git# LANG=C strace -o /dev/shm/strace.txt -f 

Re: [PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

On 3/26/21 5:48 AM, Stefan Metzmacher wrote:
> 
> Am 26.03.21 um 01:39 schrieb Jens Axboe:
>> Hi,
>>
>> As discussed in a previous thread today, the seemingly much saner approach
>> is just to allow signals (including SIGSTOP) for the PF_IO_WORKER IO
>> threads. If we just have the threads call get_signal() for
>> signal_pending(), then everything just falls out naturally with how
>> we receive and handle signals.
>>
>> Patch 1 adds support for checking and calling get_signal() from the
>> regular IO workers, the manager, and the SQPOLL thread. Patch 2 unblocks
>> SIGSTOP from the default IO thread blocked mask, and the rest just revert
>> special cases that were put in place for PF_IO_WORKER threads.
>>
>> With this done, only two special cases remain for PF_IO_WORKER, and they
>> aren't related to signals so not part of this patchset. But both of them
>> can go away as well now that we have "real" threads as IO workers, and
>> then we'll have zero special cases for PF_IO_WORKER.
>>
>> This passes the usual regression testing, my other usual 24h run has been
>> kicked off. But I wanted to send this out early.
>>
>> Thanks to Linus for the suggestion. As with most other good ideas, it's
>> obvious once you hear it. The fact that we end up with _zero_ special
>> cases with this is a clear sign that this is the right way to do it
>> indeed. The fact that this series is 2/3rds revert further drives that
>> point home. Also thanks to Eric for diligent review on the signal side
>> of things for the past changes (and hopefully ditto on this series :-))
> 
> Ok, I'm testing a8ff6a3b20bd16d071ef66824ae4428529d114f9 from
> your io_uring-5.12 branch.
> 
> And using this patch:
> diff --git a/examples/io_uring-cp.c b/examples/io_uring-cp.c
> index cc7a227a5ec7..6e26a4214015 100644
> --- a/examples/io_uring-cp.c
> +++ b/examples/io_uring-cp.c
> @@ -116,13 +116,16 @@ static void queue_write(struct io_uring *ring, struct 
> io_data *data)
> io_uring_submit(ring);
>  }
> 
> -static int copy_file(struct io_uring *ring, off_t insize)
> +static int copy_file(struct io_uring *ring, off_t _insize)
>  {
> +   off_t insize = _insize;
> unsigned long reads, writes;
> struct io_uring_cqe *cqe;
> off_t write_left, offset;
> int ret;
> 
> +again:
> +   insize = _insize;
> write_left = insize;
> writes = reads = offset = 0;
> 
> @@ -221,6 +224,12 @@ static int copy_file(struct io_uring *ring, off_t insize)
> }
> }
> 
> +   {
> +   struct timespec ts = { .tv_nsec = 99, };
> +   nanosleep(, NULL);
> +   goto again;
> +   }
> +
> return 0;
>  }
> 
> Running ./io_uring-cp ~/linux-image-5.12.0-rc2+-dbg_5.12.0-rc2+-5_amd64.deb 
> file
> What I see is this:
> 
> kill -SIGSTOP to any thread I used a worker with pid 2061 here, results in
> 
> root@ub1704-166:~# head /proc/2061/status
> Name:   iou-wrk-2041
> Umask:  0022
> State:  R (running)
> Tgid:   2041
> Ngid:   0
> Pid:2061
> PPid:   1857
> TracerPid:  0
> Uid:0   0   0   0
> Gid:0   0   0   0
> root@ub1704-166:~# head /proc/2041/status
> Name:   io_uring-cp
> Umask:  0022
> State:  T (stopped)
> Tgid:   2041
> Ngid:   0
> Pid:2041
> PPid:   1857
> TracerPid:  0
> Uid:0   0   0   0
> Gid:0   0   0   0
> root@ub1704-166:~# head /proc/2042/status
> Name:   iou-mgr-2041
> Umask:  0022
> State:  T (stopped)
> Tgid:   2041
> Ngid:   0
> Pid:2042
> PPid:   1857
> TracerPid:  0
> Uid:0   0   0   0
> Gid:0   0   0   0
> 
> So userspace and iou-mgr-2041 stop, but the workers don't.
> 49 workers burn cpu as much as possible.
> 
> kill -KILL 2061
> results in this:
> - all workers are gone
> - iou-mgr-2041 is gone
> - io_uring-cp waits in status D forever
> 
> root@ub1704-166:~# head /proc/2041/status
> Name:   io_uring-cp
> Umask:  0022
> State:  D (disk sleep)
> Tgid:   2041
> Ngid:   0
> Pid:2041
> PPid:   1857
> TracerPid:  0
> Uid:0   0   0   0
> Gid:0   0   0   0
> root@ub1704-166:~# cat /proc/2041/stack
> [<0>] io_wq_destroy_manager+0x36/0xa0
> [<0>] io_wq_put_and_exit+0x2b/0x40
> [<0>] io_uring_clean_tctx+0xc5/0x110
> [<0>] __io_uring_files_cancel+0x336/0x4e0
> [<0>] do_exit+0x16b/0x13b0
> [<0>] do_group_exit+0x8b/0x140
> [<0>] get_signal+0x219/0xc90
> [<0>] arch_do_signal_or_restart+0x1eb/0xeb0
> [<0>] exit_to_user_mode_prepare+0x115/0x1a0
> [<0>] syscall_exit_to_user_mode+0x27/0x50
> [<0>] do_syscall_64+0x45/0x90
> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> The 3rd problem is that gdb in a ubuntu 20.04 userspace vm hangs forever:
> 
> root@ub1704-166:~/samba.git# LANG=C strace -o /dev/shm/strace.txt -f -ttT gdb 
> --pid 2417
> GNU gdb (Ubuntu 9.2-0ubuntu1~20.04) 9.2
> Copyright (C) 2020 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 

[PATCH 0/6] Allow signals for IO threads

[Jens Axboe]

Hi,

As discussed in a previous thread today, the seemingly much saner approach
is just to allow signals (including SIGSTOP) for the PF_IO_WORKER IO
threads. If we just have the threads call get_signal() for
signal_pending(), then everything just falls out naturally with how
we receive and handle signals.

Patch 1 adds support for checking and calling get_signal() from the
regular IO workers, the manager, and the SQPOLL thread. Patch 2 unblocks
SIGSTOP from the default IO thread blocked mask, and the rest just revert
special cases that were put in place for PF_IO_WORKER threads.

With this done, only two special cases remain for PF_IO_WORKER, and they
aren't related to signals so not part of this patchset. But both of them
can go away as well now that we have "real" threads as IO workers, and
then we'll have zero special cases for PF_IO_WORKER.

This passes the usual regression testing, my other usual 24h run has been
kicked off. But I wanted to send this out early.

Thanks to Linus for the suggestion. As with most other good ideas, it's
obvious once you hear it. The fact that we end up with _zero_ special
cases with this is a clear sign that this is the right way to do it
indeed. The fact that this series is 2/3rds revert further drives that
point home. Also thanks to Eric for diligent review on the signal side
of things for the past changes (and hopefully ditto on this series :-))

-- 
Jens Axboe