Messages in this thread | | | Date | Mon, 2 Feb 2009 20:41:05 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 3/4] kthreads: rework kthread_stop() |
| |
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. 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.
> 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.
Oleg.
| |