Messages in this thread |  | | Date | Wed, 17 May 2023 19:09:18 +0200 | | From | Oleg Nesterov <> | | Subject | Re: [PATCH v11 8/8] vhost: use vhost_tasks for worker threads |
| |
On 05/16, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@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.
|  |