From: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
To: Neil Brown <nfbrown@novell.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>,
Anna Schumaker <anna.schumaker@netapp.com>,
"J. Bruce Fields" <bfields@fieldses.org>,
"Jeff Layton" <jlayton@poochiereds.net>,
"David S. Miller" <davem@davemloft.net>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] sunrpc: fix waitqueue_active without memory barrier in sunrpc
Date: Fri, 9 Oct 2015 06:29:44 +0000 [thread overview]
Message-ID: <17EC94B0A072C34B8DCF0D30AD16044A02874B59@BPXM09GP.gisp.nec.co.jp> (raw)
In-Reply-To: <87h9m04mbt.fsf@notabene.neil.brown.name>
Neil Brown wrote:
> Kosuke Tatsukawa <tatsu@ab.jp.nec.com> writes:
>
>> There are several places in net/sunrpc/svcsock.c which calls
>> waitqueue_active() without calling a memory barrier. Add a memory
>> barrier just as in wq_has_sleeper().
>>
>> I found this issue when I was looking through the linux source code
>> for places calling waitqueue_active() before wake_up*(), but without
>> preceding memory barriers, after sending a patch to fix a similar
>> issue in drivers/tty/n_tty.c (Details about the original issue can be
>> found here: https://lkml.org/lkml/2015/9/28/849).
>
> hi,
> this feels like the wrong approach to the problem. It requires extra
> 'smb_mb's to be spread around which are hard to understand as easy to
> forget.
>
> A quick look seems to suggest that (nearly) every waitqueue_active()
> will need an smb_mb. Could we just put the smb_mb() inside
> waitqueue_active()??
<snip>
There are around 200 occurrences of waitqueue_active() in the kernel
source, and most of the places which use it before wake_up are either
protected by some spin lock, or already has a memory barrier or some
kind of atomic operation before it.
Simply adding smp_mb() to waitqueue_active() would incur extra cost in
many cases and won't be a good idea.
Another way to solve this problem is to remove the waitqueue_active(),
making the code look like this;
if (wq)
wake_up_interruptible(wq);
This also fixes the problem because the spinlock in the wake_up*() acts
as a memory barrier and prevents the code from being reordered by the
CPU (and it also makes the resulting code is much simpler).
---
Kosuke TATSUKAWA | 3rd IT Platform Department
| IT Platform Division, NEC Corporation
| tatsu@ab.jp.nec.com
WARNING: multiple messages have this Message-ID (diff)
From: Kosuke Tatsukawa <tatsu-zZGIbrA41Td8UrSeD/g0lQ@public.gmane.org>
To: Neil Brown <nfbrown-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
Cc: Trond Myklebust
<trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>,
Anna Schumaker
<anna.schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>,
"J. Bruce Fields"
<bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>,
"Jeff Layton" <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>,
"David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>,
"linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH v2] sunrpc: fix waitqueue_active without memory barrier in sunrpc
Date: Fri, 9 Oct 2015 06:29:44 +0000 [thread overview]
Message-ID: <17EC94B0A072C34B8DCF0D30AD16044A02874B59@BPXM09GP.gisp.nec.co.jp> (raw)
In-Reply-To: <87h9m04mbt.fsf-wvvUuzkyo1HefUI2i7LXDhCRmIWqnp/j@public.gmane.org>
Neil Brown wrote:
> Kosuke Tatsukawa <tatsu-zZGIbrA41Td8UrSeD/g0lQ@public.gmane.org> writes:
>
>> There are several places in net/sunrpc/svcsock.c which calls
>> waitqueue_active() without calling a memory barrier. Add a memory
>> barrier just as in wq_has_sleeper().
>>
>> I found this issue when I was looking through the linux source code
>> for places calling waitqueue_active() before wake_up*(), but without
>> preceding memory barriers, after sending a patch to fix a similar
>> issue in drivers/tty/n_tty.c (Details about the original issue can be
>> found here: https://lkml.org/lkml/2015/9/28/849).
>
> hi,
> this feels like the wrong approach to the problem. It requires extra
> 'smb_mb's to be spread around which are hard to understand as easy to
> forget.
>
> A quick look seems to suggest that (nearly) every waitqueue_active()
> will need an smb_mb. Could we just put the smb_mb() inside
> waitqueue_active()??
<snip>
There are around 200 occurrences of waitqueue_active() in the kernel
source, and most of the places which use it before wake_up are either
protected by some spin lock, or already has a memory barrier or some
kind of atomic operation before it.
Simply adding smp_mb() to waitqueue_active() would incur extra cost in
many cases and won't be a good idea.
Another way to solve this problem is to remove the waitqueue_active(),
making the code look like this;
if (wq)
wake_up_interruptible(wq);
This also fixes the problem because the spinlock in the wake_up*() acts
as a memory barrier and prevents the code from being reordered by the
CPU (and it also makes the resulting code is much simpler).
---
Kosuke TATSUKAWA | 3rd IT Platform Department
| IT Platform Division, NEC Corporation
| tatsu-zZGIbrA41Td8UrSeD/g0lQ@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-10-09 6:30 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-09 1:44 [PATCH v2] sunrpc: fix waitqueue_active without memory barrier in sunrpc Kosuke Tatsukawa
2015-10-09 5:56 ` Neil Brown
2015-10-09 6:29 ` Kosuke Tatsukawa [this message]
2015-10-09 6:29 ` Kosuke Tatsukawa
2015-10-09 21:18 ` J. Bruce Fields
2015-10-09 21:21 ` Trond Myklebust
2015-10-12 10:41 ` Kosuke Tatsukawa
2015-10-12 10:41 ` Kosuke Tatsukawa
2015-10-12 20:26 ` J. Bruce Fields
2015-10-14 3:57 ` Kosuke Tatsukawa
2015-10-14 16:00 ` J. Bruce Fields
2015-10-14 16:00 ` J. Bruce Fields
2015-10-15 0:09 ` Kosuke Tatsukawa
2015-10-15 11:44 ` Kosuke Tatsukawa
2015-10-15 11:44 ` Kosuke Tatsukawa
2015-10-15 20:57 ` J. Bruce Fields
2015-10-16 0:49 ` Neil Brown
2015-10-16 1:46 ` Kosuke Tatsukawa
2015-10-16 1:46 ` Kosuke Tatsukawa
2015-10-16 2:28 ` Kosuke Tatsukawa
2015-10-16 2:28 ` Kosuke Tatsukawa
2015-10-22 16:31 ` J. Bruce Fields
2015-10-23 4:14 ` Kosuke Tatsukawa
2015-10-23 4:14 ` Kosuke Tatsukawa
2015-10-23 20:49 ` J. Bruce Fields
2015-10-23 20:49 ` J. Bruce Fields
2015-10-24 1:19 ` Kosuke Tatsukawa
2015-10-24 1:19 ` Kosuke Tatsukawa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=17EC94B0A072C34B8DCF0D30AD16044A02874B59@BPXM09GP.gisp.nec.co.jp \
--to=tatsu@ab.jp.nec.com \
--cc=anna.schumaker@netapp.com \
--cc=bfields@fieldses.org \
--cc=davem@davemloft.net \
--cc=jlayton@poochiereds.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nfbrown@novell.com \
--cc=trond.myklebust@primarydata.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.