Does hlist_for_each_entry_rcu need additional pointer to be passed into it?

Question

LWN gives the following example on RCU:

Subscribing to an RCU-protected hlist is also similar to the circular list:
  1 rcu_read_lock();
  2 hlist_for_each_entry_rcu(p, q, head, list) {
  3   do_something_with(p->a, p->b, p->c);
  4 }
  5 rcu_read_unlock();
Quick Quiz 3: Why do we need to pass two pointers into hlist_for_each_entry_rcu() when only one is needed for list_for_each_entry_rcu()?

Answer: Because in an hlist it is necessary to check for NULL rather than for encountering the head. (Try coding up a single-pointer hlist_for_each_entry_rcu(). If you come up with a nice solution, it would be a very good thing!)

I presume it must refer to an old version of hlist_for_each_entry_rcu() since current version (3.13.0) in rculist.h header actually presents the definition of hlist_for_each_entry_rcu taking 3 arguments, therefore eliminating the need for additional 4th pointer and doesn't seem to be very difficult to invent:

#define hlist_for_each_entry_rcu(pos, head, member)         \
    for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
            typeof(*(pos)), member);            \
        pos;                            \
        pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
            &(pos)->member)), typeof(*(pos)), member))

Am I missing something or above version given in current rculist.h is such a very good thing?

We can see something subtle is going on in __rcu_dereference_check where additional pointer is created:

#define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/


#define rcu_dereference_check(p, c) \
    __rcu_dereference_check((p), rcu_read_lock_held() || (c), __rcu)

#define __rcu_dereference_check(p, c, space) \
    ({ \
        typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
        rcu_lockdep_assert(c, "suspicious rcu_dereference_check()" \
                      " usage"); \
        rcu_dereference_sparse(p, space); \
        smp_read_barrier_depends(); \
        ((typeof(*p) __force __kernel *)(_________p1)); \

Should this question be on Code Review or similar, to discuss? http://codereview.stackexchange.com/ — Weather Vane, Aug 27 '15 at 21:02
The change looks like just the pointer to a pointer trick for avoiding a sentinel node or special cases for singly linked lists, but I'm not sure about the RCU considerations. — David Eisenstat, Aug 27 '15 at 21:49

score 2 · Answer 1 · edited May 23 '17 at 11:48

I wondered the same thing! Digging into the source, it seems like the the four argument version was replace by a three argument version somewhere between 3.8 (https://github.com/torvalds/linux/blob/v3.8/include/linux/rculist.h#L457) and 3.9 (https://github.com/torvalds/linux/blob/v3.9/include/linux/rculist.h#L456)

To compare the two macros, here's the older four argument version:

#define hlist_for_each_entry_rcu(tpos, pos, head, member)       \
    for (pos = rcu_dereference_raw(hlist_first_rcu(head));      \
        pos &&                           \
        ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
        pos = rcu_dereference_raw(hlist_next_rcu(pos)))

And here's the new three-argument version:

#define hlist_for_each_entry_rcu(pos, head, member)         \
    for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
        typeof(*(pos)), member);            \
        pos;                            \
        pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
            &(pos)->member)), typeof(*(pos)), member))

So it seems like the key difference is the hlist_entry_safe macro, which is basically ptr? ptr->member : NULL.

This is not obviously possible because hlist_entry_safe is a macro, so ptr might be an expression, and that expression should not be evaluated more than once. Ex, the obvious solution – #define hlist_entry_safe(ptr, member) ((ptr)? (ptr)->member : NULL) – will not work, because (ptr) will be evaluated twice.

Based on this answer, I assume the syntax that's being used in 3.9 – … ({ typeof(ptr) ____ptr = (ptr); … }) – is a GCC-only extension, which might explain why it wasn't possible at the time the article was written.

Does hlist_for_each_entry_rcu need additional pointer to be passed into it?

1 Answers1