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

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

[Kosuke Tatsukawa]

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

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.  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.
+ */
 static inline int waitqueue_active(wait_queue_head_t *q)
 {
 	return !list_empty(&q->task_list);
-- 
1.7.1

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

[Peter Zijlstra]

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.

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

[Kosuke Tatsukawa]

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