* [PATCH tip/core/rcu 2/9] compiler: Allow 1- and 2-byte smp_load_acquire() and smp_store_release()
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
@ 2014-10-28 22:09 ` Paul E. McKenney
2014-10-28 22:09 ` [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer Paul E. McKenney
` (6 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:09 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CPUs without single-byte and double-byte loads and stores place some
"interesting" requirements on concurrent code. For example (adapted
from Peter Hurley's test code), suppose we have the following structure:
struct foo {
spinlock_t lock1;
spinlock_t lock2;
char a; /* Protected by lock1. */
char b; /* Protected by lock2. */
};
struct foo *foop;
Of course, it is common (and good) practice to place data protected
by different locks in separate cache lines. However, if the locks are
rarely acquired (for example, only in rare error cases), and there are
a great many instances of the data structure, then memory footprint can
trump false-sharing concerns, so that it can be better to place them in
the same cache cache line as above.
But if the CPU does not support single-byte loads and stores, a store
to foop->a will do a non-atomic read-modify-write operation on foop->b,
which will come as a nasty surprise to someone holding foop->lock2. So we
now require CPUs to support single-byte and double-byte loads and stores.
Therefore, this commit adjusts the definition of __native_word() to allow
these sizes to be used by smp_load_acquire() and smp_store_release().
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
include/linux/compiler.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d5ad7b1118fc..934a834ab9f9 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -311,7 +311,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
/* Is this type a native word size -- useful for atomic operations */
#ifndef __native_word
-# define __native_word(t) (sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
+# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
#endif
/* Compile time object size, -1 for unknown */
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
2014-10-28 22:09 ` [PATCH tip/core/rcu 2/9] compiler: Allow 1- and 2-byte smp_load_acquire() and smp_store_release() Paul E. McKenney
@ 2014-10-28 22:09 ` Paul E. McKenney
2014-11-21 13:31 ` Kirill A. Shutemov
2014-10-28 22:09 ` [PATCH tip/core/rcu 4/9] dm: sparse: Annotate field with __rcu for checking Paul E. McKenney
` (5 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:09 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: Pranith Kumar <bobby.prani@gmail.com>
Got Paul's email wrong the first time.
The map field in 'struct mapped_device' is an rcu pointer. Use rcu_dereference()
while accessing it.
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
drivers/md/dm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 58f3927fd7cc..e7399362722e 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2332,7 +2332,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
merge_is_optional = dm_table_merge_is_optional(t);
- old_map = md->map;
+ old_map = rcu_dereference(md->map);
rcu_assign_pointer(md->map, t);
md->immutable_target_type = dm_table_get_immutable_target_type(t);
@@ -2351,7 +2351,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
*/
static struct dm_table *__unbind(struct mapped_device *md)
{
- struct dm_table *map = md->map;
+ struct dm_table *map = rcu_dereference(md->map);
if (!map)
return NULL;
@@ -2745,7 +2745,7 @@ int dm_suspend(struct mapped_device *md, unsigned suspend_flags)
goto out_unlock;
}
- map = md->map;
+ map = rcu_dereference(md->map);
/*
* DMF_NOFLUSH_SUSPENDING must be set before presuspend.
@@ -2839,7 +2839,7 @@ int dm_resume(struct mapped_device *md)
if (!dm_suspended_md(md))
goto out;
- map = md->map;
+ map = rcu_dereference(md->map);
if (!map || !dm_table_get_size(map))
goto out;
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer
2014-10-28 22:09 ` [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer Paul E. McKenney
@ 2014-11-21 13:31 ` Kirill A. Shutemov
2014-11-21 14:30 ` Pranith Kumar
0 siblings, 1 reply; 25+ messages in thread
From: Kirill A. Shutemov @ 2014-11-21 13:31 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, tglx, peterz, rostedt, dhowells, edumazet, dvhart, fweisbec,
oleg, bobby.prani
On Tue, Oct 28, 2014 at 03:09:56PM -0700, Paul E. McKenney wrote:
> From: Pranith Kumar <bobby.prani@gmail.com>
>
> Got Paul's email wrong the first time.
>
> The map field in 'struct mapped_device' is an rcu pointer. Use rcu_dereference()
> while accessing it.
>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
On current -next I see this:
[ 6.388264] ===============================
[ 6.389571] [ INFO: suspicious RCU usage. ]
[ 6.390869] 3.18.0-rc5-next-20141121-08303-g44cae4530372 #2 Not tainted
[ 6.392185] -------------------------------
[ 6.393479] /home/kas/git/public/linux/drivers/md/dm.c:2853 suspicious rcu_dereference_check() usage!
[ 6.394801]
other info that might help us debug this:
[ 6.398714]
rcu_scheduler_active = 1, debug_locks = 0
[ 6.401247] 1 lock held by cryptsetup/159:
[ 6.402522] #0: (&md->suspend_lock/1){+.+...}, at: [<ffffffff8179dd1d>] dm_suspend+0x3d/0x140
[ 6.403848]
stack backtrace:
[ 6.406448] CPU: 3 PID: 159 Comm: cryptsetup Not tainted 3.18.0-rc5-next-20141121-08303-g44cae4530372 #2
[ 6.407726] Hardware name: LENOVO 3460CC6/3460CC6, BIOS G6ET93WW (2.53 ) 02/04/2013
[ 6.408982] 0000000000000001 ffff8800d3ac7c38 ffffffff81b00bbd 0000000000000011
[ 6.410249] ffff8800d301a560 ffff8800d3ac7c68 ffffffff81153be7 ffff8800d3928800
[ 6.411548] ffff8800d3928970 ffff8800d3928aa0 0000000000000001 ffff8800d3ac7ca8
[ 6.412780] Call Trace:
[ 6.413980] [<ffffffff81b00bbd>] dump_stack+0x4c/0x6e
[ 6.415178] [<ffffffff81153be7>] lockdep_rcu_suspicious+0xe7/0x120
[ 6.416364] [<ffffffff8179de1d>] dm_suspend+0x13d/0x140
[ 6.417535] [<ffffffff817a2d80>] ? table_load+0x340/0x340
[ 6.418749] [<ffffffff817a2f2b>] dev_suspend+0x1ab/0x260
[ 6.419901] [<ffffffff817a2d80>] ? table_load+0x340/0x340
[ 6.421038] [<ffffffff817a3781>] ctl_ioctl+0x251/0x540
[ 6.422164] [<ffffffff812b6415>] ? mntput_no_expire+0x5/0x360
[ 6.423280] [<ffffffff817a3a83>] dm_ctl_ioctl+0x13/0x20
[ 6.424389] [<ffffffff812a6f98>] do_vfs_ioctl+0x308/0x540
[ 6.425515] [<ffffffff81175d2d>] ? rcu_read_lock_held+0x6d/0x70
[ 6.426601] [<ffffffff812b324e>] ? __fget_light+0xbe/0xd0
[ 6.427686] [<ffffffff812a7251>] SyS_ioctl+0x81/0xa0
[ 6.428760] [<ffffffff81b0d6ad>] system_call_fastpath+0x16/0x1b
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer
2014-11-21 13:31 ` Kirill A. Shutemov
@ 2014-11-21 14:30 ` Pranith Kumar
2014-11-21 14:58 ` Kirill A. Shutemov
0 siblings, 1 reply; 25+ messages in thread
From: Pranith Kumar @ 2014-11-21 14:30 UTC (permalink / raw)
To: Kirill A. Shutemov, Paul E. McKenney
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, tglx, peterz, rostedt, dhowells, edumazet, dvhart, fweisbec,
oleg
On 11/21/2014 08:31 AM, Kirill A. Shutemov wrote:
> On Tue, Oct 28, 2014 at 03:09:56PM -0700, Paul E. McKenney wrote:
>> From: Pranith Kumar <bobby.prani@gmail.com>
>>
>> Got Paul's email wrong the first time.
>>
>> The map field in 'struct mapped_device' is an rcu pointer. Use rcu_dereference()
>> while accessing it.
>>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> On current -next I see this:
>
> [ 6.388264] ===============================
> [ 6.389571] [ INFO: suspicious RCU usage. ]
> [ 6.390869] 3.18.0-rc5-next-20141121-08303-g44cae4530372 #2 Not tainted
> [ 6.392185] -------------------------------
> [ 6.393479] /home/kas/git/public/linux/drivers/md/dm.c:2853 suspicious rcu_dereference_check() usage!
> [ 6.394801]
> other info that might help us debug this:
>
Hi Kirill,
We are dereferencing an RCU pointer with the suspend_lock held which is causing this warning.
Can you please check if the following patch helps? Thanks!
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index a0ece87..e584e66 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2830,7 +2830,7 @@ static int __dm_suspend(struct mapped_device *md, struct dm_table *map,
*/
int dm_suspend(struct mapped_device *md, unsigned suspend_flags)
{
- struct dm_table *map = NULL;
+ struct dm_table *map = rcu_dereference(md->map);
int r = 0;
retry:
@@ -2850,8 +2850,6 @@ retry:
goto retry;
}
- map = rcu_dereference(md->map);
-
r = __dm_suspend(md, map, suspend_flags, TASK_INTERRUPTIBLE);
if (r)
goto out_unlock;
> [ 6.398714]
> rcu_scheduler_active = 1, debug_locks = 0
> [ 6.401247] 1 lock held by cryptsetup/159:
> [ 6.402522] #0: (&md->suspend_lock/1){+.+...}, at: [<ffffffff8179dd1d>] dm_suspend+0x3d/0x140
> [ 6.403848]
> stack backtrace:
> [ 6.406448] CPU: 3 PID: 159 Comm: cryptsetup Not tainted 3.18.0-rc5-next-20141121-08303-g44cae4530372 #2
> [ 6.407726] Hardware name: LENOVO 3460CC6/3460CC6, BIOS G6ET93WW (2.53 ) 02/04/2013
> [ 6.408982] 0000000000000001 ffff8800d3ac7c38 ffffffff81b00bbd 0000000000000011
> [ 6.410249] ffff8800d301a560 ffff8800d3ac7c68 ffffffff81153be7 ffff8800d3928800
> [ 6.411548] ffff8800d3928970 ffff8800d3928aa0 0000000000000001 ffff8800d3ac7ca8
> [ 6.412780] Call Trace:
> [ 6.413980] [<ffffffff81b00bbd>] dump_stack+0x4c/0x6e
> [ 6.415178] [<ffffffff81153be7>] lockdep_rcu_suspicious+0xe7/0x120
> [ 6.416364] [<ffffffff8179de1d>] dm_suspend+0x13d/0x140
> [ 6.417535] [<ffffffff817a2d80>] ? table_load+0x340/0x340
> [ 6.418749] [<ffffffff817a2f2b>] dev_suspend+0x1ab/0x260
> [ 6.419901] [<ffffffff817a2d80>] ? table_load+0x340/0x340
> [ 6.421038] [<ffffffff817a3781>] ctl_ioctl+0x251/0x540
> [ 6.422164] [<ffffffff812b6415>] ? mntput_no_expire+0x5/0x360
> [ 6.423280] [<ffffffff817a3a83>] dm_ctl_ioctl+0x13/0x20
> [ 6.424389] [<ffffffff812a6f98>] do_vfs_ioctl+0x308/0x540
> [ 6.425515] [<ffffffff81175d2d>] ? rcu_read_lock_held+0x6d/0x70
> [ 6.426601] [<ffffffff812b324e>] ? __fget_light+0xbe/0xd0
> [ 6.427686] [<ffffffff812a7251>] SyS_ioctl+0x81/0xa0
> [ 6.428760] [<ffffffff81b0d6ad>] system_call_fastpath+0x16/0x1b
>
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer
2014-11-21 14:30 ` Pranith Kumar
@ 2014-11-21 14:58 ` Kirill A. Shutemov
2014-11-23 12:21 ` Pranith Kumar
0 siblings, 1 reply; 25+ messages in thread
From: Kirill A. Shutemov @ 2014-11-21 14:58 UTC (permalink / raw)
To: Pranith Kumar
Cc: Paul E. McKenney, linux-kernel, mingo, laijs, dipankar, akpm,
mathieu.desnoyers, josh, tglx, peterz, rostedt, dhowells,
edumazet, dvhart, fweisbec, oleg
On Fri, Nov 21, 2014 at 09:30:36AM -0500, Pranith Kumar wrote:
> On 11/21/2014 08:31 AM, Kirill A. Shutemov wrote:
> > On Tue, Oct 28, 2014 at 03:09:56PM -0700, Paul E. McKenney wrote:
> >> From: Pranith Kumar <bobby.prani@gmail.com>
> >>
> >> Got Paul's email wrong the first time.
> >>
> >> The map field in 'struct mapped_device' is an rcu pointer. Use rcu_dereference()
> >> while accessing it.
> >>
> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> >> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> > On current -next I see this:
> >
> > [ 6.388264] ===============================
> > [ 6.389571] [ INFO: suspicious RCU usage. ]
> > [ 6.390869] 3.18.0-rc5-next-20141121-08303-g44cae4530372 #2 Not tainted
> > [ 6.392185] -------------------------------
> > [ 6.393479] /home/kas/git/public/linux/drivers/md/dm.c:2853 suspicious rcu_dereference_check() usage!
> > [ 6.394801]
> > other info that might help us debug this:
> >
>
> Hi Kirill,
>
> We are dereferencing an RCU pointer with the suspend_lock held which is causing this warning.
>
> Can you please check if the following patch helps? Thanks!
Nope. The same issue.
IIUC, the problem is that you dereference pointer outside rcu_read_lock()
section, not that suspend_lock is held.
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer
2014-11-21 14:58 ` Kirill A. Shutemov
@ 2014-11-23 12:21 ` Pranith Kumar
2014-11-23 16:39 ` Eric Dumazet
2014-11-23 16:40 ` [PATCH ] drivers/md: use proper rcu accessor Eric Dumazet
0 siblings, 2 replies; 25+ messages in thread
From: Pranith Kumar @ 2014-11-23 12:21 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Paul E. McKenney, LKML, Ingo Molnar, Lai Jiangshan,
Dipankar Sarma, Andrew Morton, Mathieu Desnoyers, Josh Triplett,
Thomas Gleixner, Peter Zijlstra, Steven Rostedt, David Howells,
Eric Dumazet, dvhart, Frédéric Weisbecker,
Oleg Nesterov
On Fri, Nov 21, 2014 at 9:58 AM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Fri, Nov 21, 2014 at 09:30:36AM -0500, Pranith Kumar wrote:
>> On 11/21/2014 08:31 AM, Kirill A. Shutemov wrote:
>> > On Tue, Oct 28, 2014 at 03:09:56PM -0700, Paul E. McKenney wrote:
>> >> From: Pranith Kumar <bobby.prani@gmail.com>
>> >>
>> >> Got Paul's email wrong the first time.
>> >>
>> >> The map field in 'struct mapped_device' is an rcu pointer. Use rcu_dereference()
>> >> while accessing it.
>> >>
>> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> >> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> >
>> > On current -next I see this:
>> >
>> > [ 6.388264] ===============================
>> > [ 6.389571] [ INFO: suspicious RCU usage. ]
>> > [ 6.390869] 3.18.0-rc5-next-20141121-08303-g44cae4530372 #2 Not tainted
>> > [ 6.392185] -------------------------------
>> > [ 6.393479] /home/kas/git/public/linux/drivers/md/dm.c:2853 suspicious rcu_dereference_check() usage!
>> > [ 6.394801]
>> > other info that might help us debug this:
>> >
>>
>> Hi Kirill,
>>
>> We are dereferencing an RCU pointer with the suspend_lock held which is causing this warning.
>>
>> Can you please check if the following patch helps? Thanks!
>
> Nope. The same issue.
>
> IIUC, the problem is that you dereference pointer outside rcu_read_lock()
> section, not that suspend_lock is held.
>
I am not sure we should be taking rcu_read_lock() there as I am not
sure how long that critical section might last. Can someone who is
more familiar with the code take a look?
I will try to look for a solution too in the mean time.
Thanks!
--
Pranith
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer
2014-11-23 12:21 ` Pranith Kumar
@ 2014-11-23 16:39 ` Eric Dumazet
2014-11-23 16:40 ` [PATCH ] drivers/md: use proper rcu accessor Eric Dumazet
1 sibling, 0 replies; 25+ messages in thread
From: Eric Dumazet @ 2014-11-23 16:39 UTC (permalink / raw)
To: Pranith Kumar
Cc: Kirill A. Shutemov, Paul E. McKenney, LKML, Ingo Molnar,
Lai Jiangshan, Dipankar Sarma, Andrew Morton, Mathieu Desnoyers,
Josh Triplett, Thomas Gleixner, Peter Zijlstra, Steven Rostedt,
David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov
On Sun, 2014-11-23 at 07:21 -0500, Pranith Kumar wrote:
> I am not sure we should be taking rcu_read_lock() there as I am not
> sure how long that critical section might last. Can someone who is
> more familiar with the code take a look?
>
> I will try to look for a solution too in the mean time.
I am sending a fix.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH ] drivers/md: use proper rcu accessor
2014-11-23 12:21 ` Pranith Kumar
2014-11-23 16:39 ` Eric Dumazet
@ 2014-11-23 16:40 ` Eric Dumazet
2014-11-23 16:53 ` Mike Snitzer
2014-11-23 17:34 ` [PATCH v2] " Eric Dumazet
1 sibling, 2 replies; 25+ messages in thread
From: Eric Dumazet @ 2014-11-23 16:40 UTC (permalink / raw)
To: Pranith Kumar
Cc: Kirill A. Shutemov, Paul E. McKenney, LKML, Ingo Molnar,
Lai Jiangshan, Dipankar Sarma, Andrew Morton, Mathieu Desnoyers,
Josh Triplett, Thomas Gleixner, Peter Zijlstra, Steven Rostedt,
David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov
From: Eric Dumazet <edumazet@google.com>
rcu_dereference() should be used in sections protected by rcu_read_lock.
For writers, holding some kind of mutex or lock,
rcu_dereference_protected() is the way to go, adding explicit lockdep
bits.
In __unbind(), although there is no mutex or lock held, we are about
to free the mapped device, so can use the constant '1' instead of a
lockdep_is_held()
Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
Cc: Pranith Kumar <bobby.prani@gmail.com>
---
drivers/md/dm.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index a0ece87ad426..2e0aa2273382 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2335,7 +2335,8 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
merge_is_optional = dm_table_merge_is_optional(t);
- old_map = rcu_dereference(md->map);
+ old_map = rcu_dereference_protected(md->map,
+ lockdep_is_held(&md->suspend_lock));
rcu_assign_pointer(md->map, t);
md->immutable_target_type = dm_table_get_immutable_target_type(t);
@@ -2355,7 +2356,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
*/
static struct dm_table *__unbind(struct mapped_device *md)
{
- struct dm_table *map = rcu_dereference(md->map);
+ struct dm_table *map = rcu_dereference_protected(md->map, 1);
if (!map)
return NULL;
@@ -2850,7 +2851,8 @@ retry:
goto retry;
}
- map = rcu_dereference(md->map);
+ map = rcu_dereference_protected(md->map,
+ lockdep_is_held(&md->suspend_lock));
r = __dm_suspend(md, map, suspend_flags, TASK_INTERRUPTIBLE);
if (r)
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH ] drivers/md: use proper rcu accessor
2014-11-23 16:40 ` [PATCH ] drivers/md: use proper rcu accessor Eric Dumazet
@ 2014-11-23 16:53 ` Mike Snitzer
2014-11-23 17:31 ` Eric Dumazet
2014-11-23 17:34 ` [PATCH v2] " Eric Dumazet
1 sibling, 1 reply; 25+ messages in thread
From: Mike Snitzer @ 2014-11-23 16:53 UTC (permalink / raw)
To: Eric Dumazet
Cc: Pranith Kumar, Kirill A. Shutemov, Paul E. McKenney, LKML,
Ingo Molnar, Lai Jiangshan, Dipankar Sarma, Andrew Morton,
Mathieu Desnoyers, Josh Triplett, Thomas Gleixner, Peter Zijlstra,
Steven Rostedt, David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov,
device-mapper development
On Sun, Nov 23, 2014 at 11:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> rcu_dereference() should be used in sections protected by rcu_read_lock.
>
> For writers, holding some kind of mutex or lock,
> rcu_dereference_protected() is the way to go, adding explicit lockdep
> bits.
>
> In __unbind(), although there is no mutex or lock held, we are about
> to free the mapped device, so can use the constant '1' instead of a
> lockdep_is_held()
That isn't true. dm_hash_remove_all() -- which calls dm_destroy --
holds _hash_lock. Why leave __unbind() brittle in the face of future
DM locking changes?
> Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> Cc: Pranith Kumar <bobby.prani@gmail.com>
Hi Eric,
I'll pick this up once I get clarification for why your __unbind
change is safe.. but it really would've helped if you cc'd
dm-devel@redhat.com or myself directly (not a single person that you
cc'd actively maintains DM).
Hopefully these DM rcu "fixes" are finished after this.
Thanks,
Mike
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH ] drivers/md: use proper rcu accessor
2014-11-23 16:53 ` Mike Snitzer
@ 2014-11-23 17:31 ` Eric Dumazet
2014-11-23 19:12 ` Mike Snitzer
0 siblings, 1 reply; 25+ messages in thread
From: Eric Dumazet @ 2014-11-23 17:31 UTC (permalink / raw)
To: Mike Snitzer
Cc: Pranith Kumar, Kirill A. Shutemov, Paul E. McKenney, LKML,
Ingo Molnar, Lai Jiangshan, Dipankar Sarma, Andrew Morton,
Mathieu Desnoyers, Josh Triplett, Thomas Gleixner, Peter Zijlstra,
Steven Rostedt, David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov,
device-mapper development
On Sun, 2014-11-23 at 11:53 -0500, Mike Snitzer wrote:
> On Sun, Nov 23, 2014 at 11:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > rcu_dereference() should be used in sections protected by rcu_read_lock.
> >
> > For writers, holding some kind of mutex or lock,
> > rcu_dereference_protected() is the way to go, adding explicit lockdep
> > bits.
> >
> > In __unbind(), although there is no mutex or lock held, we are about
> > to free the mapped device, so can use the constant '1' instead of a
> > lockdep_is_held()
>
> That isn't true. dm_hash_remove_all() -- which calls dm_destroy --
> holds _hash_lock. Why leave __unbind() brittle in the face of future
> DM locking changes?
>
Well, tell me. Before the 33423974bfc1 patch there was no protection.
If really you are about to delete an object, you have to be sure no one
is going to use it.
rcu_dereference_protected(X, 1) is how we express this thing, there is
nothing wrong here.
Fact that you hold a lock at this point is irrelevant and wont protect
the bug from happening. If you believe so, then you are wrong.
> > Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> > Cc: Pranith Kumar <bobby.prani@gmail.com>
>
> Hi Eric,
>
> I'll pick this up once I get clarification for why your __unbind
> change is safe.. but it really would've helped if you cc'd
> dm-devel@redhat.com or myself directly (not a single person that you
> cc'd actively maintains DM).
>
Hmm, my mailer complained because the mail had too many recipients
already. I did a 'reply' on the original thread.
> Hopefully these DM rcu "fixes" are finished after this.
You added a Signed-off-by on 33423974bfc1, not me.
Kirill gave the report 2 days ago and so far nobody fixed it.
I will send a v2 because other rcu_dereference() need to be changed as
well.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH ] drivers/md: use proper rcu accessor
2014-11-23 17:31 ` Eric Dumazet
@ 2014-11-23 19:12 ` Mike Snitzer
0 siblings, 0 replies; 25+ messages in thread
From: Mike Snitzer @ 2014-11-23 19:12 UTC (permalink / raw)
To: Eric Dumazet
Cc: Pranith Kumar, Kirill A. Shutemov, Paul E. McKenney, LKML,
Ingo Molnar, Lai Jiangshan, Dipankar Sarma, Andrew Morton,
Mathieu Desnoyers, Josh Triplett, Thomas Gleixner, Peter Zijlstra,
Steven Rostedt, David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov,
device-mapper development
On Sun, Nov 23 2014 at 12:31pm -0500,
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2014-11-23 at 11:53 -0500, Mike Snitzer wrote:
> > On Sun, Nov 23, 2014 at 11:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > rcu_dereference() should be used in sections protected by rcu_read_lock.
> > >
> > > For writers, holding some kind of mutex or lock,
> > > rcu_dereference_protected() is the way to go, adding explicit lockdep
> > > bits.
> > >
> > > In __unbind(), although there is no mutex or lock held, we are about
> > > to free the mapped device, so can use the constant '1' instead of a
> > > lockdep_is_held()
> >
> > That isn't true. dm_hash_remove_all() -- which calls dm_destroy --
> > holds _hash_lock. Why leave __unbind() brittle in the face of future
> > DM locking changes?
> >
>
> Well, tell me. Before the 33423974bfc1 patch there was no protection.
Wasn't protected by _hash_lock or wasn't protected by use of rcu_deference()?
> If really you are about to delete an object, you have to be sure no one
> is going to use it.
>
> rcu_dereference_protected(X, 1) is how we express this thing, there is
> nothing wrong here.
>
> Fact that you hold a lock at this point is irrelevant and wont protect
> the bug from happening. If you believe so, then you are wrong.
My asking a question about the validity of your assertion given that
assertion wasn't 100% correct is perfectly fair no? I just want to make
sure the change is accurately described. Asking that question also
doesn't imply I felt you don't know what you're doing. Fact is I really
trust you to be a _very_ capable developer.
But exactly which "bug" are we talking about? A theoretical bug or a
reported bug that caused DM to fail? So far all of these supposed rcu
deference fixes have _never_ substantiated with an actual bug (Other
than a splat from autochecking performed by rcu_dereference_check).
In fact the very first "fix" from 33423974bfc1 _seems_ to just be the
by-product from rcu janitor efforts. I'm happy others are looking after
rcu consumers but pretty sure all of this churn is what is causing these
"bugs".
But maybe I'm mistaken and all these changes shoul dbe cc'ing
stable@vger.kernel.org?
> > > Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> > > Cc: Pranith Kumar <bobby.prani@gmail.com>
> >
> > Hi Eric,
> >
> > I'll pick this up once I get clarification for why your __unbind
> > change is safe.. but it really would've helped if you cc'd
> > dm-devel@redhat.com or myself directly (not a single person that you
> > cc'd actively maintains DM).
> >
>
> Hmm, my mailer complained because the mail had too many recipients
> already. I did a 'reply' on the original thread.
It's OK, I missed the report from 2 days ago too (wasn't cc'd etc).
> > Hopefully these DM rcu "fixes" are finished after this.
>
> You added a Signed-off-by on 33423974bfc1, not me.
Right, I don't pretend to keep my finger on the pulse of rcu API as much
as I probably should. But I'm pretty sure Paul McKenney does and he
also provided his Signed-off-by -- and I really trust Paul on rcu stuff.
> Kirill gave the report 2 days ago and so far nobody fixed it.
>
> I will send a v2 because other rcu_dereference() need to be changed as
> well.
I'm still wondering if they _need_ to be changed -- we aren't already
protected in these paths due to DM's existing locking (via suspend_lock
or _hash_lock, etc)? But I'll circle back to try to properly understand
the need shortly.
Anyway, all being said: I appreciate your help here. Not liking that we
got to baiting one another; I'll refrain from doing so in the future.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2] drivers/md: use proper rcu accessor
2014-11-23 16:40 ` [PATCH ] drivers/md: use proper rcu accessor Eric Dumazet
2014-11-23 16:53 ` Mike Snitzer
@ 2014-11-23 17:34 ` Eric Dumazet
2014-11-24 4:09 ` Mike Snitzer
1 sibling, 1 reply; 25+ messages in thread
From: Eric Dumazet @ 2014-11-23 17:34 UTC (permalink / raw)
To: Pranith Kumar, Mike Snitzer, dm-devel
Cc: Kirill A. Shutemov, Paul E. McKenney, LKML, Ingo Molnar,
Lai Jiangshan, Dipankar Sarma, Andrew Morton, Mathieu Desnoyers,
Josh Triplett, Thomas Gleixner, Peter Zijlstra, Steven Rostedt,
David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov
From: Eric Dumazet <edumazet@google.com>
rcu_dereference() should be used in sections protected by rcu_read_lock.
For writers, holding some kind of mutex or lock,
rcu_dereference_protected() is the way to go, adding explicit lockdep
bits.
In __unbind(), we are the last user of this mapped device, so can use
the constant '1' instead of a lockdep_is_held(), not consistent with
other uses of rcu_dereference_protected() which use md->suspend_lock
mutex.
Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
Cc: Pranith Kumar <bobby.prani@gmail.com>
Cc: Mike Snitzer <snitzer@redhat.com>
---
v2: changed all buggy rcu_dereference()
BTW, having a mutex named suspend_lock is ugly, IMO.
drivers/md/dm.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index a0ece87ad426..5919d933bce9 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2335,7 +2335,8 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
merge_is_optional = dm_table_merge_is_optional(t);
- old_map = rcu_dereference(md->map);
+ old_map = rcu_dereference_protected(md->map,
+ lockdep_is_held(&md->suspend_lock));
rcu_assign_pointer(md->map, t);
md->immutable_target_type = dm_table_get_immutable_target_type(t);
@@ -2355,7 +2356,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
*/
static struct dm_table *__unbind(struct mapped_device *md)
{
- struct dm_table *map = rcu_dereference(md->map);
+ struct dm_table *map = rcu_dereference_protected(md->map, 1);
if (!map)
return NULL;
@@ -2850,7 +2851,8 @@ retry:
goto retry;
}
- map = rcu_dereference(md->map);
+ map = rcu_dereference_protected(md->map,
+ lockdep_is_held(&md->suspend_lock));
r = __dm_suspend(md, map, suspend_flags, TASK_INTERRUPTIBLE);
if (r)
@@ -2908,7 +2910,8 @@ retry:
goto retry;
}
- map = rcu_dereference(md->map);
+ map = rcu_dereference_protected(md->map,
+ lockdep_is_held(&md->suspend_lock));
if (!map || !dm_table_get_size(map))
goto out;
@@ -2943,7 +2946,8 @@ static void __dm_internal_suspend(struct mapped_device *md, unsigned suspend_fla
return; /* nest suspend */
}
- map = rcu_dereference(md->map);
+ map = rcu_dereference_protected(md->map,
+ lockdep_is_held(&md->suspend_lock));
/*
* Using TASK_UNINTERRUPTIBLE because only NOFLUSH internal suspend is
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2] drivers/md: use proper rcu accessor
2014-11-23 17:34 ` [PATCH v2] " Eric Dumazet
@ 2014-11-24 4:09 ` Mike Snitzer
0 siblings, 0 replies; 25+ messages in thread
From: Mike Snitzer @ 2014-11-24 4:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: Pranith Kumar, dm-devel, Kirill A. Shutemov, Paul E. McKenney,
LKML, Ingo Molnar, Lai Jiangshan, Dipankar Sarma, Andrew Morton,
Mathieu Desnoyers, Josh Triplett, Thomas Gleixner, Peter Zijlstra,
Steven Rostedt, David Howells, Eric Dumazet, dvhart,
Frédéric Weisbecker, Oleg Nesterov
On Sun, Nov 23 2014 at 12:34pm -0500,
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> rcu_dereference() should be used in sections protected by rcu_read_lock.
>
> For writers, holding some kind of mutex or lock,
> rcu_dereference_protected() is the way to go, adding explicit lockdep
> bits.
>
> In __unbind(), we are the last user of this mapped device, so can use
> the constant '1' instead of a lockdep_is_held(), not consistent with
> other uses of rcu_dereference_protected() which use md->suspend_lock
> mutex.
>
> Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> Cc: Pranith Kumar <bobby.prani@gmail.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
Thanks, I've staged this for 3.19:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.19&id=a12f5d48bdfeb5fe10157ac01c3de29269f457c6
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH tip/core/rcu 4/9] dm: sparse: Annotate field with __rcu for checking
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
2014-10-28 22:09 ` [PATCH tip/core/rcu 2/9] compiler: Allow 1- and 2-byte smp_load_acquire() and smp_store_release() Paul E. McKenney
2014-10-28 22:09 ` [PATCH tip/core/rcu 3/9] drivers/md: Use rcu_dereference() for accessing rcu pointer Paul E. McKenney
@ 2014-10-28 22:09 ` Paul E. McKenney
2014-10-28 22:09 ` [PATCH tip/core/rcu 5/9] rcu: Add sparse check for RCU_INIT_POINTER() Paul E. McKenney
` (4 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:09 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: Pranith Kumar <bobby.prani@gmail.com>
Annotate the map field with __rcu since this is a rcu pointer which is checked
by sparse.
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
drivers/md/dm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index e7399362722e..3372b8378830 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -140,7 +140,7 @@ struct mapped_device {
* Use dm_get_live_table{_fast} or take suspend_lock for
* dereference.
*/
- struct dm_table *map;
+ struct dm_table __rcu *map;
struct list_head table_devices;
struct mutex table_devices_lock;
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH tip/core/rcu 5/9] rcu: Add sparse check for RCU_INIT_POINTER()
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
` (2 preceding siblings ...)
2014-10-28 22:09 ` [PATCH tip/core/rcu 4/9] dm: sparse: Annotate field with __rcu for checking Paul E. McKenney
@ 2014-10-28 22:09 ` Paul E. McKenney
2014-10-28 22:09 ` [PATCH tip/core/rcu 6/9] rcu: Optimize cond_resched_rcu_qs() Paul E. McKenney
` (3 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:09 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: Pranith Kumar <bobby.prani@gmail.com>
Add a sparse check when RCU_INIT_POINTER() is used to assign a non __rcu
annotated pointer.
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index a4a819ffb2d1..a033d8b55773 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1047,6 +1047,7 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
*/
#define RCU_INIT_POINTER(p, v) \
do { \
+ rcu_dereference_sparse(p, __rcu); \
p = RCU_INITIALIZER(v); \
} while (0)
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH tip/core/rcu 6/9] rcu: Optimize cond_resched_rcu_qs()
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
` (3 preceding siblings ...)
2014-10-28 22:09 ` [PATCH tip/core/rcu 5/9] rcu: Add sparse check for RCU_INIT_POINTER() Paul E. McKenney
@ 2014-10-28 22:09 ` Paul E. McKenney
2014-10-28 22:10 ` [PATCH tip/core/rcu 7/9] rcu: More info about potential deadlocks with rcu_read_unlock() Paul E. McKenney
` (2 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:09 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
The current implementation of cond_resched_rcu_qs() can invoke
rcu_note_voluntary_context_switch() twice in the should_resched()
case, once via the call to __schedule() and once directly. However, as
noted by Joe Lawrence in a patch to the team subsystem, cond_resched()
returns an indication as to whether or not the call to __schedule()
actually happened. This commit therefore changes cond_resched_rcu_qs()
so as to invoke rcu_note_voluntary_context_switch() only when __schedule()
was not called.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index a033d8b55773..36ea3ba5c516 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -348,8 +348,8 @@ extern struct srcu_struct tasks_rcu_exit_srcu;
*/
#define cond_resched_rcu_qs() \
do { \
- rcu_note_voluntary_context_switch(current); \
- cond_resched(); \
+ if (!cond_resched()) \
+ rcu_note_voluntary_context_switch(current); \
} while (0)
#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_RCU_TRACE) || defined(CONFIG_SMP)
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH tip/core/rcu 7/9] rcu: More info about potential deadlocks with rcu_read_unlock()
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
` (4 preceding siblings ...)
2014-10-28 22:09 ` [PATCH tip/core/rcu 6/9] rcu: Optimize cond_resched_rcu_qs() Paul E. McKenney
@ 2014-10-28 22:10 ` Paul E. McKenney
2014-10-28 22:10 ` [PATCH tip/core/rcu 8/9] rcu: Fix FIXME in rcu_tasks_kthread() Paul E. McKenney
2014-10-28 22:10 ` [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations Paul E. McKenney
7 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:10 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: Oleg Nesterov <oleg@redhat.com>
The comment above rcu_read_unlock() explains the potential deadlock
if the caller holds one of the locks taken by rt_mutex_unlock() paths,
but it is not clear from this documentation that any lock which can
be taken from interrupt can lead to deadlock as well and we need to
take rt_mutex_lock() into account too.
The problem is that rt_mutex_lock() takes wait_lock without disabling
irqs, and thus an interrupt taking some LOCK can obviously race with
rcu_read_unlock_special() called with the same LOCK held.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 36ea3ba5c516..ae6942a84a0d 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -887,7 +887,9 @@ static inline void rcu_read_lock(void)
* Unfortunately, this function acquires the scheduler's runqueue and
* priority-inheritance spinlocks. This means that deadlock could result
* if the caller of rcu_read_unlock() already holds one of these locks or
- * any lock that is ever acquired while holding them.
+ * any lock that is ever acquired while holding them; or any lock which
+ * can be taken from interrupt context because rcu_boost()->rt_mutex_lock()
+ * does not disable irqs while taking ->wait_lock.
*
* That said, RCU readers are never priority boosted unless they were
* preempted. Therefore, one way to avoid deadlock is to make sure
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH tip/core/rcu 8/9] rcu: Fix FIXME in rcu_tasks_kthread()
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
` (5 preceding siblings ...)
2014-10-28 22:10 ` [PATCH tip/core/rcu 7/9] rcu: More info about potential deadlocks with rcu_read_unlock() Paul E. McKenney
@ 2014-10-28 22:10 ` Paul E. McKenney
2014-10-28 22:10 ` [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations Paul E. McKenney
7 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:10 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
This commit affines rcu_tasks_kthread() to the housekeeping CPUs
in CONFIG_NO_HZ_FULL builds. This is just a default, so systems
administrators are free to put this kthread somewhere else if they wish.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
kernel/rcu/update.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 3ef8ba58694e..8a39e68ff8e0 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -531,7 +531,8 @@ static int __noreturn rcu_tasks_kthread(void *arg)
struct rcu_head *next;
LIST_HEAD(rcu_tasks_holdouts);
- /* FIXME: Add housekeeping affinity. */
+ /* Run on housekeeping CPUs by default. Sysadm can move if desired. */
+ housekeeping_affine(current);
/*
* Each pass through the following loop makes one check for
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations
2014-10-28 22:09 ` [PATCH tip/core/rcu 1/9] rcu: Remove CONFIG_RCU_CPU_STALL_VERBOSE Paul E. McKenney
` (6 preceding siblings ...)
2014-10-28 22:10 ` [PATCH tip/core/rcu 8/9] rcu: Fix FIXME in rcu_tasks_kthread() Paul E. McKenney
@ 2014-10-28 22:10 ` Paul E. McKenney
2014-10-29 10:57 ` Peter Zijlstra
7 siblings, 1 reply; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-28 22:10 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, tglx,
peterz, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani, Paul E. McKenney
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Although rcu_dereference() and friends can be used in situations where
object lifetimes are being managed by something other than RCU, the
resulting sparse and lockdep-RCU noise can be annoying. This commit
therefore supplies a lockless_dereference(), which provides the
protection for dereferences without the RCU-related debugging noise.
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index ae6942a84a0d..423fd0478f8a 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -617,6 +617,21 @@ static inline void rcu_preempt_sleep_check(void)
#define RCU_INITIALIZER(v) (typeof(*(v)) __force __rcu *)(v)
/**
+ * lockless_dereference() - safely load a pointer for later dereference
+ * @p: The pointer to load
+ *
+ * Similar to rcu_dereference(), but for situations where the pointed-to
+ * object's lifetime is managed by something other than RCU. That
+ * "something other" might be reference counting or simple immortality.
+ */
+#define lockless_dereference(p) \
+({ \
+ typeof(p) _________p1 = ACCESS_ONCE(p); \
+ smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
+ (_________p1); \
+})
+
+/**
* rcu_assign_pointer() - assign to RCU-protected pointer
* @p: pointer to assign to
* @v: value to assign (publish)
--
1.8.1.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations
2014-10-28 22:10 ` [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations Paul E. McKenney
@ 2014-10-29 10:57 ` Peter Zijlstra
2014-10-29 12:42 ` Paul E. McKenney
0 siblings, 1 reply; 25+ messages in thread
From: Peter Zijlstra @ 2014-10-29 10:57 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, tglx, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani
On Tue, Oct 28, 2014 at 03:10:02PM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>
> Although rcu_dereference() and friends can be used in situations where
> object lifetimes are being managed by something other than RCU, the
> resulting sparse and lockdep-RCU noise can be annoying. This commit
> therefore supplies a lockless_dereference(), which provides the
> protection for dereferences without the RCU-related debugging noise.
>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
> +#define lockless_dereference(p) \
> +({ \
> + typeof(p) _________p1 = ACCESS_ONCE(p); \
> + smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
> + (_________p1); \
> +})
Should we not have at least a single user along with this?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations
2014-10-29 10:57 ` Peter Zijlstra
@ 2014-10-29 12:42 ` Paul E. McKenney
2014-10-29 19:15 ` Oleg Nesterov
0 siblings, 1 reply; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-29 12:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, tglx, rostedt, dhowells, edumazet, dvhart, fweisbec, oleg,
bobby.prani
On Wed, Oct 29, 2014 at 11:57:04AM +0100, Peter Zijlstra wrote:
> On Tue, Oct 28, 2014 at 03:10:02PM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >
> > Although rcu_dereference() and friends can be used in situations where
> > object lifetimes are being managed by something other than RCU, the
> > resulting sparse and lockdep-RCU noise can be annoying. This commit
> > therefore supplies a lockless_dereference(), which provides the
> > protection for dereferences without the RCU-related debugging noise.
> >
> > Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
>
> > +#define lockless_dereference(p) \
> > +({ \
> > + typeof(p) _________p1 = ACCESS_ONCE(p); \
> > + smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
> > + (_________p1); \
> > +})
>
> Should we not have at least a single user along with this?
And we do. In fact, Al Viro has pulled this into his vfs.git tree and
so I will be dropping this patch in favor of his.
Thanx, Paul
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations
2014-10-29 12:42 ` Paul E. McKenney
@ 2014-10-29 19:15 ` Oleg Nesterov
2014-10-29 18:43 ` Paul E. McKenney
0 siblings, 1 reply; 25+ messages in thread
From: Oleg Nesterov @ 2014-10-29 19:15 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
dvhart, fweisbec, bobby.prani
On 10/29, Paul E. McKenney wrote:
>
> On Wed, Oct 29, 2014 at 11:57:04AM +0100, Peter Zijlstra wrote:
> > On Tue, Oct 28, 2014 at 03:10:02PM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > >
> > > Although rcu_dereference() and friends can be used in situations where
> > > object lifetimes are being managed by something other than RCU, the
> > > resulting sparse and lockdep-RCU noise can be annoying. This commit
> > > therefore supplies a lockless_dereference(), which provides the
> > > protection for dereferences without the RCU-related debugging noise.
> > >
> > > Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> >
> > > +#define lockless_dereference(p) \
> > > +({ \
> > > + typeof(p) _________p1 = ACCESS_ONCE(p); \
> > > + smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
> > > + (_________p1); \
> > > +})
> >
> > Should we not have at least a single user along with this?
>
> And we do. In fact, Al Viro has pulled this into his vfs.git tree and
> so I will be dropping this patch in favor of his.
And it seems that most of smp_read_barrier_depends() users can be changed
to use this helper.
Oleg.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH tip/core/rcu 9/9] rcu: Provide counterpart to rcu_dereference() for non-RCU situations
2014-10-29 19:15 ` Oleg Nesterov
@ 2014-10-29 18:43 ` Paul E. McKenney
0 siblings, 0 replies; 25+ messages in thread
From: Paul E. McKenney @ 2014-10-29 18:43 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
dvhart, fweisbec, bobby.prani
On Wed, Oct 29, 2014 at 08:15:19PM +0100, Oleg Nesterov wrote:
> On 10/29, Paul E. McKenney wrote:
> >
> > On Wed, Oct 29, 2014 at 11:57:04AM +0100, Peter Zijlstra wrote:
> > > On Tue, Oct 28, 2014 at 03:10:02PM -0700, Paul E. McKenney wrote:
> > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > >
> > > > Although rcu_dereference() and friends can be used in situations where
> > > > object lifetimes are being managed by something other than RCU, the
> > > > resulting sparse and lockdep-RCU noise can be annoying. This commit
> > > > therefore supplies a lockless_dereference(), which provides the
> > > > protection for dereferences without the RCU-related debugging noise.
> > > >
> > > > Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > ---
> > >
> > > > +#define lockless_dereference(p) \
> > > > +({ \
> > > > + typeof(p) _________p1 = ACCESS_ONCE(p); \
> > > > + smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
> > > > + (_________p1); \
> > > > +})
> > >
> > > Should we not have at least a single user along with this?
> >
> > And we do. In fact, Al Viro has pulled this into his vfs.git tree and
> > so I will be dropping this patch in favor of his.
>
> And it seems that most of smp_read_barrier_depends() users can be changed
> to use this helper.
Good point! I guess I should have done this some time ago. ;-)
Thanx, Paul
^ permalink raw reply [flat|nested] 25+ messages in thread