rculist: Fix list_entry_rcu to read ptr with rcu_dereference_raw

PATCH In review Patrick Marlier 2 participants 7 messages Mar 24, 2015 – Apr 14, 2015

Subsystems (3)

Networking netfilter RCU

lore

Share & tools

lore

Patrick Marlier

author

2015-03-24 10:31 UTC #0 lore raw

PATCH 1/3 rculist: Fix list_entry_rcu to read ptr with rcu_dereference_raw

Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.

Signed-off-by Patrick Marlier <[email protected]>

---
 include/linux/rculist.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

include/linux/rculist.h +0 -0

index a18b16f..9d9baea 100644
@@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by

rcu_read_lock().
*/
#define list_entry_rcu(ptr, type, member) \
-({ \
- typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
- container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
-})
+ container_of((typeof(ptr))rcu_dereference_raw(ptr), type, member)

/**
* Where are list_empty_rcu() and list_first_entry_rcu()?

Signature · 1 line

2.1.0

Paul E. McKenney

2015-03-25 14:30 UTC #1 lore raw

Quoted 4 lines · Patrick Marlier

Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>

Avoiding an extra load could be worthwhile in a number of situations, agreed.

However, won't this change cause sparse to complain if invoked on a non-RCU-protected pointer? The ability to use list-RCU API members on both RCU and non-RCU pointers was one of the points of the previous commit, right?

Thanx, Paul

Quoted 25 lines

---
 include/linux/rculist.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

include/linux/rculist.h +0 -0

index a18b16f..9d9baea 100644
@@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct
 list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by

rcu_read_lock().
*/
#define list_entry_rcu(ptr, type, member) \

include/linux/rculist.h +1 -4

