Virtualization - May 2023 - [PATCH v11 8/8] vhost: use vhost_tasks for worker threads

Oleg Nesterov <oleg at redhat.com> writes:
> On 05/16, Eric W. Biederman wrote:
>>
>> A kernel thread can block SIGKILL and that is supported.
>>
>> For a thread that is part of a process you can't block SIGKILL when
the
>> task is part of a user mode process.
>
> Or SIGSTOP. Another thread can call
do_signal_stop()->signal_wake_up/etc.
Yes, ignoring SIGSTOP leads to the same kind of rendezvous issues as
SIGKILL.
>> There is this bit in complete_signal when SIGKILL is delivered to any
>> thread in the process.
>>
>> 			t = p;
>> 			do {
>> 				task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
>> 				sigaddset(&t->pending.signal, SIGKILL);
>> 				signal_wake_up(t, 1);
>> 			} while_each_thread(p, t);
>
> That is why the latest version adds try_set_pending_sigkill(). No, no,
> it is not that I think this is a good idea.
I see that try_set_pending_sigkill in the patch now.

That try_set_pending_sigkill just keeps the process from reporting
that it has exited, and extend the process exit indefinitely.

SIGNAL_GROUP_EXIT has already been set, so the KILL signal was
already delivered and the process is exiting.
>> For clarity that sigaddset(&t->pending.signal, SIGKILL);  Really
isn't
>> setting SIGKILL pending,
>
> Hmm. it does? Nevermind.
The point is that what try_set_pending_sigkill in the patch is doing is
keeping the "you are dead exit now" flag, from being set.

That flag is what fatal_signal_pending always tests, because we can only
know if a fatal signal is pending if we have performed short circuit
delivery on the signal.

The result is the effects of the change are mostly what people expect.
The difference the semantics being changed aren't what people think they
are.

AKA process exit is being ignored for the thread, not that SIGKILL is
being blocked.
>> The important part of that code is that SIGNAL_GROUP_EXIT gets set.
>> That indicates the entire process is being torn down.
>
> Yes. and the same is true for io-thread even if it calls get_signal()
> and dequeues SIGKILL and clears TIF_SIGPENDING.
>
>> but in that case the vhost logic needs to act like a process, just
>> like io_uring does.
>
> confused... create_io_thread() creates a sub-thread too?
Yes, create_io_uring creates an ordinary user space thread that never
runs any code in user space.
> Although I never understood this logic. I can't even understand the
usage
> of lower_32_bits() in create_io_thread().
As far as I can tell lower_32_bits(flags) is just defensive programming
that just copies the code in clone.  The code just as easily have said
u32 flags, or have just populated .flags directly.  Then .exit_signal
could have been set to 0.  Later copy_process will set .exit_signal = -1
because CLONE_THREAD is set.

The reason for adding create_io_thread calling copy_process as I recall
so that the new task does not start automatically.  This allows
functions like io_init_new_worker to initialize the new task without
races and then call wake_up_new_task.

Eric

On 05/16, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg at redhat.com> writes:
>
> >> There is this bit in complete_signal when SIGKILL is delivered to
any
> >> thread in the process.
> >>
> >> 			t = p;
> >> 			do {
> >> 				task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
> >> 				sigaddset(&t->pending.signal, SIGKILL);
> >> 				signal_wake_up(t, 1);
> >> 			} while_each_thread(p, t);
> >
> > That is why the latest version adds try_set_pending_sigkill(). No, no,
> > it is not that I think this is a good idea.
>
> I see that try_set_pending_sigkill in the patch now.
>
> That try_set_pending_sigkill just keeps the process from reporting
> that it has exited, and extend the process exit indefinitely.
>
> SIGNAL_GROUP_EXIT has already been set, so the KILL signal was
> already delivered and the process is exiting.
Agreed, that is why I said I don't think try_set_pending_sigkill() is
a good idea.

And again, the same is true for the threads created by
create_io_thread(). get_signal() from io_uring/ can dequeue a pending
SIGKILL and return, but that is all.
> >> For clarity that sigaddset(&t->pending.signal, SIGKILL); 
Really isn't
> >> setting SIGKILL pending,
> >
> > Hmm. it does? Nevermind.
>
> The point is that what try_set_pending_sigkill in the patch is doing is
> keeping the "you are dead exit now" flag, from being set.
>
> That flag is what fatal_signal_pending always tests, because we can only
> know if a fatal signal is pending if we have performed short circuit
> delivery on the signal.
>
> The result is the effects of the change are mostly what people expect.
> The difference the semantics being changed aren't what people think
they
> are.
>
> AKA process exit is being ignored for the thread, not that SIGKILL is
> being blocked.
Sorry, I don't understand. I just tried to say that
sigaddset(&t->pending.signal, SIGKILL) really sets SIGKILL pending.
Nevermind.
> > Although I never understood this logic.
I meant I never really liked how io-threads play with signals,
> I can't even understand the usage
> > of lower_32_bits() in create_io_thread().
>
> As far as I can tell lower_32_bits(flags) is just defensive programming
Cough. but this is ugly. Or I missed something.
> or have just populated .flags directly.
Exactly,
> Then .exit_signal
> could have been set to 0.
Exactly.

-------------------------------------------------------------------------------
OK. It doesn't matter. I tried to read the whole thread and got lost.

IIUC, Mike is going to send the next version? So I think we can delay
the further discussions until then.

Oleg.