Re: [PATCH 09/18] io-wq: fork worker threads from original task

[ebiederm@xmission.com (Eric W. Biederman)]

Jens Axboe <axboe@kernel.dk> writes:

> On 3/4/21 5:23 AM, Stefan Metzmacher wrote:
>> 
>> Hi Jens,
>> 
>>> +static pid_t fork_thread(int (*fn)(void *), void *arg)
>>> +{
>>> +	unsigned long flags = CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|
>>> +				CLONE_IO|SIGCHLD;
>>> +	struct kernel_clone_args args = {
>>> +		.flags		= ((lower_32_bits(flags) | CLONE_VM |
>>> +				    CLONE_UNTRACED) & ~CSIGNAL),
>>> +		.exit_signal	= (lower_32_bits(flags) & CSIGNAL),
>>> +		.stack		= (unsigned long)fn,
>>> +		.stack_size	= (unsigned long)arg,
>>> +	};
>>> +
>>> +	return kernel_clone(&args);
>>> +}
>> 
>> Can you please explain why CLONE_SIGHAND is used here?
>
> We can't have CLONE_THREAD without CLONE_SIGHAND... The io-wq workers
> don't really care about signals, we don't use them internally.
>
>> Will the userspace signal handlers executed from the kernel thread?
>
> No
>
>> Will SIGCHLD be posted to the userspace signal handlers in a userspace
>> process? Will wait() from userspace see the exit of a thread?
>
> Currently actually it does, but I think that's just an oversight. As far
> as I can tell, we want to add something like the below. Untested... I'll
> give this a spin in a bit.

How do you mean?  Where do you see do_notify_parent being called?

It should not happen in exit_notify, as the new threads should
be neither ptraced nor the thread_group_leader.  Nor should
do_notify_parent be called from wait_task_zombie as PF_IO_WORKERS
are not ptraceable.  Nor should do_notify_parent be called
reparent_leader as the PF_IO_WORKER is not the thread_group_leader.
Non-leader threads always autoreap and their exit_state is either 0
or EXIT_DEAD.

Which leaves calling do_notify_parent in release_task which is perfectly
appropriate if the io_worker is the last thread in the thread_group.

I can see modifying eligible_child so __WCLONE will not cause wait to
show the kernel thread.  I don't think wait_task_stopped or
wait_task_continued will register on PF_IO_WORKER thread if it does not
process signals but I just skimmed those two functions when I was
looking.

It definitely looks like it would be worth modifying do_signal_stop so
that the PF_IO_WORKERs are not included.  Or else modifying the
PF_IO_WORKER threads to stop with the rest of the process in that case.

Eric

> diff --git a/kernel/signal.c b/kernel/signal.c
> index ba4d1ef39a9e..e5db1d8f18e5 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1912,6 +1912,10 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>  	bool autoreap = false;
>  	u64 utime, stime;
>  
> +	/* Don't notify a parent task if an io_uring worker exits */
> +	if (tsk->flags & PF_IO_WORKER)
> +		return true;
> +
>  	BUG_ON(sig == -1);
>  
>   	/* do_notify_parent_cldstop should have been called instead.  */