hlist_add_tail_rcu disable sparse warning
RFC PATCH 1/1 hlist_add_tail_rcu disable sparse warning
sparse is unhappy about this code in hlist_add_tail_rcu:
struct hlist_node *i, *last = NULL;
for (i = hlist_first_rcu(h); i; i = hlist_next_rcu(i))
last = i;This is because hlist_next_rcu and hlist_next_rcu return __rcu pointers.
The following trivial patch disables the warning without changing the behaviour in any way.
---
I would appreciate review to confirm the function doesn't do anything unsafe though.
In particular, should this use __hlist_for_each_rcu instead? I note that __hlist_for_each_rcu does rcu_dereference internally, which is missing here.
compile-tested only.
include/linux/rculist.h +1 -1
@@ -511,7 +511,7 @@ static inline void hlist_add_tail_rcu(struct hlist_node *n, {struct hlist_node *i, *last =NULL ; -for (i =hlist_first_rcu (h); i; i =hlist_next_rcu (i)) +for (i = h->first; i; i = i->next) last = i;if (last) {
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 23 Nov 2016 22:48:19 +0200
Quoted 6 lines
I would appreciate review to confirm the function doesn't do anything unsafe though.
In particular, should this use
__hlist_for_each_rcuinstead? I note that__hlist_for_each_rcudoesrcu_dereferenceinternally, which is missing here.
I personally think it should use __hlist_for_each_rcu, otherwise nothing expresses the rcu-ness of the operation.
Quoted 6 lines · David Miller
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 23 Nov 2016 22:48:19 +0200Quoted 3 lines
I would appreciate review to confirm the function doesn't do anything unsafe though.
Quoted 6 lines
Quoted 3 lines
In particular, should this use
__hlist_for_each_rcuinstead? I note that__hlist_for_each_rcudoesrcu_dereferenceinternally, which is missing here.I personally think it should use
__hlist_for_each_rcu, otherwise nothing expresses the rcu-ness of the operation.
I like Dave's suggestion. Michael, does that change work for you?
Thanx, Paul
Quoted 11 lines · Paul E. McKenney
Quoted 10 lines · David Miller
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 23 Nov 2016 22:48:19 +0200Quoted 6 lines
I would appreciate review to confirm the function doesn't do anything unsafe though.
In particular, should this use
__hlist_for_each_rcuinstead? I note that__hlist_for_each_rcudoesrcu_dereferenceinternally, which is missing here.
Quoted 6 lines
Quoted 2 lines
I personally think it should use
__hlist_for_each_rcu, otherwise nothing expresses the rcu-ness of the operation.I like Dave's suggestion. Michael, does that change work for you?
Thanx, Paul
I think it does, I'll try and post the patch.
Signature · 1 line
MST
Quoted 11 lines · Paul E. McKenney
Quoted 10 lines · David Miller
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 23 Nov 2016 22:48:19 +0200Quoted 6 lines
I would appreciate review to confirm the function doesn't do anything unsafe though.
In particular, should this use
__hlist_for_each_rcuinstead? I note that__hlist_for_each_rcudoesrcu_dereferenceinternally, which is missing here.
Quoted 6 lines
Quoted 2 lines
I personally think it should use
__hlist_for_each_rcu, otherwise nothing expresses the rcu-ness of the operation.I like Dave's suggestion. Michael, does that change work for you?
Thanx, Paul
I think we should change __hlist_for_each_rcu to skip rcu_dereference though then, since it would be called outside any rcu read size critical sections here.
Signature · 1 line
MST
Quoted 6 lines · David Miller
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 23 Nov 2016 22:48:19 +0200Quoted 3 lines
I would appreciate review to confirm the function doesn't do anything unsafe though.
Quoted 6 lines
Quoted 3 lines
In particular, should this use
__hlist_for_each_rcuinstead? I note that__hlist_for_each_rcudoesrcu_dereferenceinternally, which is missing here.I personally think it should use
__hlist_for_each_rcu, otherwise nothing expresses the rcu-ness of the operation.
What does "rcu-ness" mean in this context?
The question is not just about making the code pretty.
This operation is called outside any rcu critical section. If you are going to call __hlist_for_each_rcu which calls rcu_dereference, you should do it inside
a critical section.
Other operations such as hlist_add_behind_rcu manipulate lists manually, maybe this one should, too? Paul?
Signature · 1 line
MST
Quoted 7 lines · Michael S. Tsirkin
sparse is unhappy about this code in
hlist_add_tail_rcu:struct hlist_node *i, *last = NULL; for (i = hlist_first_rcu(h); i; i = hlist_next_rcu(i)) last = i;
Quoted 7 lines
This is because
hlist_next_rcuandhlist_next_rcureturn__rcupointers.The following trivial patch disables the warning without changing the behaviour in any way.
Signed-off-by Michael S. Tsirkin <[email protected]>
So after reviewing this, there's no rcu-ness involved except when we assign the next pointer on the last item. This is because as the following comment says
* The caller must take whatever precautions are necessary
* (such as holding appropriate locks) to avoid racing
* with another list-mutation primitive, such as hlist_add_head_rcu()
* or hlist_del_rcu(), running on this same list.
I conclude this patch is actually the right thing to do, by comparison, __hlist_for_each_rcu suggested by Dave Miller would be confusing since it's designed to run in the rcu read side critical section.
I'll repost as non-RFC unless I hear otherwise.
Quoted 24 lines
---
I would appreciate review to confirm the function doesn't do anything unsafe though.
In particular, should this use
__hlist_for_each_rcuinstead? I note that__hlist_for_each_rcudoesrcu_dereferenceinternally, which is missing here.compile-tested only.
include/linux/rculist.h +1
-1@@ -511,7 +511,7 @@ static inline void hlist_add_tail_rcu(struct hlist_node *n, {struct hlist_node *i, *last =NULL ; -for (i =hlist_first_rcu (h); i; i =hlist_next_rcu (i)) +for (i = h->first; i; i = i->next) last = i;if (last) {