Re: [PATCH] de_thread: Move notify_count write under lock

[Oleg Nesterov <oleg@redhat.com>]

On 02/05, Kirill Tkhai wrote:
>
> В Чт, 05/02/2015 в 17:15 +0300, Kirill Tkhai пишет:
> > В Чт, 05/02/2015 в 14:38 +0100, Oleg Nesterov пишет:
> > > On 02/05, Kirill Tkhai wrote:
> > > >
> > > > The write operation may be reordered with the setting of group_exit_task.
> > > > If so, this fires in exit_notify().
> > >
> > > How?
> > >
> > > OK, yes, "sig->notify_count = -1" can be reordered with the last unlock,
> > > but we do not care?
> > >
> > > group_exit_task + notify_count is only checked under the same lock, and
> > > "notify_count = -1" can't happen until de_thread() sees it is zero.
> > >
> > > Could you explain why this is bad in more details?
> >
> > Can't exit_notify() see tsk->signal->notify_count == -1 before
> > tsk->signal->group_exit_task?

Ah, I am starting understand, thanks!

And sorry, it turns out I didn't read the changelog/comment carefully
enough.

> > As I see in Documentation/memory-barriers.txt:
> >
> > 	RELEASE operation implication:
> > 		Memory operations issued after the RELEASE may be completed before the
> > 		RELEASE operation has completed.

Yes sure. But I think this doesn't matter, and this is why I was confused.
I still do not think that they can be reordered on CPU which does de_thread()
but this doesn't matter too and perhaps I am wrong again.

> if (tsk->signal->notify_count < 0) (== -1)
>
> 	wake_up_process(tsk->signal->group_exit_task); (garbage in group_exit_task)

Yes, thanks for correcting me. Whatever de_thread() does, another CPU can
observe these changes in other order. Not to mention we do not even have
the compiler barrier.

Still. Can't you make the comment more explicit? Say, something like

			/*
			 * Do this under tasklist_lock to ensure that
			 * exit_notify() can't miss ->group_exit_task
			 */
			 sig->notify_count = -1;

although when I re-read your comment it no longer looks confusing to me,
so I won't insist. As for "set notify_count=-1 after ->exit_state check",
this is cosmetic too.

Acked-by: Oleg Nesterov <oleg@redhat.com>


> > > > --- a/fs/exec.c
> > > > +++ b/fs/exec.c
> > > > @@ -920,10 +920,16 @@ static int de_thread(struct task_struct *tsk)
> > > >  	if (!thread_group_leader(tsk)) {
> > > >  		struct task_struct *leader = tsk->group_leader;
> > > >
> > > > -		sig->notify_count = -1;	/* for exit_notify() */
> > > >  		for (;;) {
> > > >  			threadgroup_change_begin(tsk);
> > > >  			write_lock_irq(&tasklist_lock);
> > > > +			/*
> > > > +			 * We could set it once outside the for() cycle, but
> > > > +			 * this requires to use SMP barriers there and in
> > > > +			 * exit_notify(), because the write operation may
> > > > +			 * be reordered with the setting of group_exit_task.
> > > > +			 */
> > > > +			sig->notify_count = -1;	/* for exit_notify() */
> > > >  			if (likely(leader->exit_state))
> > > >  				break;
> > > >  			__set_current_state(TASK_KILLABLE);
> > >
> > > Perhaps something like this makes sense anyway to make the code more
> > > clear, but in this case I'd suggest to set ->notify_count after we
> > > check ->exit_state. And without the (afaics!) misleading comment...
> > >
> > > Or I missed something?
> > >
> > > Oleg.
> > >
> >
>
>