lkml.org 
[lkml]   [2009]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/4] kthreads: rework kthread_stop()
On Mon, Feb 02, 2009 at 07:25:44PM -0800, Eric W. Biederman wrote:
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 02/02, Eric W. Biederman wrote:
> >>
> >> Oleg on that note we should not need a barrier at all. We should be
> >> able to simply say:
> >>
> >> cmplp = k->vfork_done;
> >> if (cmplp){
> >> /* if vfork_done is NULL we have passed mm_release */
> >> kthread = container_of(cmplp, struct kthread, exited);
> >> kthread->should_stop = 1;
> >> wake_up_process(k);
> >> wait_for_completion(&kthread->exited);
> >> }
> >
> > Yes, but the compiler can read ->vfork_done twice, and turn this code
> > into
> >
> > cmplp = k->vfork_done;
> > if (cmplp){
> > kthread = container_of(k->vfork_done, struct kthread, exited);
> > ...
> >
> > and when we read k->vfork_done again it can be already NULL.
> > Probably we could use ACCESS_ONCE() instead.
> >
> > Perhaps this barrier() is not needed in practice, but just to be safe.
>
> Certainly. I definitely see where you are coming from.
> And of course all of this only works because a pointer is a word size
> so it is read and updated atomically by the compiler.
>
> I wish we had a good idiom we could use to make it clear what we
> are doing. The rcu pointer read code perhaps?

ACCESS_ONCE() suffices in many cases, but if the pointer being accessed
points to a structure that might recently have been initialized, then
rcu_dereference() will be required on Alpha. Though perhaps the
discussion below removes the need entirely, but cannot say that I fully
understand this part of the kernel.

Thanx, Paul

> > And in fact I saw the bug report with this code:
> >
> > ac.ac_tty = current->signal->tty ?
> > old_encode_dev(tty_devnum(current->signal->tty)) : 0;
> >
> > this code is wrong anyway, but ->tty was read twice. I specially
> > asked for .s file because I wasn't able to believe the bug manifests
> > itself this way.
>
> Interesting.
>
> >> Thinking of it I wish we had someplace we could store a pointer
> >> that would not be cleared so we could remove that whole confusing
> >> conditional. I just looked through task_struct and there doesn't
> >> appear to be anything promising.
> >>
> >> Perhaps we could rename vfork_done mm_done and not clear it in
> >> mm_release.
> >
> > Yes, in that case we don't need the barrier().
> >
> > I was thinking about changing mm_release() too, but we should clear
> > ->vfork_done (or whatever) in exec_mmap() anyway.
>
> Yes. I realized that just after I wrote that. So clearing
> vfork_done in all cases is a good idea so we don't make get sloppy.
>
> Eric
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/


\
 
 \ /
  Last update: 2009-02-03 14:45    [W:0.068 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site