lkml.org 
[lkml]   [2009]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 3/4] kthreads: rework kthread_stop()
    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?

    > 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



    \
     
     \ /
      Last update: 2009-02-03 04:27    [W:0.028 / U:0.016 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site