-({ \
-   typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
-   container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
-})
+   container_of((typeof(ptr))rcu_dereference_raw(ptr), type, member)
 
 /**
  * Where are list_empty_rcu() and list_first_entry_rcu()?

Signature · 1 line

2.1.0

Patrick Marlier

author

2015-03-25 15:01 UTC #2 lore raw

Quoted 7 lines · Paul E. McKenney

Quoted 4 lines · Patrick Marlier
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>
Avoiding an extra load could be worthwhile in a number of situations, agreed.

Not only a load. It adds a store and a load on the stack and I think this creates a dependency in the processor pipeline.

Quoted 4 lines

However, won't this change cause sparse to complain if invoked on a non-RCU-protected pointer? The ability to use list-RCU API members on both RCU and non-RCU pointers was one of the points of the previous commit, right?

Probably we can put back the cast but I am not familiar enough with the RCU API.

Also, the problem here is that you probably want ACCESS_ONCE to happen on the content of 'ptr' and not on the stack variable '__ptr'.

(you have to follow this chain: rcu_dereference_raw -> rcu_dereference_check -> __rcu_dereference_check -> lockless_dereference -> ACCESS_ONCE)

#define lockless_dereference(p) \

({ \
    typeof(p) _________p1 = ACCESS_ONCE(p); \
    smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
    (_________p1); \
})

#define __ACCESS_ONCE(x) ({ \
     __maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
    (volatile typeof(x) *)&(x); })

#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

Note that ACCESS_ONCE is doing "&" on x.

IMHO, I would prefer saving some useless instructions for better performance rather than giving too much flexibility on the API (also pretty sure the cast can be still done). --
Patrick Marlier

Paul E. McKenney

2015-03-28 10:42 UTC #3 lore raw

Quoted 36 lines · Patrick Marlier

Quoted 7 lines · Paul E. McKenney
Quoted 4 lines · Patrick Marlier
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>
Avoiding an extra load could be worthwhile in a number of situations, agreed.
Not only a load. It adds a store and a load on the stack and I think this creates a dependency in the processor pipeline.
Quoted 4 lines
However, won't this change cause sparse to complain if invoked on a non-RCU-protected pointer? The ability to use list-RCU API members on both RCU and non-RCU pointers was one of the points of the previous commit, right?
Probably we can put back the cast but I am not familiar enough with the RCU API.
Also, the problem here is that you probably want ACCESS_ONCE to happen on the content of 'ptr' and not on the stack variable '__ptr'.
(you have to follow this chain: rcu_dereference_raw -> rcu_dereference_check -> __rcu_dereference_check -> lockless_dereference -> ACCESS_ONCE)
#define lockless_dereference(p) \
({ \
    typeof(p) _________p1 = ACCESS_ONCE(p); \
    smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
    (_________p1); \
})
#define __ACCESS_ONCE(x) ({ \
     __maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
    (volatile typeof(x) *)&(x); })

Quoted 7 lines

#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
Note that ACCESS_ONCE is doing "&" on x.
IMHO, I would prefer saving some useless instructions for better performance rather than giving too much flexibility on the API (also pretty sure the cast can be still done).

OK, what I am going to do is to apply your patches for testing purposes. If there are no complaints, they will likely go into v4.3 or thereabouts.

Thanx, Paul

Paul E. McKenney

2015-03-28 10:47 UTC #4 lore raw

Quoted 39 lines · Paul E. McKenney

Quoted 38 lines · Patrick Marlier
Quoted 7 lines · Paul E. McKenney
Quoted 4 lines · Patrick Marlier
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>
Avoiding an extra load could be worthwhile in a number of situations, agreed.
Not only a load. It adds a store and a load on the stack and I think this creates a dependency in the processor pipeline.
Quoted 4 lines
However, won't this change cause sparse to complain if invoked on a non-RCU-protected pointer? The ability to use list-RCU API members on both RCU and non-RCU pointers was one of the points of the previous commit, right?
Probably we can put back the cast but I am not familiar enough with the RCU API.
Also, the problem here is that you probably want ACCESS_ONCE to happen on the content of 'ptr' and not on the stack variable '__ptr'.
(you have to follow this chain: rcu_dereference_raw -> rcu_dereference_check -> __rcu_dereference_check -> lockless_dereference -> ACCESS_ONCE)
#define lockless_dereference(p) \
({ \
    typeof(p) _________p1 = ACCESS_ONCE(p); \
    smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
    (_________p1); \
})
#define __ACCESS_ONCE(x) ({ \
     __maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
    (volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

Quoted 8 lines

Quoted 5 lines
Note that ACCESS_ONCE is doing "&" on x.
IMHO, I would prefer saving some useless instructions for better performance rather than giving too much flexibility on the API (also pretty sure the cast can be still done).
OK, what I am going to do is to apply your patches for testing purposes. If there are no complaints, they will likely go into v4.3 or thereabouts.

Except that I hit conflicts. Could you please rebase to rcu/dev at
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git?

Thanx, Paul

Patrick Marlier

author

2015-04-13 11:39 UTC #5 lore raw

Strange I don't get any conflict.
Maybe due to my email client so I attached the patches to this email.

Thanks,
--
Patrick

Quoted 46 lines · Paul E. McKenney

Quoted 45 lines · Paul E. McKenney
Quoted 43 lines · Patrick Marlier
Quoted 7 lines · Paul E. McKenney
Quoted 4 lines · Patrick Marlier
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>
Avoiding an extra load could be worthwhile in a number of situations, agreed.
Not only a load. It adds a store and a load on the stack and I think this creates a dependency in the processor pipeline.
Quoted 4 lines
However, won't this change cause sparse to complain if invoked on a non-RCU-protected pointer? The ability to use list-RCU API members on both RCU and non-RCU pointers was one of the points of the previous commit, right?
Probably we can put back the cast but I am not familiar enough with the RCU API.
Also, the problem here is that you probably want ACCESS_ONCE to happen on the content of 'ptr' and not on the stack variable '__ptr'.
(you have to follow this chain: rcu_dereference_raw -> rcu_dereference_check -> __rcu_dereference_check -> lockless_dereference -> ACCESS_ONCE)
#define lockless_dereference(p) \
({ \
    typeof(p) _________p1 = ACCESS_ONCE(p); \
    smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
    (_________p1); \
})
#define __ACCESS_ONCE(x) ({ \
     __maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
    (volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
Note that ACCESS_ONCE is doing "&" on x.
IMHO, I would prefer saving some useless instructions for better performance rather than giving too much flexibility on the API (also pretty sure the cast can be still done).

Quoted 8 lines

Quoted 2 lines
OK, what I am going to do is to apply your patches for testing purposes. If there are no complaints, they will likely go into v4.3 or thereabouts.
Except that I hit conflicts. Could you please rebase to rcu/dev at
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git?
Thanx, Paul

From 8ac818d418068105623e43bbd289d9553c182e6c Mon Sep 17 00:00:00 2001
From: Patrick Marlier <[email protected]>
Date: Tue, 24 Mar 2015 11:16:55 +0100
Subject: [PATCH 1/3] rculist: Fix list_entry_rcu to read ptr with
rcu_dereference_raw

Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.

Signed-off-by Patrick Marlier <[email protected]>

---
 include/linux/rculist.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

include/linux/rculist.h +1 -4

index a18b16f..9d9baea 100644
@@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
  */
 #define list_entry_rcu(ptr, type, member) \
