lkml.org 
[lkml]   [2015]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] wait: add comment before waitqueue_active noting memory barrier is required
Date
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


\
 
 \ /
  Last update: 2015-10-22 10:21    [W:0.150 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site