Re: [PATCH] wait: add comment before waitqueue_active noting memorybarrier 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@xxxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/