-({ \
-	typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
-	container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
-})
+	container_of((typeof(ptr))rcu_dereference_raw(ptr), type, member)
 
 /**
  * Where are list_empty_rcu() and list_first_entry_rcu()?

Signature · 1 line

2.1.0

From 3ab5f342939f768b693708bd32ef3f350af3b5a6 Mon Sep 17 00:00:00 2001
From: Patrick Marlier <[email protected]>
Date: Tue, 24 Mar 2015 11:21:05 +0100
Subject: [PATCH 2/3] netfilter: fix list_entry_rcu usage.

Signed-off-by Patrick Marlier <[email protected]>

---
 net/netfilter/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/netfilter/core.c +1 -1

index fea9ef5..05bd311 100644
@@ -174,7 +174,7 @@ int nf_hook_slow(u_int8_t pf, unsigned int hook, struct sk_buff *skb,
 	/* We may already have this, but read-locks nest anyway */
 	rcu_read_lock();
 
-	elem = list_entry_rcu(&nf_hooks[pf][hook], struct nf_hook_ops, list);
+	elem = list_entry_rcu(nf_hooks[pf][hook].next, struct nf_hook_ops, list);
 next_hook:
 	verdict = nf_iterate(&nf_hooks[pf][hook], skb, hook, indev,
 			     outdev, &elem, okfn, hook_thresh);

Signature · 1 line

2.1.0

From 84f0428e2c9172692aba727636a643efb6994752 Mon Sep 17 00:00:00 2001
From: Patrick Marlier <[email protected]>
Date: Tue, 24 Mar 2015 11:22:10 +0100
Subject: [PATCH 3/3] md/bitmap: fix list_entry_rcu usage.

Signed-off-by Patrick Marlier <[email protected]>

---
 drivers/md/bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/bitmap.c +1 -1

index 3a57679..ed00e46 100644
@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde
 	rcu_read_lock();
 	if (rdev == NULL)
 		/* start at the beginning */
