RCU-protected list updates for 4.2
Subsystems (3)
cover RCU-protected list updates for 4.2
Hello!
This series contains a few updates to RCU-protected lists.
1. Add RCU annotation to suppress sparse warnings in
hlist_for_each_entry_from_rcu(), courtesy of Ying Xue.
2. Fix list_entry_rcu to read ptr with rcu_dereference_raw() to
save a load instruction, courtesy of Patrick Marlier.
3. Fix list_entry_rcu() usage in next_active_rdev(), courtesy
of Patrick Marlier.
4. Fix list_entry_rcu() usage in nf_hook_slow(), courtesy of
Patrick Marlier.
Thanx, Paul
------------------------------------------------------------------------
b/drivers/md/bitmap.c | 2 +- b/include/linux/rculist.h | 9 +++------ b/net/netfilter/core.c | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-)
PATCH 1/4 rculist: Fix another sparse warning
From: Ying Xue <[email protected]>
This fixes the following sparse warnings:
make C=1 CF=-D__CHECK_ENDIAN__ net/tipc/name_table.onet/tipc/name_table.c:977:17: error: incompatible types in comparison expression (different address spaces) net/tipc/name_table.c:977:17: error: incompatible types in comparison expression (different address spaces)
To silence these spare complaints, an RCU annotation should be added to "next" pointer of hlist_node structure through hlist_next_rcu() macro when iterating over a hlist with hlist_for_each_entry_from_rcu().
--- include/linux/rculist.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/rculist.h +2 -2
@@ -549,8 +549,8 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,*/ #define hlist_for_each_entry_from_rcu (pos, member) \ for (; pos; \ - pos = hlist_entry_safe(rcu_dereference((pos)->member.next),\ - typeof(*(pos)), member)) + pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ + &(pos)->member)), typeof(*(pos)), member))#endif /* __KERNEL__ */ #endif
Signature · 1 line
1.8.1.5
PATCH 2/4 rculist: Fix list_entry_rcu to read ptr with rcu_dereference_raw
From: Patrick Marlier <[email protected]>
Change to read effectively ptr with rcu_dereference_raw and not the __ptr variable on the stack.
--- include/linux/rculist.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
include/linux/rculist.h +1 -4
@@ -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
1.8.1.5
PATCH 3/4 md/bitmap: Fix list_entry_rcu usage
In reply to Paul E. McKenneyFrom: Patrick Marlier <[email protected]>
--- drivers/md/bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/bitmap.c +1 -1
@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_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
1.8.1.5
Steven Rostedt
reviewerRe: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 13 lines · On Tue, 12 May 2015 15:46:26 -0700 "Paul E. McKenney"…
From: Patrick Marlier <[email protected]>
Signed-off-by Patrick Marlier <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- drivers/md/bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/md/bitmap.c +0
-0
drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 184
Quoted 5 lines
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_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);
Hmm, this changes the semantics.
The original code looks nasty, I first thought it was broken, but it seems to work out of sheer luck (or clever hack)
drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 187
Quoted 3 lines
drivers/md/bitmap.c +0
-0else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);
What comes after this is:
list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) {
if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);
Where &mddev->disks would return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshift md_rdev struct. But it isn't used, as the list_for_each_entry_continue_rcu() has:
#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
Thus the first use of pos is pos->member.next or:
mddev->disks.next
But now you converted it to rdev = mddev->disks.next, which means the first use is:
pos = mddev->disks.next->next
I think you are skipping the first element here.
-- Steve
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 24 lines · On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 13 lines · On Tue, 12 May 2015 15:46:26 -0700 "Paul E. McKenney"…
From: Patrick Marlier <[email protected]>
Signed-off-by Patrick Marlier <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- drivers/md/bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/md/bitmap.c +0
-0drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 184
Quoted 5 lines
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_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);Hmm, this changes the semantics.
Quoted 2 lines
The original code looks nasty, I first thought it was broken, but it seems to work out of sheer luck (or clever hack)
Definitely a clever hack - no question of "luck" here :-)
It might makes sense to change it to use list_for_each_entry_from_rcu()
if (rdev == NULL)
rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set);
else {
rdev_dec_pending(rdev, mddev);
rdev = list_next_entry_rcu(rdev->same_set.next, struct md_rdev, same_set);
}
list_for_each_entry_from_rcu(rdev, ....)but there isn't a "list_next_entry_rcu"....
Also, it would have been polity to at least 'cc' them Maintainer of this code in the original patch - no?
Thanks,
NeilBrown
Quoted 37 lines
drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 187
Quoted 4 lines
drivers/md/bitmap.c +0
-0else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);What comes after this is:
list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);Where
&mddev->diskswould return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshiftmd_rdevstruct. But it isn't used, as thelist_for_each_entry_continue_rcu()has:#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))Thus the first use of pos is
pos->member.nextor:
mddev->disks.nextBut now you converted it to rdev =
mddev->disks.next, which means the first use is:pos =
mddev->disks.next->nextI think you are skipping the first element here.
-- Steve
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 41 lines · NeilBrown
Quoted 24 lines · On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 13 lines · On Tue, 12 May 2015 15:46:26 -0700 "Paul E. McKenney"…
From: Patrick Marlier <[email protected]>
Signed-off-by Patrick Marlier <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- drivers/md/bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/md/bitmap.c +0
-0drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 184
Quoted 5 lines
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_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);Hmm, this changes the semantics.
Quoted 2 lines
The original code looks nasty, I first thought it was broken, but it seems to work out of sheer luck (or clever hack)
Definitely a clever hack - no question of "luck" here :-)
It might makes sense to change it to use
list_for_each_entry_from_rcu()if (rdev == NULL) rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set); else { rdev_dec_pending(rdev, mddev); rdev = list_next_entry_rcu(rdev->same_set.next, struct md_rdev, same_set); } list_for_each_entry_from_rcu(rdev, ....)
Quoted 1 line
but there isn't a "
list_next_entry_rcu"....
Patrick, this one is for you. As are all of the questions from Steven.
Quoted 2 lines
Also, it would have been polity to at least 'cc' them Maintainer of this code in the original patch - no?
Rest assured that this set is not going to -tip without at least an Acked-by from at least one of the maintainers. In some cases, I will interpret silence as assent, but this is not one of those cases. ;-)
Thanx, Paul
Quoted 41 lines
Thanks,
NeilBrownQuoted 37 lines
drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 187
Quoted 4 lines
drivers/md/bitmap.c +0
-0else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);What comes after this is:
list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);Where
&mddev->diskswould return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshiftmd_rdevstruct. But it isn't used, as thelist_for_each_entry_continue_rcu()has:#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))Thus the first use of pos is
pos->member.nextor:
mddev->disks.nextBut now you converted it to rdev =
mddev->disks.next, which means the first use is:pos =
mddev->disks.next->nextI think you are skipping the first element here.
-- Steve
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
In reply to NeilBrownQuoted 41 lines · NeilBrown
Quoted 24 lines · On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 13 lines · On Tue, 12 May 2015 15:46:26 -0700 "Paul E. McKenney"…
From: Patrick Marlier <[email protected]>
Signed-off-by Patrick Marlier <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- drivers/md/bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/md/bitmap.c +0
-0drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 184
Quoted 5 lines
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_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);Hmm, this changes the semantics.
Quoted 2 lines
The original code looks nasty, I first thought it was broken, but it seems to work out of sheer luck (or clever hack)
Definitely a clever hack - no question of "luck" here :-)
It might makes sense to change it to use
list_for_each_entry_from_rcu()if (rdev == NULL) rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set); else { rdev_dec_pending(rdev, mddev); rdev = list_next_entry_rcu(rdev->same_set.next, struct md_rdev, same_set); } list_for_each_entry_from_rcu(rdev, ....)
Quoted 5 lines
but there isn't a "
list_next_entry_rcu"....Also, it would have been polity to at least 'cc' them Maintainer of this code in the original patch - no?
Sure my bad. I hesitated to CC maintainers. I was almost sure that it will be rejected so I wanted to avoid noise.
Quoted 39 lines
Thanks,
NeilBrownQuoted 35 lines
drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 187
Quoted 4 lines
drivers/md/bitmap.c +0
-0else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);What comes after this is:
list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);Where
&mddev->diskswould return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshiftmd_rdevstruct. But it isn't used, as thelist_for_each_entry_continue_rcu()has:#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))Thus the first use of pos is
pos->member.nextor:mddev->disks.nextBut now you converted it to rdev =
mddev->disks.next, which means the first use is:pos = mddev->disks.next->nextI think you are skipping the first element here.
struct mddev {
...
struct list_head disks;
...}
struct list_head {
struct list_head *next, *prev;
};The tricky thing is that "list_entry_rcu" before and after the patch is reading the same thing.
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
PS: In the rculist patch I proposed, I avoid the store and the atomic reload in the stack variable __ptr. (yeap, the rcu_dereference_raw/ACCESS_ONCE is a bit confusing because it implicitly do & on the parameter).
Thanks.
--
Pat
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 44 lines · On Sat, 16 May 2015 19:42:54 +0200 Patrick Marlier <[email protected]>…
Quoted 41 lines · NeilBrown
Quoted 24 lines · On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 13 lines · On Tue, 12 May 2015 15:46:26 -0700 "Paul E. McKenney"…
From: Patrick Marlier <[email protected]>
Signed-off-by Patrick Marlier <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- drivers/md/bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/md/bitmap.c +0
-0drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 184
Quoted 5 lines
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_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);Hmm, this changes the semantics.
Quoted 2 lines
The original code looks nasty, I first thought it was broken, but it seems to work out of sheer luck (or clever hack)
Definitely a clever hack - no question of "luck" here :-)
It might makes sense to change it to use
list_for_each_entry_from_rcu()if (rdev == NULL) rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set); else { rdev_dec_pending(rdev, mddev); rdev = list_next_entry_rcu(rdev->same_set.next, struct md_rdev, same_set); } list_for_each_entry_from_rcu(rdev, ....)
Quoted 8 lines
Quoted 5 lines
but there isn't a "
list_next_entry_rcu"....Also, it would have been polity to at least 'cc' them Maintainer of this code in the original patch - no?
Sure my bad. I hesitated to CC maintainers. I was almost sure that it will be rejected so I wanted to avoid noise.
Well... If the subject has contained the magic string "RFC" I might have been less concerned.
But there have been enough times that people have changed md without telling me, and thereby broken it, that I'd much rather see the patch than not.
Quoted 46 lines
Quoted 41 lines
Thanks,
NeilBrownQuoted 35 lines
drivers/md/bitmap.c static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde · line 187
Quoted 4 lines
drivers/md/bitmap.c +0
-0else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);What comes after this is:
list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);Where
&mddev->diskswould return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshiftmd_rdevstruct. But it isn't used, as thelist_for_each_entry_continue_rcu()has:#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))Thus the first use of pos is
pos->member.nextor:mddev->disks.nextBut now you converted it to rdev =
mddev->disks.next, which means the first use is:pos = mddev->disks.next->nextI think you are skipping the first element here.
struct mddev {
...struct list_head disks;
Quoted 8 lines
...}
struct list_head { struct list_head *next, *prev; };The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.
No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.
Quoted 3 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Thanks,
NeilBrown
Quoted 14 lines
PS: In the rculist patch I proposed, I avoid the store and the atomic reload in the stack variable
__ptr. (yeap, the rcu_dereference_raw/ACCESS_ONCE is a bit confusing because it implicitly do & on the parameter).Thanks.
--
Pat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Steven Rostedt
reviewerRe: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 9 lines · On Mon, 18 May 2015 12:06:47 +1000 NeilBrown <[email protected]> wrote:
Quoted 9 lines
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };
Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.
Right.
Quoted 11 lines
Quoted 5 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Probably some debugging tool like sparse notices that the assignment isn't a true list entry and complains about it. In other words, I think the real fix is to fix the debugging tool to ignore this, because the code is correct, and this is a false positive failure, and is causing more harm than good, because people are sending out broken patches due to it.
-- Steve
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 34 lines · Steven Rostedt
Quoted 9 lines · On Mon, 18 May 2015 12:06:47 +1000 NeilBrown <[email protected]> wrote:
Quoted 9 lines
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.Right.
Quoted 11 lines
Quoted 5 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Quoted 6 lines
Probably some debugging tool like sparse notices that the assignment isn't a true list entry and complains about it. In other words, I think the real fix is to fix the debugging tool to ignore this, because the code is correct, and this is a false positive failure, and is causing more harm than good, because people are sending out broken patches due to it.
OK, finally did the history trawling that I should have done to begin with.
Back in 2010, Arnd added the __rcu pointer checking in sparse. But the RCU list primitives were used on non-RCU-protected lists, so some casting pain was required to avoid sparse complaints. (Keep in mind that the list_head structure does not mark ->next with __rcu.) Arnd's workaround was to copy the pointer to the local stack, casting it to an __rcu pointer, then use rcu_dereference_raw() to do the needed traversal of an RCU-protected pointer.
This of course resulted in an extraneous load from the stack, which Patrick noticed in his performance work, and which motivated him to send the patches.
Perhaps what I should do is create an rcu_dereference_nocheck() for use in list traversals, that omits the sparse checking. That should get rid of both the sparse warnings and the strange casts.
The code in md probably needs to change in any case, as otherwise we are invoking rcu_dereference_whatever() on a full struct list_head rather than on a single pointer. Or am I missing something here?
Thanx, Paul
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 3 lines · On Tue, 19 May 2015 15:07:25 -0700 "Paul E. McKenney"…
The code in md probably needs to change in any case, as otherwise we are invoking
rcu_dereference_whatever()on a full structlist_headrather than on a single pointer. Or am I missing something here?
I think it would be
rcu_dereference_whatever(&mddev->disks)
I don't know what you mean by "on a full struct list_head", but there is nothing actually being dereferenced here - right? Just pointer arithmetic on 'mddev'.
I should probably just
drivers/md/bitmap.c +1 -1
@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_read_lock ();if (rdev ==NULL )/* start at the beginning */ - rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set); + rdev = list_entry(&mddev->disks, struct md_rdev, same_set);else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);
as there really are no RCU issues with getting that address. Maybe I should move it outside the rcu_read_lock() just to be blatant.... but that would make the code a lot more clumsy as the rdev_dec_pending must be inside the rcu_read_lock..
So this.
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Wed, 20 May 2015 15:05:09 +1000
Subject: [PATCH] md/bitmap: remove rcu annotation from pointer arithmetic.
Evaluating "&mddev->disks" is simple pointer arithmetic, so it does not need 'rcu' annotations - no dereferencing is happening.
Also enhance the comment to explain that 'rdev' in that case is not actually a pointer to an rdev.
drivers/md/bitmap.c +6 -1
@@ -177,11 +177,16 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde* nr_pending is 0 and In_sync is clear, the entries we return will * still be in the same position on the list when we re-enter * list_for_each_entry_continue_rcu. +* +* Note that if entered with 'rdev == NULL' to start at the +* beginning, we temporarily assign 'rdev' to an address which +* isn't really an rdev, but which can be used by +* list_for_each_entry_continue_rcu() to find the first entry. */ rcu_read_lock ();if (rdev ==NULL )/* start at the beginning */ - rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set); + rdev = list_entry(&mddev->disks, struct md_rdev, same_set);else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 8 lines · NeilBrown
Quoted 3 lines · On Tue, 19 May 2015 15:07:25 -0700 "Paul E. McKenney"…
The code in md probably needs to change in any case, as otherwise we are invoking
rcu_dereference_whatever()on a full structlist_headrather than on a single pointer. Or am I missing something here?
Quoted 6 lines
I think it would be
rcu_dereference_whatever(&mddev->disks)I don't know what you mean by "on a full struct
list_head", but there is nothing actually being dereferenced here - right? Just pointer arithmetic on 'mddev'.
It really does dereference. Strange but true.
Quoted 16 lines
I should probably just
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_read_lock ();if (rdev ==NULL )/* start at the beginning */ - rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set); + rdev = list_entry(&mddev->disks, struct md_rdev, same_set);else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);
Quoted 6 lines
as there really are no RCU issues with getting that address. Maybe I should move it outside the
rcu_read_lock()just to be blatant.... but that would make the code a lot more clumsy as therdev_dec_pendingmust be inside thercu_read_lock..So this.
Fair enough -- if you aren't using RCU, there is really no point in using the RCU API. I will drop this patch from my tree. You are pushing yours, I am guessing?
Thanx, Paul
Quoted 38 lines
Thanks,
NeilBrownFrom: NeilBrown <[email protected]>
Date: Wed, 20 May 2015 15:05:09 +1000
Subject: [PATCH] md/bitmap: remove rcu annotation from pointer arithmetic.Evaluating "
&mddev->disks" is simple pointer arithmetic, so it does not need 'rcu' annotations - no dereferencing is happening.Also enhance the comment to explain that 'rdev' in that case is not actually a pointer to an rdev.
Reported-by Patrick Marlier <[email protected]>Signed-off-by NeilBrown <[email protected]>drivers/md/bitmap.c +6
-1@@ -177,11 +177,16 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde* nr_pending is 0 and In_sync is clear, the entries we return will * still be in the same position on the list when we re-enter * list_for_each_entry_continue_rcu. +* +* Note that if entered with 'rdev == NULL' to start at the +* beginning, we temporarily assign 'rdev' to an address which +* isn't really an rdev, but which can be used by +* list_for_each_entry_continue_rcu() to find the first entry. */ rcu_read_lock ();if (rdev ==NULL )/* start at the beginning */ - rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set); + rdev = list_entry(&mddev->disks, struct md_rdev, same_set);else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 9 lines · On Wed, 20 May 2015 06:28:35 -0700 "Paul E. McKenney"…
Quoted 8 lines · NeilBrown
Quoted 3 lines · On Tue, 19 May 2015 15:07:25 -0700 "Paul E. McKenney"…
The code in md probably needs to change in any case, as otherwise we are invoking
rcu_dereference_whatever()on a full structlist_headrather than on a single pointer. Or am I missing something here?
Quoted 8 lines
Quoted 6 lines
I think it would be
rcu_dereference_whatever(&mddev->disks)I don't know what you mean by "on a full struct
list_head", but there is nothing actually being dereferenced here - right? Just pointer arithmetic on 'mddev'.It really does dereference. Strange but true.
Well... your the expert. But without an lvalue, I can't see it.
Quoted 22 lines
Quoted 22 lines
I should probably just
drivers/md/bitmap.c +1
-1@@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mddercu_read_lock ();if (rdev ==NULL )/* start at the beginning */ - rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set); + rdev = list_entry(&mddev->disks, struct md_rdev, same_set);else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);as there really are no RCU issues with getting that address. Maybe I should move it outside the
rcu_read_lock()just to be blatant.... but that would make the code a lot more clumsy as therdev_dec_pendingmust be inside thercu_read_lock..
Quoted 5 lines
Quoted 1 line
So this.
Fair enough -- if you aren't using RCU, there is really no point in using the RCU API. I will drop this patch from my tree. You are pushing yours, I am guessing?
Excellent guess :-)
Hopefully for the next -rc.
Thanks,
NeilBrown
Quoted 42 lines
Thanx, Paul
Quoted 38 lines
Thanks,
NeilBrownFrom: NeilBrown <[email protected]>
Date: Wed, 20 May 2015 15:05:09 +1000
Subject: [PATCH] md/bitmap: remove rcu annotation from pointer arithmetic.Evaluating "
&mddev->disks" is simple pointer arithmetic, so it does not need 'rcu' annotations - no dereferencing is happening.Also enhance the comment to explain that 'rdev' in that case is not actually a pointer to an rdev.
Reported-by Patrick Marlier <[email protected]>Signed-off-by NeilBrown <[email protected]>drivers/md/bitmap.c +6
-1@@ -177,11 +177,16 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde* nr_pending is 0 and In_sync is clear, the entries we return will * still be in the same position on the list when we re-enter * list_for_each_entry_continue_rcu. +* +* Note that if entered with 'rdev == NULL' to start at the +* beginning, we temporarily assign 'rdev' to an address which +* isn't really an rdev, but which can be used by +* list_for_each_entry_continue_rcu() to find the first entry. */ rcu_read_lock ();if (rdev ==NULL )/* start at the beginning */ - rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set); + rdev = list_entry(&mddev->disks, struct md_rdev, same_set);else {/* release the previous rdev and start from there. */ rdev_dec_pending (rdev, mddev);
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
In reply to Paul E. McKenneyQuoted 56 lines · Paul E. McKenney
Quoted 34 lines · Steven Rostedt
Quoted 9 lines · On Mon, 18 May 2015 12:06:47 +1000 NeilBrown <[email protected]> wrote:
Quoted 9 lines
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.Right.
Quoted 11 lines
Quoted 5 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Quoted 6 lines
Probably some debugging tool like sparse notices that the assignment isn't a true list entry and complains about it. In other words, I think the real fix is to fix the debugging tool to ignore this, because the code is correct, and this is a false positive failure, and is causing more harm than good, because people are sending out broken patches due to it.
OK, finally did the history trawling that I should have done to begin with.
Back in 2010, Arnd added the
__rcupointer checking in sparse. But the RCU list primitives were used on non-RCU-protected lists, so some casting pain was required to avoid sparse complaints. (Keep in mind that thelist_headstructure does not mark ->next with__rcu.) Arnd's workaround was to copy the pointer to the local stack, casting it to an__rcupointer, then usercu_dereference_raw()to do the needed traversal of an RCU-protected pointer.This of course resulted in an extraneous load from the stack, which Patrick noticed in his performance work, and which motivated him to send the patches.
Quoted 7 lines
Perhaps what I should do is create an
rcu_dereference_nocheck()for use in list traversals, that omits the sparse checking. That should get rid of both the sparse warnings and the strange casts.The code in md probably needs to change in any case, as otherwise we are invoking
rcu_dereference_whatever()on a full structlist_headrather than on a single pointer. Or am I missing something here?
Finally getting back to this one...
I switched to lockless_dereference() instead of rcu_dereference_raw(), and am running it through the testing gamut. Patrick, are you OK with this change?
Thanx, Paul
------------------------------------------------------------------------
rculist: Make list_entry_rcu() use lockless_dereference()
The current list_entry_rcu() implementation copies the pointer to a stack variable, then invokes rcu_dereference_raw() on it. This results in an additional store-load pair. Now, most compilers will emit normal store and load instructions, which might seem to be of negligible overhead, but this results in a load-hit-store situation that can cause surprisingly long pipeline stalls, even on modern microprocessors. The problem is that it takes time for the store to get the store buffer updated, which can delay the subsequent load, which immediately follows.
This commit therefore switches to the lockless_dereference() primitive, which does not expect the __rcu annotations (that are anyway not present in the list_head structure) and which, like rcu_dereference_raw(), does not check for an enclosing RCU read-side critical section. Most importantly, it does not copy the pointer, thus avoiding the load-hit-store overhead.
include/linux/rculist.h +1 -4
@@ -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(lockless_dereference(ptr), type, member)/** * Where are list_empty_rcu() and list_first_entry_rcu()?
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 65 lines · Paul E. McKenney
Quoted 56 lines · Paul E. McKenney
Quoted 34 lines · Steven Rostedt
Quoted 9 lines · On Mon, 18 May 2015 12:06:47 +1000 NeilBrown <[email protected]> wrote:
Quoted 9 lines
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.Right.
Quoted 11 lines
Quoted 5 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Quoted 6 lines
Probably some debugging tool like sparse notices that the assignment isn't a true list entry and complains about it. In other words, I think the real fix is to fix the debugging tool to ignore this, because the code is correct, and this is a false positive failure, and is causing more harm than good, because people are sending out broken patches due to it.
OK, finally did the history trawling that I should have done to begin with.
Back in 2010, Arnd added the
__rcupointer checking in sparse. But the RCU list primitives were used on non-RCU-protected lists, so some casting pain was required to avoid sparse complaints. (Keep in mind that thelist_headstructure does not mark ->next with__rcu.) Arnd's workaround was to copy the pointer to the local stack, casting it to an__rcupointer, then usercu_dereference_raw()to do the needed traversal of an RCU-protected pointer.This of course resulted in an extraneous load from the stack, which Patrick noticed in his performance work, and which motivated him to send the patches.
Quoted 7 lines
Perhaps what I should do is create an
rcu_dereference_nocheck()for use in list traversals, that omits the sparse checking. That should get rid of both the sparse warnings and the strange casts.The code in md probably needs to change in any case, as otherwise we are invoking
rcu_dereference_whatever()on a full structlist_headrather than on a single pointer. Or am I missing something here?
Quoted 5 lines
Finally getting back to this one...
I switched to
lockless_dereference()instead ofrcu_dereference_raw(), and am running it through the testing gamut. Patrick, are you OK with this change?
Paul,
This sounds good to me. It should fix the performance issue (will check with my benchmark).
I think for drivers/md/bitmap.c:next_active_rdev() the problem was fixed but do you know if it also fixed for net/netfilter/core.c:nf_hook_slow()?
Thanks.
--
Pat
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 69 lines · Patrick Marlier
Quoted 67 lines · Paul E. McKenney
Quoted 56 lines · Paul E. McKenney
Quoted 34 lines · Steven Rostedt
Quoted 9 lines · On Mon, 18 May 2015 12:06:47 +1000 NeilBrown <[email protected]> wrote:
Quoted 9 lines
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.Right.
Quoted 11 lines
Quoted 5 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Quoted 6 lines
Probably some debugging tool like sparse notices that the assignment isn't a true list entry and complains about it. In other words, I think the real fix is to fix the debugging tool to ignore this, because the code is correct, and this is a false positive failure, and is causing more harm than good, because people are sending out broken patches due to it.
OK, finally did the history trawling that I should have done to begin with.
Back in 2010, Arnd added the
__rcupointer checking in sparse. But the RCU list primitives were used on non-RCU-protected lists, so some casting pain was required to avoid sparse complaints. (Keep in mind that thelist_headstructure does not mark ->next with__rcu.) Arnd's workaround was to copy the pointer to the local stack, casting it to an__rcupointer, then usercu_dereference_raw()to do the needed traversal of an RCU-protected pointer.This of course resulted in an extraneous load from the stack, which Patrick noticed in his performance work, and which motivated him to send the patches.
Quoted 7 lines
Perhaps what I should do is create an
rcu_dereference_nocheck()for use in list traversals, that omits the sparse checking. That should get rid of both the sparse warnings and the strange casts.The code in md probably needs to change in any case, as otherwise we are invoking
rcu_dereference_whatever()on a full structlist_headrather than on a single pointer. Or am I missing something here?Finally getting back to this one...
Quoted 8 lines
Quoted 3 lines
I switched to
lockless_dereference()instead ofrcu_dereference_raw(), and am running it through the testing gamut. Patrick, are you OK with this change?Paul,
This sounds good to me. It should fix the performance issue (will check with my benchmark).
Thank you, looking forward to seeing the results!
Quoted 3 lines
I think for drivers/md/bitmap.c:next_active_rdev() the problem was fixed but do you know if it also fixed for net/netfilter/core.c:nf_hook_slow()?
It does appear so. The statement now reads:
elem = list_entry_rcu(state->hook_list, struct nf_hook_ops, list);
And ->hook_list is defined as follows:
struct list_head *hook_list; Thanx, Paul
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 1 line · Paul E. McKenney
On Sun, Sep 13, 2015 at 12:06:33PM +0200, Patrick Marlier wrote:
[ . . . ]
Quoted 6 lines
Quoted 4 lines
Paul,
This sounds good to me. It should fix the performance issue (will check with my benchmark).
Thank you, looking forward to seeing the results!
Just following up -- how is the benchmarking going?
Thanx, Paul
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 5 lines · Paul E. McKenney
Quoted 1 line · Paul E. McKenney
On Sun, Sep 13, 2015 at 12:06:33PM +0200, Patrick Marlier wrote:
[ . . . ]
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Paul,
This sounds good to me. It should fix the performance issue (will check with my benchmark).
Thank you, looking forward to seeing the results!
Just following up -- how is the benchmarking going?
Note that in my module I am using the kernel version 3.16.0-31 (I ported your change). Here the results of my benchmark that tests rculist in the case of read only.
# 1st column : The number of threads
# 2nd : ops/s the original version
# 3rd : ops/s your version with lockless_dereference 1 883923 1747554
2 1741441 3543062
3 2462360 5103647
4 3437273 7176608
6 5155803 9812348
8 6718111 13330050
10 8519227 17458294
12 9773632 20298897
14 11555198 23191424
16 11643264 25125712
I get same performance with my patch.
So indeed it fixes the performance problem I was seeing.
Thanks a lot!
--
Pat
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 35 lines · Patrick Marlier
Quoted 5 lines · Paul E. McKenney
Quoted 1 line · Paul E. McKenney
On Sun, Sep 13, 2015 at 12:06:33PM +0200, Patrick Marlier wrote:
[ . . . ]
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Paul,
This sounds good to me. It should fix the performance issue (will check with my benchmark).
Thank you, looking forward to seeing the results!
Just following up -- how is the benchmarking going?
Note that in my module I am using the kernel version 3.16.0-31 (I ported your change).
Here the results of my benchmark that tests rculist in the case of read only.# 1st column : The number of threads
# 2nd : ops/s the original version
# 3rd : ops/s your version withlockless_dereference1 883923 1747554
2 1741441 3543062
3 2462360 5103647
4 3437273 7176608
6 5155803 9812348
8 6718111 13330050
10 8519227 17458294
12 9773632 20298897
14 11555198 23191424
16 11643264 25125712
Quoted 2 lines
I get same performance with my patch.
So indeed it fixes the performance problem I was seeing.
Nice, thank you!!!
Thanx, Paul
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
In reply to NeilBrownQuoted 46 lines · NeilBrown
Quoted 44 lines · On Sat, 16 May 2015 19:42:54 +0200 Patrick Marlier <[email protected]>…
Quoted 24 lines · NeilBrown
Quoted 23 lines · On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <[email protected]> wrote:
On Tue, 12 May 2015 15:46:26 -0700
"Paul E. McKenney" <[email protected]> wrote:
What comes after this is:list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);Where
&mddev->diskswould return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshiftmd_rdevstruct. But it isn't used, as thelist_for_each_entry_continue_rcu()has:#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))Thus the first use of pos is
pos->member.nextor:Quoted 8 lines
Quoted 8 lines
mddev->disks.nextBut now you converted it to rdev =
mddev->disks.next, which means the first use is:pos = mddev->disks.next->nextI think you are skipping the first element here.
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };
Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.
Here I meant "list_entry_rcu" (in include/linux/rculist.h) not the change to drivers/md/bitmap.c.
Quoted 7 lines
Quoted 2 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Do not apply the patch on drivers/md/bitmap.c but only on include/linux/rculist.h and you will see that the compilation fails.
--
Pat
Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage
Quoted 61 lines · Patrick Marlier
Quoted 46 lines · NeilBrown
Quoted 44 lines · On Sat, 16 May 2015 19:42:54 +0200 Patrick Marlier <[email protected]>…
Quoted 24 lines · NeilBrown
Quoted 23 lines · On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <[email protected]> wrote:
On Tue, 12 May 2015 15:46:26 -0700
"Paul E. McKenney" <[email protected]> wrote:
What comes after this is:list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { if (rdev->raid_disk >= 0 &&Now the original code had:
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);Where
&mddev->diskswould return the address of the disks field of mddev which is a list head. Then it would get the 'same_set' offset, which is 0, and rdev is pointing to a makeshiftmd_rdevstruct. But it isn't used, as thelist_for_each_entry_continue_rcu()has:#define list_for_each_entry_continue_rcu(pos, head, member) \ for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \ &pos->member != (head); \ pos = list_entry_rcu(pos->member.next, typeof(*pos), member))Thus the first use of pos is
pos->member.nextor:Quoted 8 lines
Quoted 8 lines
mddev->disks.nextBut now you converted it to rdev =
mddev->disks.next, which means the first use is:pos = mddev->disks.next->nextI think you are skipping the first element here.
struct mddev {
...struct list_head disks;...}
struct list_head { struct list_head *next, *prev; };Quoted 6 lines
Quoted 2 lines
The tricky thing is that "
list_entry_rcu" before and after the patch is reading the same thing.No it isn't.
Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field.Here I meant "
list_entry_rcu" (ininclude/linux/rculist.h) not the change todrivers/md/bitmap.c.Quoted 3 lines
Quoted 2 lines
However in your case, the change I proposed is probably wrong I trust you on this side. :) What's your proposal to fix it with the rculist patch?
Quoted 7 lines
Quoted 4 lines
What needs fixing? I don't see anything broken.
Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it?
Do not apply the patch on
drivers/md/bitmap.cbut only oninclude/linux/rculist.hand you will see that the compilation fails.
Here an example of the compilation error in drivers/md/bitmap.c
In file included from include/linux/sched.h:17:0,
from include/linux/blkdev.h:4,
from drivers/md/bitmap.c:18:drivers/md/bitmap.c: In function ‘next_active_rdev’:include/linux/compiler.h:469:24: error: lvalue required as unary ‘&’ operand
(volatile typeof(x) *)&(x); })
^
include/linux/kernel.h:800:49: note: in definition of macro ‘container_of’
const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
include/linux/compiler.h:470:26: note: in expansion of macro ‘__ACCESS_ONCE’
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
^
include/linux/rcupdate.h:630:26: note: in expansion of macro ‘ACCESS_ONCE’
typeof(p) _________p1 = ACCESS_ONCE(p); \
^
include/linux/rcupdate.h:587:48: note: in expansion of macro ‘lockless_dereference’
typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
^include/linux/rcupdate.h:723:2: note: in expansion of macro ‘__rcu_dereference_check’
__rcu_dereference_check((p), rcu_read_lock_held() || (c), __rcu)
^include/linux/rcupdate.h:746:32: note: in expansion of macro ‘rcu_dereference_check’
#define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@
needed? @@@*/
^
include/linux/rculist.h:251:28: note: in expansion of macro ‘rcu_dereference_raw’
container_of((typeof(ptr))rcu_dereference_raw(ptr), type, member); \
^
drivers/md/bitmap.c:184:10: note: in expansion of macro ‘list_entry_rcu’
rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);
^Thanks.
--
Pat
PATCH 4/4 netfilter: Fix list_entry_rcu usage
In reply to Paul E. McKenneyFrom: Patrick Marlier <[email protected]>
--- net/netfilter/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/core.c +1 -1
@@ -166,7 +166,7 @@ int nf_hook_slow(struct sk_buff *skb, struct nf_hook_state *state)/* We may already have this, but read-locks nest anyway */ rcu_read_lock (); - elem =list_entry_rcu (&nf_hooks[state->pf][state->hook], + elem =list_entry_rcu (nf_hooks[state->pf][state->hook].next, struct nf_hook_ops, list); next_hook: verdict = nf_iterate(&nf_hooks[state->pf][state->hook], skb, state,
Signature · 1 line
1.8.1.5
Steven Rostedt
reviewerRe: [PATCH tip/core/rcu 4/4] netfilter: Fix list_entry_rcu usage
Quoted 13 lines · On Tue, 12 May 2015 15:46:27 -0700 "Paul E. McKenney"…
From: Patrick Marlier <[email protected]>
Signed-off-by Patrick Marlier <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- net/netfilter/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/netfilter/core.c +0
-0
net/netfilter/core.c int nf_hook_slow(struct sk_buff *skb, struct nf_hook_state *state) · line 170
Quoted 6 lines
net/netfilter/core.c +1
-1@@ -166,7 +166,7 @@ int nf_hook_slow(struct sk_buff *skb, struct nf_hook_state *state)/* We may already have this, but read-locks nest anyway */ rcu_read_lock (); - elem =list_entry_rcu (&nf_hooks[state->pf][state->hook], + elem =list_entry_rcu (nf_hooks[state->pf][state->hook].next, struct nf_hook_ops, list);
Looks to be setting to the next element, not the current one.
-- Steve
Quoted 2 lines
net/netfilter/core.c +0
-0next_hook: verdict = nf_iterate(&nf_hooks[state->pf][state->hook], skb, state,