On Wed, Nov 23, 2016 at 10:48:19PM +0200, Michael S. Tsirkin wrote: > 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. > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
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. > --- > > 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. > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > index 8beb98d..33574db 100644 > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -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) {