-		rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);
+		rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set);
 	else {
 		/* release the previous rdev and start from there. */
 		rdev_dec_pending(rdev, mddev);

Signature · 1 line

2.1.0

Paul E. McKenney

2015-04-14 20:34 UTC #6 lore raw

Quoted 2 lines · Patrick Marlier

Strange I don't get any conflict.
Maybe due to my email client so I attached the patches to this email.

That did the trick, thank you! I of course pass on the ritual advice to check your email client settings.

I have queued these for 4.2, and will let you know if any problems show up during testing.

Thanx, Paul

Quoted 148 lines

Thanks,
--
Patrick
Quoted 46 lines · Paul E. McKenney
Quoted 45 lines · Paul E. McKenney
Quoted 43 lines · Patrick Marlier
Quoted 7 lines · Paul E. McKenney
Quoted 4 lines · Patrick Marlier
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>
Avoiding an extra load could be worthwhile in a number of situations, agreed.
Not only a load. It adds a store and a load on the stack and I think this creates a dependency in the processor pipeline.
Quoted 4 lines
However, won't this change cause sparse to complain if invoked on a non-RCU-protected pointer? The ability to use list-RCU API members on both RCU and non-RCU pointers was one of the points of the previous commit, right?
Probably we can put back the cast but I am not familiar enough with the RCU API.
Also, the problem here is that you probably want ACCESS_ONCE to happen on the content of 'ptr' and not on the stack variable '__ptr'.
(you have to follow this chain: rcu_dereference_raw -> rcu_dereference_check -> __rcu_dereference_check -> lockless_dereference -> ACCESS_ONCE)
#define lockless_dereference(p) \
({ \
    typeof(p) _________p1 = ACCESS_ONCE(p); \
    smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
    (_________p1); \
})
#define __ACCESS_ONCE(x) ({ \
     __maybe_unused typeof(x) __var = (__force typeof(x)) 0; \
    (volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
Note that ACCESS_ONCE is doing "&" on x.
IMHO, I would prefer saving some useless instructions for better performance rather than giving too much flexibility on the API (also pretty sure the cast can be still done).
Quoted 8 lines
Quoted 2 lines
OK, what I am going to do is to apply your patches for testing purposes. If there are no complaints, they will likely go into v4.3 or thereabouts.
Except that I hit conflicts. Could you please rebase to rcu/dev at
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git?
Thanx, Paul
From 8ac818d418068105623e43bbd289d9553c182e6c Mon Sep 17 00:00:00 2001
From: Patrick Marlier <[email protected]>
Date: Tue, 24 Mar 2015 11:16:55 +0100
Subject: [PATCH 1/3] rculist: Fix list_entry_rcu to read ptr with
rcu_dereference_raw
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
Signed-off-by Patrick Marlier <[email protected]>
---
 include/linux/rculist.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
include/linux/rculist.h +1 -4
index a18b16f..9d9baea 100644
@@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
  */
 #define list_entry_rcu(ptr, type, member) \
-({ \
-	typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
-	container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
-})
+	container_of((typeof(ptr))rcu_dereference_raw(ptr), type, member)
 
 /**
  * Where are list_empty_rcu() and list_first_entry_rcu()?
Signature · 12 lines
2.1.0


From 3ab5f342939f768b693708bd32ef3f350af3b5a6 Mon Sep 17 00:00:00 2001
From: Patrick Marlier <[email protected]>
Date: Tue, 24 Mar 2015 11:21:05 +0100
Subject: [PATCH 2/3] netfilter: fix list_entry_rcu usage.

Signed-off-by: Patrick Marlier <[email protected]>
---
 net/netfilter/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/core.c +1 -1
index fea9ef5..05bd311 100644
@@ -174,7 +174,7 @@ int nf_hook_slow(u_int8_t pf, unsigned int hook, struct sk_buff *skb,
 	/* We may already have this, but read-locks nest anyway */
 	rcu_read_lock();
 
-	elem = list_entry_rcu(&nf_hooks[pf][hook], struct nf_hook_ops, list);
+	elem = list_entry_rcu(nf_hooks[pf][hook].next, struct nf_hook_ops, list);
 next_hook:
 	verdict = nf_iterate(&nf_hooks[pf][hook], skb, hook, indev,
 			     outdev, &elem, okfn, hook_thresh);
Signature · 12 lines
2.1.0


From 84f0428e2c9172692aba727636a643efb6994752 Mon Sep 17 00:00:00 2001
From: Patrick Marlier <[email protected]>
Date: Tue, 24 Mar 2015 11:22:10 +0100
Subject: [PATCH 3/3] md/bitmap: fix list_entry_rcu usage.

Signed-off-by: Patrick Marlier <[email protected]>
---
 drivers/md/bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/bitmap.c +1 -1
index 3a57679..ed00e46 100644
@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde
 	rcu_read_lock();
 	if (rdev == NULL)
 		/* start at the beginning */
-		rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);
+		rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set);
 	else {
 		/* release the previous rdev and start from there. */
 		rdev_dec_pending(rdev, mddev);
Signature · 1 line
2.1.0