lkml.org 
[lkml]   [2008]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 01/15] Kernel Tracepoints
    On Tue, Jul 15, 2008 at 12:08:13PM -0400, Mathieu Desnoyers wrote:
    > * Peter Zijlstra (peterz@infradead.org) wrote:
    > > 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....
    > >
    >
    > Actually, we could probably do the following, which also adds an extra
    > coherency check about non-NULL pointer assumptions :
    >
    > #ifdef CONFIG_RCU_DEBUG /* this would be new */
    > #define DEBUG_RCU_BUG_ON(x) BUG_ON(x)
    > #else
    > #define DEBUG_RCU_BUG_ON(x)
    > #endif
    >
    > #define rcu_dereference(p) ({ \
    > typeof(p) _________p1 = ACCESS_ONCE(p); \
    > if (p != NULL) \
    > smp_read_barrier_depends(); \
    > (_________p1); \
    > })
    >
    > #define rcu_dereference_non_null(p) ({ \
    > typeof(p) _________p1 = ACCESS_ONCE(p); \
    > DEBUG_RCU_BUG_ON(p == NULL); \
    > smp_read_barrier_depends(); \
    > (_________p1); \
    > })

    The big question is "why"? smp_read_barrier_depends() is pretty
    lightweight, after all.

    Thanx, Paul

    > The use-case where rcu_dereference() would be used is when it is
    > followed by a null pointer check (grepping through the sources shows me
    > this is a very very common case). In rare cases, it is assumed that the
    > pointer is never NULL and it is used just after the rcu_dereference. It
    > those cases, the extra test could be saved on alpha by using
    > rcu_dereference_non_null(p), which would check the the pointer is indeed
    > never NULL under some debug kernel configuration.
    >
    > Does it make sense ?
    >
    > Mathieu
    >
    > > > > If so, does this really matter since we're already in an unlikely
    > > > > section? Again, if so, this deserves a comment ;-)
    > > > >
    > > > > [ still think those preempt_* calls should be called
    > > > > rcu_read_sched_lock() or such. ]
    > > > >
    > > > > Anyway, does this still generate better code?
    > > > >
    > > >
    > > > On x86_64 :
    > > >
    > > > 820: bf 01 00 00 00 mov $0x1,%edi
    > > > 825: e8 00 00 00 00 callq 82a <thread_return+0x136>
    > > > 82a: 48 8b 1d 00 00 00 00 mov 0x0(%rip),%rbx # 831 <thread_return+0x13d>
    > > > 831: 48 85 db test %rbx,%rbx
    > > > 834: 75 21 jne 857 <thread_return+0x163>
    > > > 836: eb 27 jmp 85f <thread_return+0x16b>
    > > > 838: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
    > > > 83f: 00
    > > > 840: 48 8b 95 68 ff ff ff mov -0x98(%rbp),%rdx
    > > > 847: 48 8b b5 60 ff ff ff mov -0xa0(%rbp),%rsi
    > > > 84e: 4c 89 e7 mov %r12,%rdi
    > > > 851: 48 83 c3 08 add $0x8,%rbx
    > > > 855: ff d0 callq *%rax
    > > > 857: 48 8b 03 mov (%rbx),%rax
    > > > 85a: 48 85 c0 test %rax,%rax
    > > > 85d: 75 e1 jne 840 <thread_return+0x14c>
    > > > 85f: bf 01 00 00 00 mov $0x1,%edi
    > > > 864:
    > > >
    > > > for 68 bytes.
    > > >
    > > > My original implementation was 77 bytes, so yes, we have a win.
    > >
    > > Ah, good good ! :-)
    > >
    >
    > --
    > Mathieu Desnoyers
    > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


    \
     
     \ /
      Last update: 2008-08-01 23:17    [W:0.031 / U:34.700 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site