> On Tue, 2008-07-15 at 11:22 -0400, Mathieu Desnoyers wrote:
> > * Peter Zijlstra (
peterz@infradead.org) wrote:
> > >
> > > I'm confused by the barrier games here.
> > >
> > > Why not:
> > >
> > > void **it_func;
> > >
> > > preempt_disable();
> > > it_func = rcu_dereference((tp)->funcs);
> > > if (it_func) {
> > > for (; *it_func; it_func++)
> > > ((void(*)(proto))(*it_func))(args);
> > > }
> > > preempt_enable();
> > >
> > > That is, why can we skip the barrier when !it_func? is that because at
> > > that time we don't actually dereference it_func and therefore cannot
> > > observe stale data?
> > >
> >
> > Exactly. I used the implementation of rcu_assign_pointer as a hint that
> > we did not need barriers when setting the pointer to NULL, and thus we
> > should not need the read barrier when reading the NULL pointer, because
> > it references no data.
> >
> > #define rcu_assign_pointer(p, v) \
> > ({ \
> > if (!__builtin_constant_p(v) || \
> > ((v) != NULL)) \
> > smp_wmb(); \
> > (p) = (v); \
> > })
>
> Yeah, I saw that,.. made me wonder. It basically assumes that when we
> write:
>
> rcu_assign_pointer(foo, NULL);
>
> foo will not be used as an index or offset.
>
> I guess Paul has thought it through and verified all in-kernel use
> cases, but it still makes me feel unconfortable.
>
> > #define rcu_dereference(p) ({ \
> > typeof(p) _________p1 = ACCESS_ONCE(p); \
> > smp_read_barrier_depends(); \
> > (_________p1); \
> > })
> >
> > But I think you are right, since we are already in unlikely code, using
> > rcu_dereference as you do is better than my use of read barrier depends.
> > It should not change anything in the assembly result except on alpha,
> > where the read_barrier_depends() is not a nop.
> >
> > I wonder if there would be a way to add this kind of NULL pointer case
> > check without overhead in rcu_dereference() on alpha. I guess not, since
> > the pointer is almost never known at compile-time. And I guess Paul must
> > already have thought about it. The only case where we could add this
> > test is when we know that we have a if (ptr != NULL) test following the
> > rcu_dereference(); we could then assume the compiler will merge the two
> > branches since they depend on the same condition.
>
> I remember seeing a thread about all this special casing NULL, but have
> never been able to find it again - my google skillz always fail me.
>
> Basically it doesn't work if you use the variable as an index/offset,
> because in that case 0 is a valid offset and you still generate a data
> dependency.
>
> IIRC the conclusion was that the gains were too small to spend more time
> on it, although I would like to hear about the special case in
> rcu_assign_pointer.
>
> /me goes use git blame....
>
