Messages in this thread | | | From | Kosuke Tatsukawa <> | Subject | Re: [PATCH] wait: add comment before waitqueue_active noting memory barrier is required | Date | Thu, 22 Oct 2015 08:00:47 +0000 |
| |
Peter Zijlstra wrote: > On Fri, Oct 09, 2015 at 12:35:59AM +0000, Kosuke Tatsukawa wrote: >> This patch adds a comment before waitqueue_active noting that a memory >> barrier is required. >> >> Besides the original problem in drivers/tty/n_tty.c which caused a >> program stall (described in https://lkml.org/lkml/2015/9/28/849), there > > Do not use lkml.org for links in Changelogs -- preferably do _NOT_ refer > to external sources but include all relevant information in the > Changelog. If you have to use links, use: > > http://lkml.kernel.org/r/$msgid > > which is a stable link format.
Thank you for the comments. I'll explain the details in the changelog instead of using links.
>> were several other places in the linux kernel source, which calls >> waitqueue_active without a memory barrier. >> >> blk-mq: fix waitqueue_active without memory barrier in block/blk-mq-tag.c >> media: fix waitqueue_active without memory barrier in cpia2 driver >> mei: fix waitqueue_active without memory barrier in mei drivers >> brcmfmac: fix waitqueue_active without memory barrier in brcmfmac driver >> btrfs: fix waitqueue_active without memory barrier in btrfs >> sunrpc: fix waitqueue_active without memory barrier in sunrpc >> ALSA: seq_oss: fix waitqueue_active without memory barrier in snd-seq-oss >> kvm: fix waitqueue_active without memory barrier in virt/kvm/async_pf.c > > This seems ill specified and superfluous at this point.
Ok.
>> Hopefully, the comment will make people using waitqueue_active a little >> more cautious. >> >> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com> >> --- >> include/linux/wait.h | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/wait.h b/include/linux/wait.h >> index 1e1bf9f..e385564 100644 >> --- a/include/linux/wait.h >> +++ b/include/linux/wait.h >> @@ -102,6 +102,14 @@ init_waitqueue_func_entry(wait_queue_t *q, wait_queue_func_t func) >> q->func = func; >> } >> >> +/* >> + * Note: Some sort of memory barrier must be called before calling >> + * waitqueue_active on SMP, so that any writes done prior to this >> + * can be seen by other CPUs. > > It should very much explain _WHY_ this would be a problem. > > The below is logically separate from the previous, so a new paragraph is > useful.
Ok.
>> Also, since waitqueue_active will >> + * return 0 even when the queue is locked, the waiter must ensure >> + * that a memory barrier is called after add_wait_queue, so that >> + * following reads don't get moved up before the queue has changed. > > And this just doesn't parse at all. It also doesn't fully explain why > that is a problem.
I'll rewrite this second part so that it will be understandable. I'll send an updated patch reflecting your comments.
Best regards. --- Kosuke TATSUKAWA | 3rd IT Platform Department | IT Platform Division, NEC Corporation | tatsu@ab.jp.nec.com
| |