Re: [PATCH] wait: add comment before waitqueue_active noting memory barrier is required

[Peter Zijlstra <peterz@infradead.org>]

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.

> 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.

> 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.

>      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.