Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 28 May 2013 13:10:46 +0400 | From | Roman Gushchin <> | Subject | Re: [PATCH v2] rcu: fix a race in hlist_nulls_for_each_entry_rcu macro |
| |
On 28.05.2013 04:12, Eric Dumazet wrote: > On Mon, 2013-05-27 at 21:55 +0400, Roman Gushchin wrote: >> Hi, Paul! >> >>> On 25.05.2013 15:37, Paul E. McKenney wrote: >>>> Again, I believe that your retry logic needs to extend back into the >>>> calling function for your some_func() example above. >> >> And what do you think about the following approach (diff below)? >> >> It seems to me, it's enough clear (especially with good accompanying comments) >> and produces a good binary code (without significant overhead). >> Also, we will remove a hidden reef in using rcu-protected (h)list traverses with restarts. >> > >> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h >> index 2ae1371..4af5ee5 100644 >> --- a/include/linux/rculist_nulls.h >> +++ b/include/linux/rculist_nulls.h >> @@ -107,7 +107,8 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n, >> * >> */ >> #define hlist_nulls_for_each_entry_rcu(tpos, pos, head, member) \ >> - for (pos = rcu_dereference_raw(hlist_nulls_first_rcu(head)); \ >> + for (ACCESS_ONCE(*(head)), \ >> + pos = rcu_dereference_raw(hlist_nulls_first_rcu(head)); \ >> (!is_a_nulls(pos)) && \ >> ({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1; }); \ >> pos = rcu_dereference_raw(hlist_nulls_next_rcu(pos))) > > It looks like this still relies on gcc being friendly here. > > I repeat again : @head here is a constant.
No. Actually, there are two volatile objects: pointer to the first element (as a part of the head structure), and the first element by itself. So, to be strict, head as a structure contains a volatile field. Head->first should be treated as a volatile pointer to a volatile data. So, the whole head object is volatile.
> > Macro already uses ACCESS_ONCE(), we only have to instruct gcc that > caching the value is forbidden if we restart the loop > (aka "goto begin;" see Documentation/RCU/rculist_nulls.txt line 146)
My patch seems to be correct, because, ACCESS_ONCE(*(head)) will cause gcc to (re)read head data from the memory. According to gcc documentation: "A scalar volatile object is read when it is accessed in a void context: volatile int *src = somevalue; *src; Such expressions are rvalues, and GCC implements this as a read of the volatile object being pointed to." And this is exactly our case.
> Adding a barrier() is probably what we want.
I agree, inserting barrier() is also a correct and working fix.
Thanks!
Regards, Roman
| ![\](/images/icornerr.gif) |