Tracepoints v3 for linux-next

Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:

Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.

Quoted 6 lines

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 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

Quoted 36 lines

Quoted 28 lines
Quoted 8 lines
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
Quoted 5 lines
864:
for 68 bytes.
My original implementation was 77 bytes, so yes, we have a win.
Ah, good good ! :-)

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Peter Zijlstra

maintainer

2008-07-15 16:25 UTC #12 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 110 lines · Mathieu Desnoyers

Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.
Quoted 6 lines
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 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.

Quoted 1 line

Does it make sense ?

This would break the case where the dereferenced variable is used as an index/offset where 0 is a valid value and still generates data dependencies.

So if with your new version we do:

  i = rcu_dereference(foo);
  j = table[i];

which translates into:

  i = ACCESS_ONCE(foo);
  if (i)
    smp_read_barrier_depends();
  j = table[i];

which when i == 0, would fail to do the barrier and can thus cause j to be a wrong value.

Sadly I'll have to defer to Paul to explain exactly how that can happen
- I always get my head in a horrible twist with this case.

Mathieu Desnoyers

author

2008-07-15 16:51 UTC #13 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 129 lines · * Peter Zijlstra ([email protected]) wrote:

Quoted 110 lines · Mathieu Desnoyers
Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.
Quoted 6 lines
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 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.
Quoted 1 line
Does it make sense ?
This would break the case where the dereferenced variable is used as an index/offset where 0 is a valid value and still generates data dependencies.
So if with your new version we do:
  i = rcu_dereference(foo);
  j = table[i];
which translates into:
  i = ACCESS_ONCE(foo);
  if (i)
    smp_read_barrier_depends();
  j = table[i];

Quoted 6 lines

which when i == 0, would fail to do the barrier and can thus cause j to be a wrong value.
Sadly I'll have to defer to Paul to explain exactly how that can happen
- I always get my head in a horrible twist with this case.

I completely agree with you. However, given the current rcu_assign_pointer() implementation, we already have this problem. My proposal assumes the current rcu_assign_pointer() behavior is correct and that those are never ever used for index/offsets.

We could enforce this as a compile-time check with something along the lines of :

#define BUILD_BUG_ON_NOT_OFFSETABLE(x) (void)(x)[0]

And use it both in rcu_assign_pointer() and rcu_dereference(). It would check for any type passed to rcu_assign_pointer and rcu_dereference which is not either a pointer or an array.

Then if someone really want to shoot himself in the foot by casting a pointer to a long after the rcu_deref, that's his problem.

Hrm, looking at rcu_assign_pointer tells me that the ((v) != NULL) test should probably already complain if v is not a pointer. So my build test is probably unneeded.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Paul E. McKenney

2008-08-01 21:15 UTC #14 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 151 lines · Mathieu Desnoyers

Quoted 129 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 110 lines · Mathieu Desnoyers
Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.
Quoted 6 lines
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 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.
Quoted 1 line
Does it make sense ?
This would break the case where the dereferenced variable is used as an index/offset where 0 is a valid value and still generates data dependencies.
So if with your new version we do:
  i = rcu_dereference(foo);
  j = table[i];
which translates into:
  i = ACCESS_ONCE(foo);
  if (i)
    smp_read_barrier_depends();
  j = table[i];
Quoted 6 lines
which when i == 0, would fail to do the barrier and can thus cause j to be a wrong value.
Sadly I'll have to defer to Paul to explain exactly how that can happen
- I always get my head in a horrible twist with this case.
I completely agree with you. However, given the current rcu_assign_pointer() implementation, we already have this problem. My proposal assumes the current rcu_assign_pointer() behavior is correct and that those are never ever used for index/offsets.
We could enforce this as a compile-time check with something along the lines of :
#define BUILD_BUG_ON_NOT_OFFSETABLE(x) (void)(x)[0]
And use it both in rcu_assign_pointer() and rcu_dereference(). It would check for any type passed to rcu_assign_pointer and rcu_dereference which is not either a pointer or an array.

Quoted 6 lines

Then if someone really want to shoot himself in the foot by casting a pointer to a long after the rcu_deref, that's his problem.
Hrm, looking at rcu_assign_pointer tells me that the ((v) != NULL) test should probably already complain if v is not a pointer. So my build test is probably unneeded.

Yeah, I was thinking in terms of rcu_dereference() working with both rcu_assign_pointer() and an as-yet-mythical rcu_assign_index(). Perhaps this would be a good time to get better names:

Current: rcu_assign_pointer() rcu_dereference()
New Pointers: rcu_publish_pointer() rcu_subscribe_pointer()
New Indexes: rcu_publish_index() rcu_subscribe_index()

And, while I am at it, work in a way of checking for either being in the appropriate RCU read-side critical section and/or having the needed lock/mutex/whatever held -- something I believe PeterZ was prototyping some months back.

Though I still am having a hard time with the conditional in rcu_dereference() vs. the smp_read_barrier_depends()...

Thanx, Paul

Peter Zijlstra

maintainer

2008-08-02 00:04 UTC #15 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 7 lines · Paul E. McKenney

Yeah, I was thinking in terms of rcu_dereference() working with both rcu_assign_pointer() and an as-yet-mythical rcu_assign_index(). Perhaps this would be a good time to get better names:
Current: rcu_assign_pointer() rcu_dereference()
New Pointers: rcu_publish_pointer() rcu_subscribe_pointer()
New Indexes: rcu_publish_index() rcu_subscribe_index()

Is it really worth the effort, splitting it out into these two cases?

Quoted 4 lines

And, while I am at it, work in a way of checking for either being in the appropriate RCU read-side critical section and/or having the needed lock/mutex/whatever held -- something I believe PeterZ was prototyping some months back.

Yeah - I have (bitrotted a bit, but should be salvageable) lockdep annotations for rcu_dereference().

The problem with them is the huge amount of false positives.. Take for example the Radix tree code, its perfectly fine to use the radix tree code without RCU - say you do the old rwlock style, still it uses rcu_dereference().

I never figured out a suitable way to annotate that.

Paul E. McKenney

2008-08-02 00:17 UTC #16 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 11 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney
Yeah, I was thinking in terms of rcu_dereference() working with both rcu_assign_pointer() and an as-yet-mythical rcu_assign_index(). Perhaps this would be a good time to get better names:
Current: rcu_assign_pointer() rcu_dereference()
New Pointers: rcu_publish_pointer() rcu_subscribe_pointer()
New Indexes: rcu_publish_index() rcu_subscribe_index()
Is it really worth the effort, splitting it out into these two cases?

Either we should split out into the pointer/index cases, or the definition of rcu_assign_pointer() should probably lose its current check for NULL...

Quoted 8 lines

Quoted 4 lines
And, while I am at it, work in a way of checking for either being in the appropriate RCU read-side critical section and/or having the needed lock/mutex/whatever held -- something I believe PeterZ was prototyping some months back.
Yeah - I have (bitrotted a bit, but should be salvageable) lockdep annotations for rcu_dereference().

Quoted 6 lines

The problem with them is the huge amount of false positives.. Take for example the Radix tree code, its perfectly fine to use the radix tree code without RCU - say you do the old rwlock style, still it uses rcu_dereference().
I never figured out a suitable way to annotate that.

My thought was to add a second argument that contained a boolean. If the rcu_dereference() was either within an RCU read-side critical section on the one hand or if the boolean evaluated to "true" on the other, then no assertion. This would require SPIN_LOCK_HELD() or similar primitives. (And one of the reasons for the renaming

Of course, in the case of radix tree, it would be necessary to pass the boolean in through the radix-tree read-side APIs, which would perhaps be a bit annoying.

Thanx, Paul

Paul E. McKenney

2008-08-01 21:15 UTC #17 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 129 lines · Peter Zijlstra

Quoted 110 lines · Mathieu Desnoyers
Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.
Quoted 6 lines
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 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.
Quoted 1 line
Does it make sense ?
This would break the case where the dereferenced variable is used as an index/offset where 0 is a valid value and still generates data dependencies.
So if with your new version we do:
  i = rcu_dereference(foo);
  j = table[i];
which translates into:
  i = ACCESS_ONCE(foo);
  if (i)
    smp_read_barrier_depends();
  j = table[i];

Quoted 5 lines

which when i == 0, would fail to do the barrier and can thus cause j to be a wrong value.
Sadly I'll have to defer to Paul to explain exactly how that can happen
- I always get my head in a horrible twist with this case.

Does http://lkml.org/lkml/2008/2/2/255 help? (Hmmm... I was intending to do a more formal write up of this, but clearly haven't gotten to it...)

Thanx, Paul

Mathieu Desnoyers

2008-07-15 16:26 UTC #18 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 88 lines · * Mathieu Desnoyers ([email protected]) wrote:

Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.
Quoted 6 lines
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

Quoted 3 lines

#define rcu_dereference(p)     ({ \
                                typeof(p) _________p1 = ACCESS_ONCE(p); \
                                if (p != NULL) \

Actually this line should be :

                                if (_________p1 != NULL) \

Quoted 7 lines

                                  smp_read_barrier_depends(); \
                                (_________p1); \
                                })

#define rcu_dereference_non_null(p)     ({ \
                                typeof(p) _________p1 = ACCESS_ONCE(p); \
                                DEBUG_RCU_BUG_ON(p == NULL); \

And this one :

                                DEBUG_RCU_BUG_ON(_________p1 == NULL); \

Mathieu

Quoted 56 lines

                                smp_read_barrier_depends(); \
                                (_________p1); \
                                })
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
Quoted 36 lines
Quoted 28 lines
Quoted 8 lines
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
Quoted 5 lines
864:
for 68 bytes.
My original implementation was 77 bytes, so yes, we have a win.
Ah, good good ! :-)
Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Paul E. McKenney

2008-08-01 21:14 UTC #19 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 95 lines · Mathieu Desnoyers

Quoted 71 lines · * Peter Zijlstra ([email protected]) wrote:
Quoted 26 lines · Mathieu Desnoyers
Quoted 15 lines · * Peter Zijlstra ([email protected]) 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();
Quoted 4 lines
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.
Quoted 7 lines
#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.
Quoted 11 lines
#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.
Quoted 7 lines
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.
Quoted 6 lines
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); \
                                })

Quoted 6 lines

#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

Quoted 52 lines

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
Quoted 36 lines
Quoted 28 lines
Quoted 8 lines
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
Quoted 5 lines
864:
for 68 bytes.
My original implementation was 77 bytes, so yes, we have a win.
Ah, good good ! :-)
Signature · 2 lines
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-15 17:50 UTC #20 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 24 lines · * Peter Zijlstra ([email protected]) wrote:

Quoted 24 lines

Quoted 2 lines

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:

Quoted 6 lines

Quoted 3 lines
for 68 bytes.
My original implementation was 77 bytes, so yes, we have a win.
Ah, good good ! :-)

For the same number of instruction bytes, here is yet another improvement. I removed the it_func[0] NULL test case, which is impossible. We never have an empty array. If the array is empty, the array pointer is set to NULL and the array is eventually freed when a quiescent state is reached.

/*
* it_func[0] is never NULL because there is at least one element in the array
* when the array itself is non NULL.
*/

#define __DO_TRACE(tp, proto, args)                                     \
        do {                                                            \
                void **it_func;                                         \
                                                                        \
                preempt_disable();                                      \
                it_func = rcu_dereference((tp)->funcs);                 \
                if (it_func) {                                          \
                        do {                                            \
                                ((void(*)(proto))(*it_func))(args);     \
                        } while (*(++it_func));                         \
                }                                                       \
                preempt_enable();                                       \
        } while (0)

P.S.: I'll change the preempt_enable/disable for rcu locks when I port this patchset to linux.next. I temporarily keep the preempt disable/enable statements.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Peter Zijlstra

maintainer

2008-07-15 14:03 UTC #21 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 44 lines · Mathieu Desnoyers

Quoted 14 lines · * Peter Zijlstra ([email protected]) wrote:
include/linux/tracepoint.h line 42
Quoted 9 lines · Mathieu Desnoyers
include/linux/tracepoint.h new file +9 -0
+#define __DO_TRACE(tp, proto, args)					\
+	do {								\
+		int i;							\
+		void **funcs;						\
+		preempt_disable();					\
+		funcs = (tp)->funcs;					\
+		smp_read_barrier_depends();				\
+		if (funcs) {						\
+			for (i = 0; funcs[i]; i++) {			\
Also, why is the preempt_disable needed?
Addition and removal of tracepoints is synchronized by RCU using the scheduler (and preempt_disable) as guarantees to find a quiescent state (this is really RCU "classic"). The update side uses rcu_barrier_sched() with call_rcu_sched() and the read/execute side uses "preempt_disable()/preempt_enable()".
Quoted 5 lines
Quoted 5 lines
kernel/tracepoint.c new file +5 -0
+static void tracepoint_entry_free_old(struct tracepoint_entry *entry, void *old)
+{
+	if (!old)
+		return;
+	entry->oldptr = old;
Quoted 9 lines
kernel/tracepoint.c line 85
Quoted 6 lines
kernel/tracepoint.c new file +6 -0
+	entry->rcu_pending = 1;
+	/* write rcu_pending before calling the RCU callback */
+	smp_wmb();
+#ifdef CONFIG_PREEMPT_RCU
+	synchronize_sched();	/* Until we have the call_rcu_sched() */
+#endif
Does this have something to do with the preempt_disable above?
Yes, it does. We make sure the previous array containing probes, which has been scheduled for deletion by the rcu callback, is indeed freed before we proceed to the next update. It therefore limits the rate of modification of a single tracepoint to one update per RCU period. The objective here is to permit fast batch add/removal of probes on _different_ tracepoints.

Quoted 2 lines

This use of "synchronize_sched()" can be changed for call_rcu_sched() in linux-next, I'll fix this.

Right, I thought as much, its just that the raw preempt_disable() without comments leaves one wondering if there is anything else going on.

Would it make sense to add:

rcu_read_sched_lock()
rcu_read_sched_unlock()

to match:

call_rcu_sched()
rcu_barrier_sched()
synchronize_sched()

Mathieu Desnoyers

author

2008-07-15 14:46 UTC #22 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 57 lines · * Peter Zijlstra ([email protected]) wrote:

Quoted 44 lines · Mathieu Desnoyers
Quoted 14 lines · * Peter Zijlstra ([email protected]) wrote:
include/linux/tracepoint.h line 42
Quoted 9 lines · Mathieu Desnoyers
include/linux/tracepoint.h new file +9 -0
+#define __DO_TRACE(tp, proto, args)					\
+	do {								\
+		int i;							\
+		void **funcs;						\
+		preempt_disable();					\
+		funcs = (tp)->funcs;					\
+		smp_read_barrier_depends();				\
+		if (funcs) {						\
+			for (i = 0; funcs[i]; i++) {			\
Also, why is the preempt_disable needed?
Addition and removal of tracepoints is synchronized by RCU using the scheduler (and preempt_disable) as guarantees to find a quiescent state (this is really RCU "classic"). The update side uses rcu_barrier_sched() with call_rcu_sched() and the read/execute side uses "preempt_disable()/preempt_enable()".
Quoted 5 lines
Quoted 5 lines
kernel/tracepoint.c new file +5 -0
+static void tracepoint_entry_free_old(struct tracepoint_entry *entry, void *old)
+{
+	if (!old)
+		return;
+	entry->oldptr = old;
Quoted 9 lines
kernel/tracepoint.c line 85
Quoted 6 lines
kernel/tracepoint.c new file +6 -0
+	entry->rcu_pending = 1;
+	/* write rcu_pending before calling the RCU callback */
+	smp_wmb();
+#ifdef CONFIG_PREEMPT_RCU
+	synchronize_sched();	/* Until we have the call_rcu_sched() */
+#endif
Does this have something to do with the preempt_disable above?
Yes, it does. We make sure the previous array containing probes, which has been scheduled for deletion by the rcu callback, is indeed freed before we proceed to the next update. It therefore limits the rate of modification of a single tracepoint to one update per RCU period. The objective here is to permit fast batch add/removal of probes on _different_ tracepoints.
Quoted 2 lines
This use of "synchronize_sched()" can be changed for call_rcu_sched() in linux-next, I'll fix this.
Right, I thought as much, its just that the raw preempt_disable() without comments leaves one wondering if there is anything else going on.
Would it make sense to add:
rcu_read_sched_lock()
rcu_read_sched_unlock()

Quoted 8 lines

to match:
call_rcu_sched()
rcu_barrier_sched()
synchronize_sched()
?

Yes, I would add them to include/linux/rcupdate.h. I'll include it with my next release.

Talking about headers, I have noticed that placing headers with the code may not be as clean as I would hope. For instance, the kernel/irq-trace.h header, when included from kernel/irq/handle.c, has to be included with:

#include "../irq-trace.h"

Which is not _that_ bad, but we we want to instrument the irq handler found in arch/x86/kernel/cpu/mcheck/mce_intel_64.c, including #include "../../../../../kernel/irq-trace.h" makes me go "yeeeek!"

How about creating include/trace/irq.h and friends ?

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Peter Zijlstra

maintainer

2008-07-15 15:14 UTC #23 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 11 lines · Mathieu Desnoyers

Talking about headers, I have noticed that placing headers with the code may not be as clean as I would hope. For instance, the kernel/irq-trace.h header, when included from kernel/irq/handle.c, has to be included with:
#include "../irq-trace.h"
Which is not _that_ bad, but we we want to instrument the irq handler found in arch/x86/kernel/cpu/mcheck/mce_intel_64.c, including #include "../../../../../kernel/irq-trace.h" makes me go "yeeeek!"
How about creating include/trace/irq.h and friends ?

Might as well.. anybody else got opinions?

Mathieu Desnoyers

author

2008-07-15 18:22 UTC #24 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 8 lines · * Peter Zijlstra ([email protected]) wrote:

Quoted 6 lines · Mathieu Desnoyers
Talking about headers, I have noticed that placing headers with the code may not be as clean as I would hope. For instance, the kernel/irq-trace.h header, when included from kernel/irq/handle.c, has to be included with:
#include "../irq-trace.h"

Quoted 8 lines

Quoted 5 lines
Which is not _that_ bad, but we we want to instrument the irq handler found in arch/x86/kernel/cpu/mcheck/mce_intel_64.c, including #include "../../../../../kernel/irq-trace.h" makes me go "yeeeek!"
How about creating include/trace/irq.h and friends ?
Might as well.. anybody else got opinions?

I'm also wondering if it's better to have :

filemap.h
fs.h
hugetlb.h
ipc.h
ipv4.h
ipv6.h
irq.h
kernel.h
memory.h
net.h
page.h
sched.h
swap.h
timer.h

all in include/trace/ or to create subdirectories first, like :

include/trace/net/
include/trace/mm/
...

or to go the other way around and re-use the existing subdirectories :

include/net/trace/
include/mm/trace/
...

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Steven Rostedt

maintainer

2008-07-15 18:33 UTC #25 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 17 lines · Mathieu Desnoyers

I'm also wondering if it's better to have :
filemap.h
fs.h
hugetlb.h
ipc.h
ipv4.h
ipv6.h
irq.h
kernel.h
memory.h
net.h
page.h
sched.h
swap.h
timer.h

This might be a better idea.

Quoted 5 lines

all in include/trace/ or to create subdirectories first, like :
include/trace/net/
include/trace/mm/

I think that is too much. A single trace directory should be sufficent.

Quoted 7 lines

....
or to go the other way around and re-use the existing subdirectories :
include/net/trace/
include/mm/trace/
....

I'm definitely against that.

-- Steve

Masami Hiramatsu

2008-07-15 18:55 UTC #26 lore raw

Re: [patch 01/15] Kernel Tracepoints

Hi,

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Mathieu Desnoyers
Talking about headers, I have noticed that placing headers with the code may not be as clean as I would hope. For instance, the kernel/irq-trace.h header, when included from kernel/irq/handle.c, has to be included with:
#include "../irq-trace.h"

Quoted 7 lines

Quoted 5 lines
Which is not _that_ bad, but we we want to instrument the irq handler found in arch/x86/kernel/cpu/mcheck/mce_intel_64.c, including #include "../../../../../kernel/irq-trace.h" makes me go "yeeeek!"
How about creating include/trace/irq.h and friends ?
Might as well.. anybody else got opinions?

I just wonder why DEFINE_TRACE are used in separated headers instead of include/linux/irq.h directly.

anyway, #include <trace/XXX.h> is good to me.

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Mathieu Desnoyers

author

2008-07-15 19:13 UTC #27 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 15 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi,
Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Mathieu Desnoyers
Talking about headers, I have noticed that placing headers with the code may not be as clean as I would hope. For instance, the kernel/irq-trace.h header, when included from kernel/irq/handle.c, has to be included with:
#include "../irq-trace.h"
Which is not _that_ bad, but we we want to instrument the irq handler found in arch/x86/kernel/cpu/mcheck/mce_intel_64.c, including #include "../../../../../kernel/irq-trace.h" makes me go "yeeeek!"

Quoted 9 lines

Quoted 3 lines
Quoted 1 line
How about creating include/trace/irq.h and friends ?
Might as well.. anybody else got opinions?
I just wonder why DEFINE_TRACE are used in separated headers instead of include/linux/irq.h directly.
anyway, #include <trace/XXX.h> is good to me.

Having these headers all placed nicely together will make it easier for people who are looking for already existing tracepoints to locate them.

It's also worth noting that I am considering deploying a standard set of tracepoints for userspace in a relatively short time frame. e.g. having the ability to add tracepoints to pthread mutexes seems like an interesting thing to have. And that will definitely require those headers to sit somewhere around /usr/include/trace/ or something similar, otherwise trying to locate those tracepoints will be hellish.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-15 19:02 UTC #28 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 57 lines · * Peter Zijlstra ([email protected]) wrote:

Quoted 44 lines · Mathieu Desnoyers
Quoted 14 lines · * Peter Zijlstra ([email protected]) wrote:
include/linux/tracepoint.h line 42
Quoted 9 lines · Mathieu Desnoyers
include/linux/tracepoint.h new file +9 -0
+#define __DO_TRACE(tp, proto, args)					\
+	do {								\
+		int i;							\
+		void **funcs;						\
+		preempt_disable();					\
+		funcs = (tp)->funcs;					\
+		smp_read_barrier_depends();				\
+		if (funcs) {						\
+			for (i = 0; funcs[i]; i++) {			\
Also, why is the preempt_disable needed?
Addition and removal of tracepoints is synchronized by RCU using the scheduler (and preempt_disable) as guarantees to find a quiescent state (this is really RCU "classic"). The update side uses rcu_barrier_sched() with call_rcu_sched() and the read/execute side uses "preempt_disable()/preempt_enable()".
Quoted 5 lines
Quoted 5 lines
kernel/tracepoint.c new file +5 -0
+static void tracepoint_entry_free_old(struct tracepoint_entry *entry, void *old)
+{
+	if (!old)
+		return;
+	entry->oldptr = old;
Quoted 9 lines
kernel/tracepoint.c line 85
Quoted 6 lines
kernel/tracepoint.c new file +6 -0
+	entry->rcu_pending = 1;
+	/* write rcu_pending before calling the RCU callback */
+	smp_wmb();
+#ifdef CONFIG_PREEMPT_RCU
+	synchronize_sched();	/* Until we have the call_rcu_sched() */
+#endif
Does this have something to do with the preempt_disable above?
Yes, it does. We make sure the previous array containing probes, which has been scheduled for deletion by the rcu callback, is indeed freed before we proceed to the next update. It therefore limits the rate of modification of a single tracepoint to one update per RCU period. The objective here is to permit fast batch add/removal of probes on _different_ tracepoints.
Quoted 2 lines
This use of "synchronize_sched()" can be changed for call_rcu_sched() in linux-next, I'll fix this.
Right, I thought as much, its just that the raw preempt_disable() without comments leaves one wondering if there is anything else going on.
Would it make sense to add:
rcu_read_sched_lock()
rcu_read_sched_unlock()

Quoted 8 lines

to match:
call_rcu_sched()
rcu_barrier_sched()
synchronize_sched()
?

Actually I think it's better to call them rcu_read_lock_sched() and rcu_read_unlock_sched() to match the _bh() equivalent already in rcupdate.h.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Peter Zijlstra

maintainer

2008-07-15 19:52 UTC #29 lore raw

Re: [patch 01/15] Kernel Tracepoints

Quoted 13 lines · Mathieu Desnoyers

Quoted 11 lines · * Peter Zijlstra ([email protected]) wrote:
Would it make sense to add:
rcu_read_sched_lock()
rcu_read_sched_unlock()
to match:
call_rcu_sched()
rcu_barrier_sched()
synchronize_sched()

Quoted 6 lines

Quoted 2 lines
?
Actually I think it's better to call them rcu_read_lock_sched() and rcu_read_unlock_sched() to match the _bh() equivalent already in rcupdate.h.

Sure,.

Mathieu Desnoyers

author

2008-07-09 15:04 UTC #30 lore raw

PATCH 2/15 Tracepoints Documentation

Documentation of tracepoint usage.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 Documentation/tracepoints.txt |  101 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

Documentation/tracepoints.txt new file +101 -0

Index: linux-2.6-lttng/Documentation/tracepoints.txt
===================================================================
@@ -0,0 +1,101 @@
+ 	             Using the Linux Kernel Tracepoints
+
+			    Mathieu Desnoyers
+
+
+This document introduces Linux Kernel Tracepoints and their use. It provides
+examples of how to insert tracepoints in the kernel and connect probe functions
+to them and provides some examples of probe functions.
+
+
+* Purpose of tracepoints
+
+A tracepoint placed in code provides a hook to call a function (probe) that you
+can provide at runtime. A tracepoint can be "on" (a probe is connected to it) or
+"off" (no probe is attached). When a tracepoint is "off" it has no effect,
+except for adding a tiny time penalty (checking a condition for a branch) and
+space penalty (adding a few bytes for the function call at the end of the
+instrumented function and adds a data structure in a separate section).  When a
+tracepoint is "on", the function you provide is called each time the tracepoint
+is executed, in the execution context of the caller. When the function provided
+ends its execution, it returns to the caller (continuing from the tracepoint
+site).
+
+You can put tracepoints at important locations in the code. They are
+lightweight hooks that can pass an arbitrary number of parameters,
+which prototypes are described in a tracepoint declaration placed in a header
+file.
+
+They can be used for tracing and performance accounting.
+
+
+* Usage
+
+Two elements are required for tracepoints :
+
+- A tracepoint definition, placed in a header file.
+- The tracepoint statement, in C code.
+
+In order to use tracepoints, you should include linux/tracepoint.h.
+
+In subsys/subsys-trace.h :
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(subsys_eventname,
+	TPPTOTO(int firstarg, struct task_struct *p),
+	TPARGS(firstarg, p));
+
+In subsys/file.c (where the tracing statement must be added) :
+
+#include "subsys-trace.h"
+
+void somefct(void)
+{
+	...
+	trace_subsys_eventname(arg, task);
+	...
+}
+
+Where :
+- subsys_eventname is an identifier unique to your event
+    - subsys is the name of your subsystem.
+    - eventname is the name of the event to trace.
+- TPPTOTO(int firstarg, struct task_struct *p) is the prototype of the function
+  called by this tracepoint.
+- TPARGS(firstarg, p) are the parameters names, same as found in the prototype.
+
+Connecting a function (probe) to a tracepoint is done by providing a probe
+(function to call) for the specific tracepoint through
+register_trace_subsys_eventname().  Removing a probe is done through
+unregister_trace_subsys_eventname(); it will remove the probe sure there is no
+caller left using the probe when it returns. Probe removal is preempt-safe
+because preemption is disabled around the probe call. See the "Probe example"
+section below for a sample probe module.
+
+The tracepoint mechanism supports inserting multiple instances of the same
+tracepoint, but a single definition must be made of a given tracepoint name over
+all the kernel to make sure no type conflict will occur. Name mangling of the
+tracepoints is done using the prototypes to make sure typing is correct.
+Verification of probe type correctness is done at the registration site by the
+compiler. Tracepoints can be put in inline functions, inlined static functions,
+and unrolled loops as well as regular functions.
+
+The naming scheme "subsys_event" is suggested here as a convention intended
+to limit collisions. Tracepoint names are global to the kernel: they are
+considered as being the same whether they are in the core kernel image or in
+modules.
+
+
+* Probe / tracepoint example
+
+See the example provided in samples/tracepoints/src
+
+Compile them with your kernel.
+
+Run, as root :
+modprobe tracepoint-example (insmod order is not important)
+modprobe tracepoint-probe-example
+cat /proc/tracepoint-example (returns an expected error)
+rmmod tracepoint-example tracepoint-probe-example
+dmesg

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:05 UTC #31 lore raw

PATCH 3/15 Tracepoints Samples

Tracepoint example code under samples/.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 samples/Kconfig                                |    6 ++
 samples/Makefile                               |    2 
 samples/tracepoints/Makefile                   |    6 ++
 samples/tracepoints/tp-samples-trace.h         |   13 +++++
 samples/tracepoints/tracepoint-probe-sample.c  |   55 +++++++++++++++++++++++++
 samples/tracepoints/tracepoint-probe-sample2.c |   42 +++++++++++++++++++
 samples/tracepoints/tracepoint-sample.c        |   53 ++++++++++++++++++++++++
 7 files changed, 176 insertions(+), 1 deletion(-)

samples/Kconfig +6 -0

Index: linux-2.6-lttng/samples/Kconfig
===================================================================
@@ -13,6 +13,12 @@ config SAMPLE_MARKERS
 	help
 	  This build markers example modules.
 
+config SAMPLE_TRACEPOINTS
+	tristate "Build tracepoints examples -- loadable modules only"
+	depends on TRACEPOINTS && m
+	help
+	  This build tracepoints example modules.
+
 config SAMPLE_KOBJECT
 	tristate "Build kobject examples"
 	help

samples/tracepoints/Makefile new file +6 -0

Index: linux-2.6-lttng/samples/tracepoints/Makefile
===================================================================
@@ -0,0 +1,6 @@
+# builds the tracepoint example kernel modules;
+# then to use one (as root):  insmod <module_name.ko>
+
+obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-sample.o
+obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample.o
+obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample2.o

samples/tracepoints/tracepoint-probe-sample.c new file +55 -0

55 lines not on this page · raw message

samples/tracepoints/tracepoint-sample.c new file +53 -0

53 lines not on this page · raw message

samples/tracepoints/tp-samples-trace.h new file +13 -0

Index: linux-2.6-lttng/samples/tracepoints/tp-samples-trace.h
===================================================================
@@ -0,0 +1,13 @@
+#ifndef _TP_SAMPLES_TRACE_H
+#define _TP_SAMPLES_TRACE_H
+
+#include <linux/proc_fs.h>	/* for struct inode and struct file */
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(subsys_event,
+	TPPROTO(struct inode *inode, struct file *file),
+	TPARGS(inode, file));
+DEFINE_TRACE(subsys_eventb,
+	TPPROTO(void),
+	TPARGS());
+#endif

samples/Makefile +1 -1

Index: linux-2.6-lttng/samples/Makefile
===================================================================
@@ -1,3 +1,3 @@
 # Makefile for Linux samples code
 
-obj-$(CONFIG_SAMPLES)	+= markers/ kobject/ kprobes/
+obj-$(CONFIG_SAMPLES)	+= markers/ kobject/ kprobes/ tracepoints/

samples/tracepoints/tracepoint-probe-sample2.c new file +42 -0

42 lines not on this page · raw message

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:05 UTC #32 lore raw

PATCH 4/15 LTTng instrumentation - irq

Instrumentation of IRQ related events : irq, softirq, tasklet entry and exit and softirq "raise" events.

It allows tracers to perform latency analysis on those various types of interrupts and to detect interrupts with max/min/avg duration. It helps detecting driver or hardware problems which cause an ISR to take ages to execute. It has been shown to be the case with bogus hardware causing an mmio read to take a few milliseconds.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Russell King <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/irq-trace.h  |   36 ++++++++++++++++++++++++++++++++++++
 kernel/irq/handle.c |    6 ++++++
 kernel/softirq.c    |    8 ++++++++
 3 files changed, 50 insertions(+)

kernel/irq/handle.c +6 -0

Index: linux-2.6-lttng/kernel/irq/handle.c
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include "../irq-trace.h"
 
 #include "internals.h"

@@ -130,6 +131,9 @@ irqreturn_t handle_IRQ_event(unsigned in
 {
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
+	struct pt_regs *regs = get_irq_regs();
+
+	trace_irq_entry(irq, regs);
 
 	handle_dynamic_tick(action);

@@ -148,6 +152,8 @@ irqreturn_t handle_IRQ_event(unsigned in
 		add_interrupt_randomness(irq);
 	local_irq_disable();
 
+	trace_irq_exit();
+
 	return retval;
 }

kernel/softirq.c +8 -0

Index: linux-2.6-lttng/kernel/softirq.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/rcupdate.h>
 #include <linux/smp.h>
 #include <linux/tick.h>
+#include "irq-trace.h"
 
 #include <asm/irq.h>
 /*

@@ -205,7 +206,9 @@ restart:
 
 	do {
 		if (pending & 1) {
+			trace_irq_softirq_entry(h, softirq_vec);
 			h->action(h);
+			trace_irq_softirq_exit(h, softirq_vec);
 			rcu_bh_qsctr_inc(cpu);
 		}
 		h++;

@@ -297,6 +300,7 @@ void irq_exit(void)
  */
 inline void raise_softirq_irqoff(unsigned int nr)
 {
+	trace_irq_softirq_raise(nr);
 	__raise_softirq_irqoff(nr);
 
 	/*

@@ -394,7 +398,9 @@ static void tasklet_action(struct softir
 			if (!atomic_read(&t->count)) {
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
 					BUG();
+				trace_irq_tasklet_low_entry(t);
 				t->func(t->data);
+				trace_irq_tasklet_low_exit(t);
 				tasklet_unlock(t);
 				continue;
 			}

@@ -429,7 +435,9 @@ static void tasklet_hi_action(struct sof
 			if (!atomic_read(&t->count)) {
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
 					BUG();
+				trace_irq_tasklet_high_entry(t);
 				t->func(t->data);
+				trace_irq_tasklet_high_exit(t);
 				tasklet_unlock(t);
 				continue;
 			}

kernel/irq-trace.h new file +36 -0

Index: linux-2.6-lttng/kernel/irq-trace.h
===================================================================
@@ -0,0 +1,36 @@
+#ifndef _IRQ_TRACE_H
+#define _IRQ_TRACE_H
+
+#include <linux/kdebug.h>
+#include <linux/interrupt.h>
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(irq_entry,
+	TPPROTO(unsigned int id, struct pt_regs *regs),
+	TPARGS(id, regs));
+DEFINE_TRACE(irq_exit,
+	TPPROTO(void),
+	TPARGS());
+DEFINE_TRACE(irq_softirq_entry,
+	TPPROTO(struct softirq_action *h, struct softirq_action *softirq_vec),
+	TPARGS(h, softirq_vec));
+DEFINE_TRACE(irq_softirq_exit,
+	TPPROTO(struct softirq_action *h, struct softirq_action *softirq_vec),
+	TPARGS(h, softirq_vec));
+DEFINE_TRACE(irq_softirq_raise,
+	TPPROTO(unsigned int nr),
+	TPARGS(nr));
+DEFINE_TRACE(irq_tasklet_low_entry,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+DEFINE_TRACE(irq_tasklet_low_exit,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+DEFINE_TRACE(irq_tasklet_high_entry,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+DEFINE_TRACE(irq_tasklet_high_exit,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Masami Hiramatsu

2008-07-09 16:41 UTC #33 lore raw

Re: [patch 04/15] LTTng instrumentation - irq

Quoted 57 lines · Mathieu Desnoyers

Instrumentation of IRQ related events : irq, softirq, tasklet entry and exit and softirq "raise" events.
It allows tracers to perform latency analysis on those various types of interrupts and to detect interrupts with max/min/avg duration. It helps detecting driver or hardware problems which cause an ISR to take ages to execute. It has been shown to be the case with bogus hardware causing an mmio read to take a few milliseconds.
Those tracepoints are used by LTTng.
About the performance impact of tracepoints (which is comparable to markers), even without immediate values optimizations, tests done by Hideo Aoki on ia64 show no regression. His test case was using hackbench on a kernel where scheduler instrumentation (about 5 events in code scheduler code) was added. See the "Tracepoints" patch header for performance result detail.
Signed-off-by Mathieu Desnoyers <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Russell King <[email protected]>
CC Masami Hiramatsu <[email protected]>
CC 'Peter Zijlstra' <[email protected]>
CC "Frank Ch. Eigler" <[email protected]>
CC 'Ingo Molnar' <[email protected]>
CC 'Hideo AOKI' <[email protected]>
CC Takashi Nishiie <[email protected]>
CC 'Steven Rostedt' <[email protected]>
CC Eduard - Gabriel Munteanu <[email protected]>
---
 kernel/irq-trace.h  |   36 ++++++++++++++++++++++++++++++++++++
 kernel/irq/handle.c |    6 ++++++
 kernel/softirq.c    |    8 ++++++++
 3 files changed, 50 insertions(+)
Index: linux-2.6-lttng/kernel/irq/handle.c
===================================================================
kernel/irq/handle.c +4 -0
@@ -15,6 +15,7 @@
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include "../irq-trace.h"
 
 #include "internals.h"
 
@@ -130,6 +131,9 @@ irqreturn_t handle_IRQ_event(unsigned in
 {
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
+	struct pt_regs *regs = get_irq_regs();
+
+	trace_irq_entry(irq, regs);
 
 	handle_dynamic_tick(action);
 

kernel/irq/handle.c irqreturn_t handle_IRQ_event(unsigned in · line 156

Quoted 5 lines

kernel/irq/handle.c +2 -0

@@ -148,6 +152,8 @@ irqreturn_t handle_IRQ_event(unsigned in
 		add_interrupt_randomness(irq);
 	local_irq_disable();
 
+	trace_irq_exit();
+

Hi Mathieu,
What would you think tracing return value of irq handlers here? like:
trace_irq_exit(retval);

So, we can check the irq was handled correctly or not.

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Mathieu Desnoyers

author

2008-07-09 17:06 UTC #34 lore raw

Re: [patch 04/15] LTTng instrumentation - irq (update)

Quoted 11 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,
What would you think tracing return value of irq handlers here? like:
trace_irq_exit(retval);
So, we can check the irq was handled correctly or not.
Thank you,

Sure, here is the updated patch.

Thanks,

Mathieu

LTTng instrumentation - irq

Instrumentation of IRQ related events : irq, softirq, tasklet entry and exit and softirq "raise" events.

Those tracepoints are used by LTTng.

Changelog:
- Add retval as irq_exit argument.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Russell King <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/irq-trace.h  |   36 ++++++++++++++++++++++++++++++++++++
 kernel/irq/handle.c |    6 ++++++
 kernel/softirq.c    |    8 ++++++++
 3 files changed, 50 insertions(+)

kernel/irq/handle.c +6 -0

Index: linux-2.6-lttng/kernel/irq/handle.c
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include "../irq-trace.h"
 
 #include "internals.h"

@@ -130,6 +131,9 @@ irqreturn_t handle_IRQ_event(unsigned in
 {
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
+	struct pt_regs *regs = get_irq_regs();
+
+	trace_irq_entry(irq, regs);
 
 	handle_dynamic_tick(action);

@@ -148,6 +152,8 @@ irqreturn_t handle_IRQ_event(unsigned in
 		add_interrupt_randomness(irq);
 	local_irq_disable();
 
+	trace_irq_exit(retval);
+
 	return retval;
 }

kernel/softirq.c +8 -0

Index: linux-2.6-lttng/kernel/softirq.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/rcupdate.h>
 #include <linux/smp.h>
 #include <linux/tick.h>
+#include "irq-trace.h"
 
 #include <asm/irq.h>
 /*

@@ -231,7 +232,9 @@ restart:
 
 	do {
 		if (pending & 1) {
+			trace_irq_softirq_entry(h, softirq_vec);
 			h->action(h);
+			trace_irq_softirq_exit(h, softirq_vec);
 			rcu_bh_qsctr_inc(cpu);
 		}
 		h++;

@@ -323,6 +326,7 @@ void irq_exit(void)
  */
 inline void raise_softirq_irqoff(unsigned int nr)
 {
+	trace_irq_softirq_raise(nr);
 	__raise_softirq_irqoff(nr);
 
 	/*

@@ -412,7 +416,9 @@ static void tasklet_action(struct softir
 			if (!atomic_read(&t->count)) {
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
 					BUG();
+				trace_irq_tasklet_low_entry(t);
 				t->func(t->data);
+				trace_irq_tasklet_low_exit(t);
 				tasklet_unlock(t);
 				continue;
 			}

@@ -447,7 +453,9 @@ static void tasklet_hi_action(struct sof
 			if (!atomic_read(&t->count)) {
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
 					BUG();
+				trace_irq_tasklet_high_entry(t);
 				t->func(t->data);
+				trace_irq_tasklet_high_exit(t);
 				tasklet_unlock(t);
 				continue;
 			}

kernel/irq-trace.h new file +36 -0

Index: linux-2.6-lttng/kernel/irq-trace.h
===================================================================
@@ -0,0 +1,36 @@
+#ifndef _IRQ_TRACE_H
+#define _IRQ_TRACE_H
+
+#include <linux/kdebug.h>
+#include <linux/interrupt.h>
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(irq_entry,
+	TPPROTO(unsigned int id, struct pt_regs *regs),
+	TPARGS(id, regs));
+DEFINE_TRACE(irq_exit,
+	TPPROTO(irqreturn_t retval),
+	TPARGS(retval));
+DEFINE_TRACE(irq_softirq_entry,
+	TPPROTO(struct softirq_action *h, struct softirq_action *softirq_vec),
+	TPARGS(h, softirq_vec));
+DEFINE_TRACE(irq_softirq_exit,
+	TPPROTO(struct softirq_action *h, struct softirq_action *softirq_vec),
+	TPARGS(h, softirq_vec));
+DEFINE_TRACE(irq_softirq_raise,
+	TPPROTO(unsigned int nr),
+	TPARGS(nr));
+DEFINE_TRACE(irq_tasklet_low_entry,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+DEFINE_TRACE(irq_tasklet_low_exit,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+DEFINE_TRACE(irq_tasklet_high_entry,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+DEFINE_TRACE(irq_tasklet_high_exit,
+	TPPROTO(struct tasklet_struct *t),
+	TPARGS(t));
+
+#endif

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:01 UTC #35 lore raw

Mathieu Desnoyers

author

2008-07-09 15:02 UTC #36 lore raw

PATCH 5/15 LTTng instrumentation - scheduler

Instrument the scheduler activity (sched_switch, migration, wakeups, wait for a task, signal delivery) and process/thread creation/destruction (fork, exit, kthread stop). Actually, kthread creation is not instrumented in this patch because it is architecture dependent. It allows to connect tracers such as ftrace which detects scheduling latencies, good/bad scheduler decisions. Tools like LTTng can export this scheduler information along with instrumentation of the rest of the kernel activity to perform post-mortem analysis on the scheduler activity.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/exit.c        |    6 ++++++
 kernel/fork.c        |    3 +++
 kernel/kthread.c     |    5 +++++
 kernel/sched-trace.h |   43 +++++++++++++++++++++++++++++++++++++++++++
 kernel/sched.c       |    4 ++++
 kernel/signal.c      |    3 +++
 6 files changed, 64 insertions(+)

kernel/kthread.c +5 -0

Index: linux-2.6-lttng/kernel/kthread.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include "sched-trace.h"
 
 #define KTHREAD_NICE_LEVEL (-5)

@@ -187,6 +188,8 @@ int kthread_stop(struct task_struct *k)
 	/* It could exit after stop_info.k set, but before wake_up_process. */
 	get_task_struct(k);
 
+	trace_sched_kthread_stop(k);
+
 	/* Must init completion *before* thread sees kthread_stop_info.k */
 	init_completion(&kthread_stop_info.done);
 	smp_wmb();

@@ -202,6 +205,8 @@ int kthread_stop(struct task_struct *k)
 	ret = kthread_stop_info.err;
 	mutex_unlock(&kthread_stop_lock);
 
+	trace_sched_kthread_stop_ret(ret);
+
 	return ret;
 }
 EXPORT_SYMBOL(kthread_stop);

kernel/sched.c +4 -0

Index: linux-2.6-lttng/kernel/sched.c
===================================================================
@@ -1987,6 +1987,7 @@ void wait_task_inactive(struct task_stru
 		 * just go back and repeat.
 		 */
 		rq = task_rq_lock(p, &flags);
+		trace_sched_wait_task(p);
 		running = task_running(rq, p);
 		on_rq = p->se.on_rq;
 		task_rq_unlock(rq, &flags);

@@ -2275,6 +2276,7 @@ static int try_to_wake_up(struct task_st
 
 	smp_wmb();
 	rq = task_rq_lock(p, &flags);
+	trace_sched_try_wakeup(p);
 	old_state = p->state;
 	if (!(old_state & state))
 		goto out;

@@ -2457,6 +2459,7 @@ void wake_up_new_task(struct task_struct
 	struct rq *rq;
 
 	rq = task_rq_lock(p, &flags);
+	trace_sched_wakeup_new_task(p);
 	BUG_ON(p->state != TASK_RUNNING);
 	update_rq_clock(rq);

@@ -2884,6 +2887,7 @@ static void sched_migrate_task(struct ta
 	    || unlikely(cpu_is_offline(dest_cpu)))
 		goto out;
 
+	trace_sched_migrate_task(p, dest_cpu);
 	/* force the process onto the specified CPU */
 	if (migrate_task(p, dest_cpu, &req)) {
 		/* Need to wait for migration thread (might exit: take ref). */

kernel/exit.c +6 -0

Index: linux-2.6-lttng/kernel/exit.c
===================================================================
@@ -46,6 +46,7 @@
 #include <linux/resource.h>
 #include <linux/blkdev.h>
 #include <linux/task_io_accounting_ops.h>
+#include "sched-trace.h"
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>

@@ -149,6 +150,7 @@ static void __exit_signal(struct task_st
 
 static void delayed_put_task_struct(struct rcu_head *rhp)
 {
+	trace_sched_process_free(container_of(rhp, struct task_struct, rcu));
 	put_task_struct(container_of(rhp, struct task_struct, rcu));
 }

@@ -1040,6 +1042,8 @@ NORET_TYPE void do_exit(long code)
 
 	if (group_dead)
 		acct_process();
+	trace_sched_process_exit(tsk);
+
 	exit_sem(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);

@@ -1524,6 +1528,8 @@ static long do_wait(enum pid_type type, 
 	struct task_struct *tsk;
 	int flag, retval;
 
+	trace_sched_process_wait(pid);
+
 	add_wait_queue(&current->signal->wait_chldexit,&wait);
 repeat:
 	/* If there is nothing that can match our critier just get out */

kernel/fork.c +3 -0

Index: linux-2.6-lttng/kernel/fork.c
===================================================================
@@ -56,6 +56,7 @@
 #include <linux/proc_fs.h>
 #include <linux/blkdev.h>
 #include <linux/magic.h>
+#include "sched-trace.h"
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -1362,6 +1363,8 @@ long do_fork(unsigned long clone_flags,
 	if (!IS_ERR(p)) {
 		struct completion vfork;
 
+		trace_sched_process_fork(current, p);
+
 		nr = task_pid_vnr(p);
 
 		if (clone_flags & CLONE_PARENT_SETTID)

kernel/signal.c +3 -0

Index: linux-2.6-lttng/kernel/signal.c
===================================================================
@@ -26,6 +26,7 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
+#include "sched-trace.h"
 
 #include <asm/param.h>
 #include <asm/uaccess.h>

@@ -807,6 +808,8 @@ static int send_signal(int sig, struct s
 	struct sigpending *pending;
 	struct sigqueue *q;
 
+	trace_sched_signal_send(sig, t);
+
 	assert_spin_locked(&t->sighand->siglock);
 	if (!prepare_signal(sig, t))
 		return 0;

kernel/sched-trace.h new file +43 -0

43 lines not on this page · raw message

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:34 UTC #37 lore raw

Re: [patch 05/15] LTTng instrumentation - scheduler (repost)

There were 2 rejects when I ported the patch to linux-next. Sorry. Here is a repost.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/exit.c        |    6 ++++++
 kernel/fork.c        |    3 +++
 kernel/kthread.c     |    5 +++++
 kernel/sched-trace.h |   43 +++++++++++++++++++++++++++++++++++++++++++
 kernel/sched.c       |   11 ++++++-----
 kernel/signal.c      |    3 +++
 6 files changed, 66 insertions(+), 5 deletions(-)

kernel/kthread.c +5 -0

Index: linux-2.6-lttng/kernel/kthread.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include "sched-trace.h"
 
 #define KTHREAD_NICE_LEVEL (-5)

@@ -187,6 +188,8 @@ int kthread_stop(struct task_struct *k)
 	/* It could exit after stop_info.k set, but before wake_up_process. */
 	get_task_struct(k);
 
+	trace_sched_kthread_stop(k);
+
 	/* Must init completion *before* thread sees kthread_stop_info.k */
 	init_completion(&kthread_stop_info.done);
 	smp_wmb();

@@ -202,6 +205,8 @@ int kthread_stop(struct task_struct *k)
 	ret = kthread_stop_info.err;
 	mutex_unlock(&kthread_stop_lock);
 
+	trace_sched_kthread_stop_ret(ret);
+
 	return ret;
 }
 EXPORT_SYMBOL(kthread_stop);

kernel/sched.c +6 -5

Index: linux-2.6-lttng/kernel/sched.c
===================================================================
@@ -71,6 +71,7 @@
 #include <linux/debugfs.h>
 #include <linux/ctype.h>
 #include <linux/ftrace.h>
+#include "sched-trace.h"
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>

@@ -1987,6 +1988,7 @@ void wait_task_inactive(struct task_stru
 		 * just go back and repeat.
 		 */
 		rq = task_rq_lock(p, &flags);
+		trace_sched_wait_task(p);
 		running = task_running(rq, p);
 		on_rq = p->se.on_rq;
 		task_rq_unlock(rq, &flags);

@@ -2275,6 +2277,7 @@ static int try_to_wake_up(struct task_st
 
 	smp_wmb();
 	rq = task_rq_lock(p, &flags);
+	trace_sched_try_wakeup(p);
 	old_state = p->state;
 	if (!(old_state & state))
 		goto out;

@@ -2457,6 +2460,7 @@ void wake_up_new_task(struct task_struct
 	struct rq *rq;
 
 	rq = task_rq_lock(p, &flags);
+	trace_sched_wakeup_new_task(p);
 	BUG_ON(p->state != TASK_RUNNING);
 	update_rq_clock(rq);

@@ -2647,11 +2651,7 @@ context_switch(struct rq *rq, struct tas
 	struct mm_struct *mm, *oldmm;
 
 	prepare_task_switch(rq, prev, next);
-	trace_mark(kernel_sched_schedule,
-		"prev_pid %d next_pid %d prev_state %ld "
-		"## rq %p prev %p next %p",
-		prev->pid, next->pid, prev->state,
-		rq, prev, next);
+	trace_sched_switch(prev, next);
 	mm = next->mm;
 	oldmm = prev->active_mm;
 	/*

@@ -2884,6 +2884,7 @@ static void sched_migrate_task(struct ta
 	    || unlikely(cpu_is_offline(dest_cpu)))
 		goto out;
 
+	trace_sched_migrate_task(p, dest_cpu);
 	/* force the process onto the specified CPU */
 	if (migrate_task(p, dest_cpu, &req)) {
 		/* Need to wait for migration thread (might exit: take ref). */

kernel/exit.c +6 -0

Index: linux-2.6-lttng/kernel/exit.c
===================================================================
@@ -46,6 +46,7 @@
 #include <linux/resource.h>
 #include <linux/blkdev.h>
 #include <linux/task_io_accounting_ops.h>
+#include "sched-trace.h"
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>

@@ -149,6 +150,7 @@ static void __exit_signal(struct task_st
 
 static void delayed_put_task_struct(struct rcu_head *rhp)
 {
+	trace_sched_process_free(container_of(rhp, struct task_struct, rcu));
 	put_task_struct(container_of(rhp, struct task_struct, rcu));
 }

@@ -1040,6 +1042,8 @@ NORET_TYPE void do_exit(long code)
 
 	if (group_dead)
 		acct_process();
+	trace_sched_process_exit(tsk);
+
 	exit_sem(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);

@@ -1524,6 +1528,8 @@ static long do_wait(enum pid_type type, 
 	struct task_struct *tsk;
 	int flag, retval;
 
+	trace_sched_process_wait(pid);
+
 	add_wait_queue(&current->signal->wait_chldexit,&wait);
 repeat:
 	/* If there is nothing that can match our critier just get out */

kernel/fork.c +3 -0

Index: linux-2.6-lttng/kernel/fork.c
===================================================================
@@ -56,6 +56,7 @@
 #include <linux/proc_fs.h>
 #include <linux/blkdev.h>
 #include <linux/magic.h>
+#include "sched-trace.h"
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -1362,6 +1363,8 @@ long do_fork(unsigned long clone_flags,
 	if (!IS_ERR(p)) {
 		struct completion vfork;
 
+		trace_sched_process_fork(current, p);
+
 		nr = task_pid_vnr(p);
 
 		if (clone_flags & CLONE_PARENT_SETTID)

kernel/signal.c +3 -0

Index: linux-2.6-lttng/kernel/signal.c
===================================================================
@@ -26,6 +26,7 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
+#include "sched-trace.h"
 
 #include <asm/param.h>
 #include <asm/uaccess.h>

@@ -807,6 +808,8 @@ static int send_signal(int sig, struct s
 	struct sigpending *pending;
 	struct sigqueue *q;
 
+	trace_sched_signal_send(sig, t);
+
 	assert_spin_locked(&t->sighand->siglock);
 	if (!prepare_signal(sig, t))
 		return 0;

kernel/sched-trace.h new file +43 -0

43 lines not on this page · raw message

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Ingo Molnar

2008-07-09 15:39 UTC #38 lore raw

Re: [patch 05/15] LTTng instrumentation - scheduler (repost)

Quoted 2 lines · Mathieu Desnoyers

There were 2 rejects when I ported the patch to linux-next. Sorry. Here is a repost.

there's still a standing objection (NAK) from Peter Zijstra so none of this can go into linux-next yet i'm afraid.

Ingo

Mathieu Desnoyers

author

2008-07-09 16:00 UTC #39 lore raw

Re: [patch 05/15] LTTng instrumentation - scheduler (repost)

Quoted 10 lines · * Ingo Molnar ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers
There were 2 rejects when I ported the patch to linux-next. Sorry. Here is a repost.
there's still a standing objection (NAK) from Peter Zijstra so none of this can go into linux-next yet i'm afraid.
Ingo

Hi Ingo,

This "tracepoint" infrastructure has been created to answer to Peter's concerns about trace_mark uglyness. The last comment I had from him when we discussed the tracepoint idea was "Looking forward to your proposal..", so I guess it's up to him to decide if tracepoints are clean enough to live.

I'll submit a revised scheduler instrumentation which turns all scheduler trace_mark() already in linux-next into tracepoints, with the same arguments. ftrace will have to be updated accordingly.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 16:22 UTC #40 lore raw

Re: [patch 05/15] LTTng instrumentation - scheduler (merge ftrace markers)

Peter : this is the instrumentation from ftrace (exact same location of trace_mark statements) turned into tracepoints. How do you like it ? If we merge this, ftrace will have to be adapted accordingly.

LTTng instrumentation - scheduler

Changelog :
- Change instrumentation location and parameter to match ftrace instrumentation, previously done with kernel markers.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/exit.c        |    6 ++++++
 kernel/fork.c        |    3 +++
 kernel/kthread.c     |    5 +++++
 kernel/sched-trace.h |   45 +++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched.c       |   17 ++++++-----------
 kernel/signal.c      |    3 +++
 6 files changed, 68 insertions(+), 11 deletions(-)

kernel/kthread.c +5 -0

Index: linux-2.6-lttng/kernel/kthread.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include "sched-trace.h"
 
 #define KTHREAD_NICE_LEVEL (-5)

@@ -187,6 +188,8 @@ int kthread_stop(struct task_struct *k)
 	/* It could exit after stop_info.k set, but before wake_up_process. */
 	get_task_struct(k);
 
+	trace_sched_kthread_stop(k);
+
 	/* Must init completion *before* thread sees kthread_stop_info.k */
 	init_completion(&kthread_stop_info.done);
 	smp_wmb();

@@ -202,6 +205,8 @@ int kthread_stop(struct task_struct *k)
 	ret = kthread_stop_info.err;
 	mutex_unlock(&kthread_stop_lock);
 
+	trace_sched_kthread_stop_ret(ret);
+
 	return ret;
 }
 EXPORT_SYMBOL(kthread_stop);

kernel/sched.c +6 ~~-11~~

Index: linux-2.6-lttng/kernel/sched.c
===================================================================
@@ -71,6 +71,7 @@
 #include <linux/debugfs.h>
 #include <linux/ctype.h>
 #include <linux/ftrace.h>
+#include "sched-trace.h"
 
 #include <asm/tlb.h>
 #include <asm/irq_regs.h>

@@ -1987,6 +1988,7 @@ void wait_task_inactive(struct task_stru
 		 * just go back and repeat.
 		 */
 		rq = task_rq_lock(p, &flags);
+		trace_sched_wait_task(rq, p);
 		running = task_running(rq, p);
 		on_rq = p->se.on_rq;
 		task_rq_unlock(rq, &flags);

@@ -2337,9 +2339,7 @@ out_activate:
 	success = 1;
 
 out_running:
-	trace_mark(kernel_sched_wakeup,
-		"pid %d state %ld ## rq %p task %p rq->curr %p",
-		p->pid, p->state, rq, p, rq->curr);
+	trace_sched_wakeup(rq, p);
 	check_preempt_curr(rq, p);
 
 	p->state = TASK_RUNNING;

@@ -2472,9 +2472,7 @@ void wake_up_new_task(struct task_struct
 		p->sched_class->task_new(rq, p);
 		inc_nr_running(rq);
 	}
-	trace_mark(kernel_sched_wakeup_new,
-		"pid %d state %ld ## rq %p task %p rq->curr %p",
-		p->pid, p->state, rq, p, rq->curr);
+	trace_sched_wakeup_new(rq, p);
 	check_preempt_curr(rq, p);
 #ifdef CONFIG_SMP
 	if (p->sched_class->task_wake_up)

@@ -2647,11 +2645,7 @@ context_switch(struct rq *rq, struct tas
 	struct mm_struct *mm, *oldmm;
 
 	prepare_task_switch(rq, prev, next);
-	trace_mark(kernel_sched_schedule,
-		"prev_pid %d next_pid %d prev_state %ld "
-		"## rq %p prev %p next %p",
-		prev->pid, next->pid, prev->state,
-		rq, prev, next);
+	trace_sched_switch(rq, prev, next);
 	mm = next->mm;
 	oldmm = prev->active_mm;
 	/*

@@ -2884,6 +2878,7 @@ static void sched_migrate_task(struct ta
 	    || unlikely(cpu_is_offline(dest_cpu)))
 		goto out;
 
+	trace_sched_migrate_task(rq, p, dest_cpu);
 	/* force the process onto the specified CPU */
 	if (migrate_task(p, dest_cpu, &req)) {
 		/* Need to wait for migration thread (might exit: take ref). */

kernel/exit.c +6 -0

Index: linux-2.6-lttng/kernel/exit.c
===================================================================
@@ -46,6 +46,7 @@
 #include <linux/resource.h>
 #include <linux/blkdev.h>
 #include <linux/task_io_accounting_ops.h>
+#include "sched-trace.h"
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>

@@ -149,6 +150,7 @@ static void __exit_signal(struct task_st
 
 static void delayed_put_task_struct(struct rcu_head *rhp)
 {
+	trace_sched_process_free(container_of(rhp, struct task_struct, rcu));
 	put_task_struct(container_of(rhp, struct task_struct, rcu));
 }

@@ -1040,6 +1042,8 @@ NORET_TYPE void do_exit(long code)
 
 	if (group_dead)
 		acct_process();
+	trace_sched_process_exit(tsk);
+
 	exit_sem(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);

@@ -1524,6 +1528,8 @@ static long do_wait(enum pid_type type, 
 	struct task_struct *tsk;
 	int flag, retval;
 
+	trace_sched_process_wait(pid);
+
 	add_wait_queue(&current->signal->wait_chldexit,&wait);
 repeat:
 	/* If there is nothing that can match our critier just get out */

kernel/fork.c +3 -0

Index: linux-2.6-lttng/kernel/fork.c
===================================================================
@@ -56,6 +56,7 @@
 #include <linux/proc_fs.h>
 #include <linux/blkdev.h>
 #include <linux/magic.h>
+#include "sched-trace.h"
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -1362,6 +1363,8 @@ long do_fork(unsigned long clone_flags,
 	if (!IS_ERR(p)) {
 		struct completion vfork;
 
+		trace_sched_process_fork(current, p);
+
 		nr = task_pid_vnr(p);
 
 		if (clone_flags & CLONE_PARENT_SETTID)

kernel/signal.c +3 -0

Index: linux-2.6-lttng/kernel/signal.c
===================================================================
@@ -26,6 +26,7 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
+#include "sched-trace.h"
 
 #include <asm/param.h>
 #include <asm/uaccess.h>

@@ -807,6 +808,8 @@ static int send_signal(int sig, struct s
 	struct sigpending *pending;
 	struct sigqueue *q;
 
+	trace_sched_signal_send(sig, t);
+
 	assert_spin_locked(&t->sighand->siglock);
 	if (!prepare_signal(sig, t))
 		return 0;

kernel/sched-trace.h new file +45 -0

45 lines not on this page · raw message

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 19:10 UTC #41 lore raw

PATCH 1/1 ftrace port to tracepoints (linux-next)

Porting the trace_mark() used by ftrace to tracepoints. (cleanup)

This patch applies after the "Tracepoints v3" patchset for linux-next.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/trace/trace_sched_switch.c |  114 ++++++---------------------------
 kernel/trace/trace_sched_wakeup.c |  129 +++++++++-----------------------------
 2 files changed, 52 insertions(+), 191 deletions(-)

kernel/trace/trace_sched_switch.c +21 ~~-93~~

Index: linux-2.6-lttng/kernel/trace/trace_sched_switch.c
===================================================================
@@ -9,9 +9,9 @@
 #include <linux/debugfs.h>
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
-#include <linux/marker.h>
 #include <linux/ftrace.h>
 
+#include "../sched-trace.h"
 #include "trace.h"
 
 static struct trace_array	*ctx_trace;

@@ -19,16 +19,17 @@ static int __read_mostly	tracer_enabled;
 static atomic_t			sched_ref;
 
 static void
-sched_switch_func(void *private, void *__rq, struct task_struct *prev,
+probe_sched_switch(struct rq *__rq, struct task_struct *prev,
 			struct task_struct *next)
 {
-	struct trace_array **ptr = private;
-	struct trace_array *tr = *ptr;
 	struct trace_array_cpu *data;
 	unsigned long flags;
 	long disabled;
 	int cpu;
 
+	if (!atomic_read(&sched_ref))
+		return;
+
 	tracing_record_cmdline(prev);
 	tracing_record_cmdline(next);

@@ -37,95 +38,42 @@ sched_switch_func(void *private, void *_
 
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
-	data = tr->data[cpu];
+	data = ctx_trace->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);
 
 	if (likely(disabled == 1))
-		tracing_sched_switch_trace(tr, data, prev, next, flags);
+		tracing_sched_switch_trace(ctx_trace, data, prev, next, flags);
 
 	atomic_dec(&data->disabled);
 	local_irq_restore(flags);
 }
 
-static notrace void
-sched_switch_callback(void *probe_data, void *call_data,
-		      const char *format, va_list *args)
-{
-	struct task_struct *prev;
-	struct task_struct *next;
-	struct rq *__rq;
-
-	if (!atomic_read(&sched_ref))
-		return;
-
-	/* skip prev_pid %d next_pid %d prev_state %ld */
-	(void)va_arg(*args, int);
-	(void)va_arg(*args, int);
-	(void)va_arg(*args, long);
-	__rq = va_arg(*args, typeof(__rq));
-	prev = va_arg(*args, typeof(prev));
-	next = va_arg(*args, typeof(next));
-
-	/*
-	 * If tracer_switch_func only points to the local
-	 * switch func, it still needs the ptr passed to it.
-	 */
-	sched_switch_func(probe_data, __rq, prev, next);
-}
-
 static void
-wakeup_func(void *private, void *__rq, struct task_struct *wakee, struct
-			task_struct *curr)
+probe_sched_wakeup(struct rq *__rq, struct task_struct *wakee)
 {
-	struct trace_array **ptr = private;
-	struct trace_array *tr = *ptr;
 	struct trace_array_cpu *data;
 	unsigned long flags;
 	long disabled;
 	int cpu;
 
-	if (!tracer_enabled)
+	if (!likely(tracer_enabled))
 		return;
 
-	tracing_record_cmdline(curr);
+	tracing_record_cmdline(current);
 
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
-	data = tr->data[cpu];
+	data = ctx_trace->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);
 
 	if (likely(disabled == 1))
-		tracing_sched_wakeup_trace(tr, data, wakee, curr, flags);
+		tracing_sched_wakeup_trace(ctx_trace, data, wakee, current,
+			flags);
 
 	atomic_dec(&data->disabled);
 	local_irq_restore(flags);
 }
 
-static notrace void
-wake_up_callback(void *probe_data, void *call_data,
-		 const char *format, va_list *args)
-{
-	struct task_struct *curr;
-	struct task_struct *task;
-	struct rq *__rq;
-
-	if (likely(!tracer_enabled))
-		return;
-
-	/* Skip pid %d state %ld */
-	(void)va_arg(*args, int);
-	(void)va_arg(*args, long);
-	/* now get the meat: "rq %p task %p rq->curr %p" */
-	__rq = va_arg(*args, typeof(__rq));
-	task = va_arg(*args, typeof(task));
-	curr = va_arg(*args, typeof(curr));
-
-	tracing_record_cmdline(task);
-	tracing_record_cmdline(curr);
-
-	wakeup_func(probe_data, __rq, task, curr);
-}
-
 static void sched_switch_reset(struct trace_array *tr)
 {
 	int cpu;

@@ -140,31 +88,21 @@ static int tracing_sched_register(void)
 {
 	int ret;
 
-	ret = marker_probe_register("kernel_sched_wakeup",
-			"pid %d state %ld ## rq %p task %p rq->curr %p",
-			wake_up_callback,
-			&ctx_trace);
+	ret = register_trace_sched_wakeup(probe_sched_wakeup);
 	if (ret) {
 		pr_info("wakeup trace: Couldn't add marker"
 			" probe to kernel_sched_wakeup\n");
 		return ret;
 	}
 
-	ret = marker_probe_register("kernel_sched_wakeup_new",
-			"pid %d state %ld ## rq %p task %p rq->curr %p",
-			wake_up_callback,
-			&ctx_trace);
+	ret = register_trace_sched_wakeup_new(probe_sched_wakeup);
 	if (ret) {
 		pr_info("wakeup trace: Couldn't add marker"
 			" probe to kernel_sched_wakeup_new\n");
 		goto fail_deprobe;
 	}
 
-	ret = marker_probe_register("kernel_sched_schedule",
-		"prev_pid %d next_pid %d prev_state %ld "
-		"## rq %p prev %p next %p",
-		sched_switch_callback,
-		&ctx_trace);
+	ret = register_trace_sched_switch(probe_sched_switch);
 	if (ret) {
 		pr_info("sched trace: Couldn't add marker"
 			" probe to kernel_sched_schedule\n");

@@ -173,27 +111,17 @@ static int tracing_sched_register(void)
 
 	return ret;
 fail_deprobe_wake_new:
-	marker_probe_unregister("kernel_sched_wakeup_new",
-				wake_up_callback,
-				&ctx_trace);
+	unregister_trace_sched_wakeup_new(probe_sched_wakeup);
 fail_deprobe:
-	marker_probe_unregister("kernel_sched_wakeup",
-				wake_up_callback,
-				&ctx_trace);
+	unregister_trace_sched_wakeup(probe_sched_wakeup);
 	return ret;
 }
 
 static void tracing_sched_unregister(void)
 {
-	marker_probe_unregister("kernel_sched_schedule",
-				sched_switch_callback,
-				&ctx_trace);
-	marker_probe_unregister("kernel_sched_wakeup_new",
-				wake_up_callback,
-				&ctx_trace);
-	marker_probe_unregister("kernel_sched_wakeup",
-				wake_up_callback,
-				&ctx_trace);
+	unregister_trace_sched_switch(probe_sched_switch);
+	unregister_trace_sched_wakeup_new(probe_sched_wakeup);
+	unregister_trace_sched_wakeup(probe_sched_wakeup);
 }
 
 static void tracing_start_sched_switch(void)

kernel/trace/trace_sched_wakeup.c +31 ~~-98~~

239 lines not on this page · raw message

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Takashi Nishiie

2008-07-10 03:14 UTC #42 lore raw

RE: [PATCH] ftrace port to tracepoints (linux-next)

Hi,Mathieu

I think that it is wonderful that the source code becomes simple by changing kernel markers to tracepoints.

However, it seems to forget to correct the error message.

For example.
Mathieu Wrote:

Quoted 10 lines

kernel/trace/trace_sched_switch.c +1 -4

-	ret = marker_probe_register("kernel_sched_wakeup",
-			"pid %d state %ld ## rq %p task %p rq->curr %p",
-			wake_up_callback,
-			&ctx_trace);
+	ret = register_trace_sched_wakeup(probe_sched_wakeup);

	if (ret) {
		pr_info("wakeup trace: Couldn't add marker"
			" probe to kernel_sched_wakeup\n");
		return ret;
	}

if (ret) {
- pr_info("wakeup trace: Couldn't add marker"
+ pr_info("wakeup trace: Couldn't activate tracepoint"
" probe to kernel_sched_wakeup\n");
return ret;
}

Thank you,
--
Takashi Nishiie

Mathieu Desnoyers

author

2008-07-10 03:57 UTC #43 lore raw

Re: [PATCH] ftrace port to tracepoints (linux-next) (nitpick update)

Quoted 7 lines · * Takashi Nishiie ([email protected]) wrote:

Hi,Mathieu
I think that it is wonderful that the source code becomes simple by changing kernel markers to tracepoints.
However, it seems to forget to correct the error message.

Good catch. Here is the revised version. Thanks!

Mathieu

ftrace port to tracepoints

Porting the trace_mark() used by ftrace to tracepoints. (cleanup)

Changelog :
- Change error messages : marker -> tracepoint

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/trace/trace_sched_switch.c |  120 ++++++---------------------------
 kernel/trace/trace_sched_wakeup.c |  135 +++++++++-----------------------------
 2 files changed, 58 insertions(+), 197 deletions(-)

kernel/trace/trace_sched_switch.c +24 ~~-96~~

Index: linux-2.6-lttng/kernel/trace/trace_sched_switch.c
===================================================================
@@ -9,9 +9,9 @@
 #include <linux/debugfs.h>
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
-#include <linux/marker.h>
 #include <linux/ftrace.h>
 
+#include "../sched-trace.h"
 #include "trace.h"
 
 static struct trace_array	*ctx_trace;

@@ -19,16 +19,17 @@ static int __read_mostly	tracer_enabled;
 static atomic_t			sched_ref;
 
 static void
-sched_switch_func(void *private, void *__rq, struct task_struct *prev,
+probe_sched_switch(struct rq *__rq, struct task_struct *prev,
 			struct task_struct *next)
 {
-	struct trace_array **ptr = private;
-	struct trace_array *tr = *ptr;
 	struct trace_array_cpu *data;
 	unsigned long flags;
 	long disabled;
 	int cpu;
 
+	if (!atomic_read(&sched_ref))
+		return;
+
 	tracing_record_cmdline(prev);
 	tracing_record_cmdline(next);

@@ -37,95 +38,42 @@ sched_switch_func(void *private, void *_
 
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
-	data = tr->data[cpu];
+	data = ctx_trace->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);
 
 	if (likely(disabled == 1))
-		tracing_sched_switch_trace(tr, data, prev, next, flags);
+		tracing_sched_switch_trace(ctx_trace, data, prev, next, flags);
 
 	atomic_dec(&data->disabled);
 	local_irq_restore(flags);
 }
 
-static notrace void
-sched_switch_callback(void *probe_data, void *call_data,
-		      const char *format, va_list *args)
-{
-	struct task_struct *prev;
-	struct task_struct *next;
-	struct rq *__rq;
-
-	if (!atomic_read(&sched_ref))
-		return;
-
-	/* skip prev_pid %d next_pid %d prev_state %ld */
-	(void)va_arg(*args, int);
-	(void)va_arg(*args, int);
-	(void)va_arg(*args, long);
-	__rq = va_arg(*args, typeof(__rq));
-	prev = va_arg(*args, typeof(prev));
-	next = va_arg(*args, typeof(next));
-
-	/*
-	 * If tracer_switch_func only points to the local
-	 * switch func, it still needs the ptr passed to it.
-	 */
-	sched_switch_func(probe_data, __rq, prev, next);
-}
-
 static void
-wakeup_func(void *private, void *__rq, struct task_struct *wakee, struct
-			task_struct *curr)
+probe_sched_wakeup(struct rq *__rq, struct task_struct *wakee)
 {
-	struct trace_array **ptr = private;
-	struct trace_array *tr = *ptr;
 	struct trace_array_cpu *data;
 	unsigned long flags;
 	long disabled;
 	int cpu;
 
-	if (!tracer_enabled)
+	if (!likely(tracer_enabled))
 		return;
 
-	tracing_record_cmdline(curr);
+	tracing_record_cmdline(current);
 
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
-	data = tr->data[cpu];
+	data = ctx_trace->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);
 
 	if (likely(disabled == 1))
-		tracing_sched_wakeup_trace(tr, data, wakee, curr, flags);
+		tracing_sched_wakeup_trace(ctx_trace, data, wakee, current,
+			flags);
 
 	atomic_dec(&data->disabled);
 	local_irq_restore(flags);
 }
 
-static notrace void
-wake_up_callback(void *probe_data, void *call_data,
-		 const char *format, va_list *args)
-{
-	struct task_struct *curr;
-	struct task_struct *task;
-	struct rq *__rq;
-
-	if (likely(!tracer_enabled))
-		return;
-
-	/* Skip pid %d state %ld */
-	(void)va_arg(*args, int);
-	(void)va_arg(*args, long);
-	/* now get the meat: "rq %p task %p rq->curr %p" */
-	__rq = va_arg(*args, typeof(__rq));
-	task = va_arg(*args, typeof(task));
-	curr = va_arg(*args, typeof(curr));
-
-	tracing_record_cmdline(task);
-	tracing_record_cmdline(curr);
-
-	wakeup_func(probe_data, __rq, task, curr);
-}
-
 static void sched_switch_reset(struct trace_array *tr)
 {
 	int cpu;

@@ -140,60 +88,40 @@ static int tracing_sched_register(void)
 {
 	int ret;
 
-	ret = marker_probe_register("kernel_sched_wakeup",
-			"pid %d state %ld ## rq %p task %p rq->curr %p",
-			wake_up_callback,
-			&ctx_trace);
+	ret = register_trace_sched_wakeup(probe_sched_wakeup);
 	if (ret) {
-		pr_info("wakeup trace: Couldn't add marker"
+		pr_info("wakeup trace: Couldn't activate tracepoint"
 			" probe to kernel_sched_wakeup\n");
 		return ret;
 	}
 
-	ret = marker_probe_register("kernel_sched_wakeup_new",
-			"pid %d state %ld ## rq %p task %p rq->curr %p",
-			wake_up_callback,
-			&ctx_trace);
+	ret = register_trace_sched_wakeup_new(probe_sched_wakeup);
 	if (ret) {
-		pr_info("wakeup trace: Couldn't add marker"
+		pr_info("wakeup trace: Couldn't activate tracepoint"
 			" probe to kernel_sched_wakeup_new\n");
 		goto fail_deprobe;
 	}
 
-	ret = marker_probe_register("kernel_sched_schedule",
-		"prev_pid %d next_pid %d prev_state %ld "
-		"## rq %p prev %p next %p",
-		sched_switch_callback,
-		&ctx_trace);
+	ret = register_trace_sched_switch(probe_sched_switch);
 	if (ret) {
-		pr_info("sched trace: Couldn't add marker"
+		pr_info("sched trace: Couldn't activate tracepoint"
 			" probe to kernel_sched_schedule\n");
 		goto fail_deprobe_wake_new;
 	}
 
 	return ret;
 fail_deprobe_wake_new:
-	marker_probe_unregister("kernel_sched_wakeup_new",
-				wake_up_callback,
-				&ctx_trace);
+	unregister_trace_sched_wakeup_new(probe_sched_wakeup);
 fail_deprobe:
-	marker_probe_unregister("kernel_sched_wakeup",
-				wake_up_callback,
-				&ctx_trace);
+	unregister_trace_sched_wakeup(probe_sched_wakeup);
 	return ret;
 }
 
 static void tracing_sched_unregister(void)
 {
-	marker_probe_unregister("kernel_sched_schedule",
-				sched_switch_callback,
-				&ctx_trace);
-	marker_probe_unregister("kernel_sched_wakeup_new",
-				wake_up_callback,
-				&ctx_trace);
-	marker_probe_unregister("kernel_sched_wakeup",
-				wake_up_callback,
-				&ctx_trace);
+	unregister_trace_sched_switch(probe_sched_switch);
+	unregister_trace_sched_wakeup_new(probe_sched_wakeup);
+	unregister_trace_sched_wakeup(probe_sched_wakeup);
 }
 
 static void tracing_start_sched_switch(void)

kernel/trace/trace_sched_wakeup.c +34 ~~-101~~

244 lines not on this page · raw message

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

A message that is not in the archive (sent off-list or not yet received): 20080711143709.GB11500@Krystal

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20080714153334.GA651@Krystal

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mathieu Desnoyers

author

2008-07-14 16:25 UTC #48 lore raw

Re: [PATCH] ftrace memory barriers

(CC'ed LKML)

Quoted 13 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 6 lines · Mathieu Desnoyers
Thanks a lot Steve! By the way, you might be interested in looking at the extra tracing enable/disabled tests you have in ftrace : it should be enough to register/unregister the probes to activate/deactivate tracing. I don't see why you would need if() checks in the probe itself.
That was actually one of the causes of the bugs I had. We had two markers, one tracing schedule switches, the other wake ups. As soon as one is enabled, tracing starts, and you will miss the tracing of the other one.

Quoted 9 lines

The result was a strange trace were things were waking up then the waker was being scheduled in??
I need to enable all markers at the same time, which can be done with the if () check. Not to mention, I like to be able to shut of tracing on oops without needing to do anyting drastic (like deactivating probes).
-- Steve

I see the reason why you need this, but beware that the activation boolean you are using in your check won't be seen as active at the same time by all CPUs because of out-of-order read/writes. So while the update is per se atomic, the replication of the updated value to all processors isn't. However, when one CPU sees the new version, this version will stay valid.

This is one of the reasons why I don't aim to provide atomic activation of multiple markers : thinking that the state will be atomically changed on all CPUs without using the proper memory barriers is a bit bogus anyway. Does ftrace expect such atomic propagation of the change across all CPUs or does it deal with the information it receives on a per-cpu basis ?

In any case, you should also make sure that you call synchronize_sched() after the probe registration to make sure the quiescent state is reached and that all probes are connected (so that no marker still see the old data structures). This should be done before you activate your boolean in the probes.

I agree that the extra check is very well suited to the OOPS case.

Mathieu

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Steven Rostedt

maintainer

2008-07-14 16:35 UTC #49 lore raw

Re: [PATCH] ftrace memory barriers

Quoted 27 lines · Mathieu Desnoyers

(CC'ed LKML)
Quoted 13 lines · * Steven Rostedt ([email protected]) wrote:
Quoted 6 lines · Mathieu Desnoyers
Thanks a lot Steve! By the way, you might be interested in looking at the extra tracing enable/disabled tests you have in ftrace : it should be enough to register/unregister the probes to activate/deactivate tracing. I don't see why you would need if() checks in the probe itself.
That was actually one of the causes of the bugs I had. We had two markers, one tracing schedule switches, the other wake ups. As soon as one is enabled, tracing starts, and you will miss the tracing of the other one.
Quoted 9 lines
The result was a strange trace were things were waking up then the waker was being scheduled in??
I need to enable all markers at the same time, which can be done with the if () check. Not to mention, I like to be able to shut of tracing on oops without needing to do anyting drastic (like deactivating probes).
-- Steve

Quoted 6 lines

I see the reason why you need this, but beware that the activation boolean you are using in your check won't be seen as active at the same time by all CPUs because of out-of-order read/writes. So while the update is per se atomic, the replication of the updated value to all processors isn't. However, when one CPU sees the new version, this version will stay valid.

Yep, I understand that. It was that the same CPU trace provided funny output. It it were a trace over separate CPUS then that is something people would accept.

Quoted 7 lines

This is one of the reasons why I don't aim to provide atomic activation of multiple markers : thinking that the state will be atomically changed on all CPUs without using the proper memory barriers is a bit bogus anyway. Does ftrace expect such atomic propagation of the change across all CPUs or does it deal with the information it receives on a per-cpu basis ?

All the buffers and traces are per-cpu. We sort out the rest on output, and we use the clock as the interleaver. Doing that doesn't guarantee that events actually do happen in the order that appear, but it gives one a good idea of where to look.

Quoted 6 lines

In any case, you should also make sure that you call synchronize_sched() after the probe registration to make sure the quiescent state is reached and that all probes are connected (so that no marker still see the old data structures). This should be done before you activate your boolean in the probes.

Good to know, thanks. Hmm, I'm not sure that code can schedule :-/ Maybe it can. I'll have to take a look.

Quoted 2 lines

I agree that the extra check is very well suited to the OOPS case.

Thanks,

-- Steve

Mathieu Desnoyers

author

2008-07-09 15:02 UTC #50 lore raw

PATCH 6/15 LTTng instrumentation - timer

Instrument timer activity (timer set, expired, current time updates) to keep information about the "real time" flow within the kernel. It can be used by a trace analysis tool to synchronize information coming from various sources, e.g. to merge traces with system logs.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC "David S. Miller" <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/itimer.c      |    5 +++++
 kernel/timer-trace.h |   24 ++++++++++++++++++++++++
 kernel/timer.c       |    8 +++++++-
 3 files changed, 36 insertions(+), 1 deletion(-)

kernel/itimer.c +5 -0

Index: linux-2.6-lttng/kernel/itimer.c
===================================================================
@@ -12,6 +12,7 @@
 #include <linux/time.h>
 #include <linux/posix-timers.h>
 #include <linux/hrtimer.h>
+#include "timer-trace.h"
 
 #include <asm/uaccess.h>

@@ -132,6 +133,8 @@ enum hrtimer_restart it_real_fn(struct h
 	struct signal_struct *sig =
 		container_of(timer, struct signal_struct, real_timer);
 
+	trace_timer_itimer_expired(sig);
+
 	kill_pid_info(SIGALRM, SEND_SIG_PRIV, sig->leader_pid);
 
 	return HRTIMER_NORESTART;

@@ -157,6 +160,8 @@ int do_setitimer(int which, struct itime
 	    !timeval_valid(&value->it_interval))
 		return -EINVAL;
 
+	trace_timer_itimer_set(which, value);
+
 	switch (which) {
 	case ITIMER_REAL:
 again:

kernel/timer.c +7 -1

Index: linux-2.6-lttng/kernel/timer.c
===================================================================
@@ -37,12 +37,14 @@
 #include <linux/delay.h>
 #include <linux/tick.h>
 #include <linux/kallsyms.h>
+#include "timer-trace.h"
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
 #include <asm/div64.h>
 #include <asm/timex.h>
 #include <asm/io.h>
+#include <asm/irq_regs.h>
 
 u64 jiffies_64 __cacheline_aligned_in_smp = INITIAL_JIFFIES;

@@ -288,6 +290,7 @@ static void internal_add_timer(struct tv
 		i = (expires >> (TVR_BITS + 3 * TVN_BITS)) & TVN_MASK;
 		vec = base->tv5.vec + i;
 	}
+	trace_timer_set(timer);
 	/*
 	 * Timers are FIFO:
 	 */

@@ -1066,6 +1069,7 @@ void do_timer(unsigned long ticks)
 {
 	jiffies_64 += ticks;
 	update_times(ticks);
+	trace_timer_update_time(&xtime, &wall_to_monotonic);
 }
 
 #ifdef __ARCH_WANT_SYS_ALARM

@@ -1147,7 +1151,9 @@ asmlinkage long sys_getegid(void)
 
 static void process_timeout(unsigned long __data)
 {
-	wake_up_process((struct task_struct *)__data);
+	struct task_struct *task = (struct task_struct *)__data;
+	trace_timer_timeout(task);
+	wake_up_process(task);
 }
 
 /**

kernel/timer-trace.h new file +24 -0

Index: linux-2.6-lttng/kernel/timer-trace.h
===================================================================
@@ -0,0 +1,24 @@
+#ifndef _TIMER_TRACE_H
+#define _TIMER_TRACE_H
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(timer_itimer_expired,
+	TPPROTO(struct signal_struct *sig),
+	TPARGS(sig));
+DEFINE_TRACE(timer_itimer_set,
+	TPPROTO(int which, struct itimerval *value),
+	TPARGS(which, value));
+DEFINE_TRACE(timer_set,
+	TPPROTO(struct timer_list *timer),
+	TPARGS(timer));
+/*
+ * xtime_lock is taken when kernel_timer_update_time tracepoint is reached.
+ */
+DEFINE_TRACE(timer_update_time,
+	TPPROTO(struct timespec *_xtime, struct timespec *_wall_to_monotonic),
+	TPARGS(_xtime, _wall_to_monotonic));
+DEFINE_TRACE(timer_timeout,
+	TPPROTO(struct task_struct *p),
+	TPARGS(p));
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:03 UTC #51 lore raw

PATCH 7/15 LTTng instrumentation - kernel

Instrument the core kernel : module load/free and printk events. It helps the tracer to keep track of module related events and to export valuable printk information into the traces.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Andrew Morton <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 kernel/kernel-trace.h |   19 +++++++++++++++++++
 kernel/module.c       |    5 +++++
 kernel/printk.c       |    6 ++++++
 3 files changed, 30 insertions(+)

kernel/printk.c +6 -0

Index: linux-2.6-lttng/kernel/printk.c
===================================================================
@@ -32,6 +32,7 @@
 #include <linux/security.h>
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
+#include "kernel-trace.h"
 
 #include <asm/uaccess.h>

@@ -59,6 +60,7 @@ int console_printk[4] = {
 	MINIMUM_CONSOLE_LOGLEVEL,	/* minimum_console_loglevel */
 	DEFAULT_CONSOLE_LOGLEVEL,	/* default_console_loglevel */
 };
+EXPORT_SYMBOL_GPL(console_printk);
 
 /*
  * Low level drivers may need that to know if they can schedule in

@@ -601,6 +603,7 @@ asmlinkage int printk(const char *fmt, .
 	int r;
 
 	va_start(args, fmt);
+	trace_kernel_printk(__builtin_return_address(0));
 	r = vprintk(fmt, args);
 	va_end(args);

@@ -677,6 +680,9 @@ asmlinkage int vprintk(const char *fmt, 
 	raw_local_irq_save(flags);
 	this_cpu = smp_processor_id();
 
+	trace_kernel_vprintk(__builtin_return_address(0),
+		printk_buf, printed_len);
+
 	/*
 	 * Ouch, printk recursed into itself!
 	 */

kernel/module.c +5 -0

Index: linux-2.6-lttng/kernel/module.c
===================================================================
@@ -48,6 +48,7 @@
 #include <linux/license.h>
 #include <asm/sections.h>
 #include <linux/tracepoint.h>
+#include "kernel-trace.h"
 
 #if 0
 #define DEBUGP printk

@@ -1422,6 +1423,8 @@ static int __unlink_module(void *_mod)
 /* Free a module, remove from lists, etc (must hold module_mutex). */
 static void free_module(struct module *mod)
 {
+	trace_kernel_module_free(mod);
+
 	/* Delete from various lists */
 	stop_machine_run(__unlink_module, mod, NULL);
 	remove_notes_attrs(mod);

@@ -2237,6 +2240,8 @@ static struct module *load_module(void _
 	/* Get rid of temporary copy */
 	vfree(hdr);
 
+	trace_kernel_module_load(mod);
+
 	/* Done! */
 	return mod;

kernel/kernel-trace.h new file +19 -0

Index: linux-2.6-lttng/kernel/kernel-trace.h
===================================================================
@@ -0,0 +1,19 @@
+#ifndef _KERNEL_TRACE_H
+#define _KERNEL_TRACE_H
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(kernel_printk,
+	TPPROTO(void *retaddr),
+	TPARGS(retaddr));
+DEFINE_TRACE(kernel_vprintk,
+	TPPROTO(void *retaddr, char *buf, int len),
+	TPARGS(retaddr, buf, len));
+DEFINE_TRACE(kernel_module_free,
+	TPPROTO(struct module *mod),
+	TPARGS(mod));
+DEFINE_TRACE(kernel_module_load,
+	TPPROTO(struct module *mod),
+	TPARGS(mod));
+
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 14:59 UTC #52 lore raw

PATCH 8/15 LTTng instrumentation - filemap

Instrumentation of waits caused by memory accesses on mmap regions.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Dave Hansen <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 mm/filemap-trace.h |   13 +++++++++++++
 mm/filemap.c       |    3 +++
 2 files changed, 16 insertions(+)

mm/filemap.c +3 -0

Index: linux-2.6-lttng/mm/filemap.c
===================================================================
@@ -33,6 +33,7 @@
 #include <linux/cpuset.h>
 #include <linux/hardirq.h> /* for BUG_ON(!in_atomic()) only */
 #include <linux/memcontrol.h>
+#include "filemap-trace.h"
 #include "internal.h"
 
 /*

@@ -541,9 +542,11 @@ void wait_on_page_bit(struct page *page,
 {
 	DEFINE_WAIT_BIT(wait, &page->flags, bit_nr);
 
+	trace_filemap_wait_start(page, bit_nr);
 	if (test_bit(bit_nr, &page->flags))
 		__wait_on_bit(page_waitqueue(page), &wait, sync_page,
 							TASK_UNINTERRUPTIBLE);
+	trace_filemap_wait_end(page, bit_nr);
 }
 EXPORT_SYMBOL(wait_on_page_bit);

mm/filemap-trace.h new file +13 -0

Index: linux-2.6-lttng/mm/filemap-trace.h
===================================================================
@@ -0,0 +1,13 @@
+#ifndef _FILEMAP_TRACE_H
+#define _FILEMAP_TRACE_H
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(filemap_wait_start,
+	TPPROTO(struct page *page, int bit_nr),
+	TPARGS(page, bit_nr));
+DEFINE_TRACE(filemap_wait_end,
+	TPPROTO(struct page *page, int bit_nr),
+	TPARGS(page, bit_nr));
+
+#endif

Signature · 9 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mathieu Desnoyers

author

2008-07-09 14:59 UTC #53 lore raw

PATCH 9/15 LTTng instrumentation - swap

Instrumentation of waits caused by swap activity. Also instrumentation swapon/swapoff events to keep track of active swap partitions.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Dave Hansen <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 mm/memory.c     |    2 ++
 mm/page_io.c    |    2 ++
 mm/swap-trace.h |   20 ++++++++++++++++++++
 mm/swapfile.c   |    4 ++++
 4 files changed, 28 insertions(+)

mm/memory.c +2 -0

Index: linux-2.6-lttng/mm/memory.c
===================================================================
@@ -51,6 +51,7 @@
 #include <linux/init.h>
 #include <linux/writeback.h>
 #include <linux/memcontrol.h>
+#include "swap-trace.h"
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>

@@ -2213,6 +2214,7 @@ static int do_swap_page(struct mm_struct
 		/* Had to read the page from swap area: Major fault */
 		ret = VM_FAULT_MAJOR;
 		count_vm_event(PGMAJFAULT);
+		trace_swap_in(page, entry);
 	}
 
 	if (mem_cgroup_charge(page, mm, GFP_KERNEL)) {

mm/page_io.c +2 -0

Index: linux-2.6-lttng/mm/page_io.c
===================================================================
@@ -17,6 +17,7 @@
 #include <linux/bio.h>
 #include <linux/swapops.h>
 #include <linux/writeback.h>
+#include "swap-trace.h"
 #include <asm/pgtable.h>
 
 static struct bio *get_swap_bio(gfp_t gfp_flags, pgoff_t index,

@@ -114,6 +115,7 @@ int swap_writepage(struct page *page, st
 		rw |= (1 << BIO_RW_SYNC);
 	count_vm_event(PSWPOUT);
 	set_page_writeback(page);
+	trace_swap_out(page);
 	unlock_page(page);
 	submit_bio(rw, bio);
 out:

mm/swapfile.c +4 -0

Index: linux-2.6-lttng/mm/swapfile.c
===================================================================
@@ -32,6 +32,7 @@
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 #include <linux/swapops.h>
+#include "swap-trace.h"
 
 DEFINE_SPINLOCK(swap_lock);
 unsigned int nr_swapfiles;

@@ -1310,6 +1311,7 @@ asmlinkage long sys_swapoff(const char _
 	swap_map = p->swap_map;
 	p->swap_map = NULL;
 	p->flags = 0;
+	trace_swap_file_close(swap_file);
 	spin_unlock(&swap_lock);
 	mutex_unlock(&swapon_mutex);
 	vfree(swap_map);

@@ -1695,6 +1697,7 @@ asmlinkage long sys_swapon(const char __
 	} else {
 		swap_info[prev].next = p - swap_info;
 	}
+	trace_swap_file_open(swap_file, name);
 	spin_unlock(&swap_lock);
 	mutex_unlock(&swapon_mutex);
 	error = 0;

@@ -1796,6 +1799,7 @@ get_swap_info_struct(unsigned type)
 {
 	return &swap_info[type];
 }
+EXPORT_SYMBOL_GPL(get_swap_info_struct);
 
 /*
  * swap_lock prevents swap_map being freed. Don't grab an extra

mm/swap-trace.h new file +20 -0

Index: linux-2.6-lttng/mm/swap-trace.h
===================================================================
@@ -0,0 +1,20 @@
+#ifndef _SWAP_TRACE_H
+#define _SWAP_TRACE_H
+
+#include <linux/swap.h>
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(swap_in,
+	TPPROTO(struct page *page, swp_entry_t entry),
+	TPARGS(page, entry));
+DEFINE_TRACE(swap_out,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(swap_file_open,
+	TPPROTO(struct file *file, char *filename),
+	TPARGS(file, filename));
+DEFINE_TRACE(swap_file_close,
+	TPPROTO(struct file *file),
+	TPARGS(file));
+
+#endif

Signature · 9 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mathieu Desnoyers

author

2008-07-09 14:59 UTC #54 lore raw

PATCH 10/15 LTTng instrumentation - memory page faults

Instrument the page fault entry and exit. Useful to detect delays caused by page faults and bad memory usage patterns.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Andi Kleen <[email protected]>

CC Dave Hansen <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 mm/memory-trace.h |   14 ++++++++++++++
 mm/memory.c       |   33 ++++++++++++++++++++++++---------
 2 files changed, 38 insertions(+), 9 deletions(-)

mm/memory.c +24 -9

Index: linux-2.6-lttng/mm/memory.c
===================================================================
@@ -61,6 +61,7 @@
 
 #include <linux/swapops.h>
 #include <linux/elf.h>
+#include "memory-trace.h"
 
 #ifndef CONFIG_NEED_MULTIPLE_NODES
 /* use the per-pgdat data instead for discontigmem - mbligh */

@@ -2664,30 +2665,44 @@ unlock:
 int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 		unsigned long address, int write_access)
 {
+	int res;
 	pgd_t *pgd;
 	pud_t *pud;
 	pmd_t *pmd;
 	pte_t *pte;
 
+	trace_memory_handle_fault_entry(mm, vma, address, write_access);
+
 	__set_current_state(TASK_RUNNING);
 
 	count_vm_event(PGFAULT);
 
-	if (unlikely(is_vm_hugetlb_page(vma)))
-		return hugetlb_fault(mm, vma, address, write_access);
+	if (unlikely(is_vm_hugetlb_page(vma))) {
+		res = hugetlb_fault(mm, vma, address, write_access);
+		goto end;
+	}
 
 	pgd = pgd_offset(mm, address);
 	pud = pud_alloc(mm, pgd, address);
-	if (!pud)
-		return VM_FAULT_OOM;
+	if (!pud) {
+		res = VM_FAULT_OOM;
+		goto end;
+	}
 	pmd = pmd_alloc(mm, pud, address);
-	if (!pmd)
-		return VM_FAULT_OOM;
+	if (!pmd) {
+		res = VM_FAULT_OOM;
+		goto end;
+	}
 	pte = pte_alloc_map(mm, pmd, address);
-	if (!pte)
-		return VM_FAULT_OOM;
+	if (!pte) {
+		res = VM_FAULT_OOM;
+		goto end;
+	}
 
-	return handle_pte_fault(mm, vma, address, pte, pmd, write_access);
+	res = handle_pte_fault(mm, vma, address, pte, pmd, write_access);
+end:
+	trace_memory_handle_fault_exit(res);
+	return res;
 }
 
 #ifndef __PAGETABLE_PUD_FOLDED

mm/memory-trace.h new file +14 -0

Index: linux-2.6-lttng/mm/memory-trace.h
===================================================================
@@ -0,0 +1,14 @@
+#ifndef _MEMORY_TRACE_H
+#define _MEMORY_TRACE_H
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(memory_handle_fault_entry,
+	TPPROTO(struct mm_struct *mm, struct vm_area_struct *vma,
+		unsigned long address, int write_access),
+	TPARGS(mm, vma, address, write_access));
+DEFINE_TRACE(memory_handle_fault_exit,
+	TPPROTO(int res),
+	TPARGS(res));
+
+#endif

Signature · 9 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mathieu Desnoyers

author

2008-07-09 15:03 UTC #55 lore raw

PATCH 11/15 LTTng instrumentation - page

Paging activity instrumentation. Instruments page allocation/free to keep track of page allocation. This does not cover hugetlb activity, which is covered by a separate patch.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Martin Bligh <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 mm/page-trace.h |   16 ++++++++++++++++
 mm/page_alloc.c |    6 ++++++
 2 files changed, 22 insertions(+)

mm/page_alloc.c +6 -0

Index: linux-2.6-lttng/mm/page_alloc.c
===================================================================
@@ -46,6 +46,7 @@
 #include <linux/page-isolation.h>
 #include <linux/memcontrol.h>
 #include <linux/debugobjects.h>
+#include "page-trace.h"
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>

@@ -510,6 +511,8 @@ static void __free_pages_ok(struct page 
 	int i;
 	int reserved = 0;
 
+	trace_page_free(page, order);
+
 	for (i = 0 ; i < (1 << order) ; ++i)
 		reserved += free_pages_check(page + i);
 	if (reserved)

@@ -966,6 +969,8 @@ static void free_hot_cold_page(struct pa
 	struct per_cpu_pages *pcp;
 	unsigned long flags;
 
+	trace_page_free(page, 0);
+
 	if (PageAnon(page))
 		page->mapping = NULL;
 	if (free_pages_check(page))

@@ -1630,6 +1635,7 @@ nopage:
 		show_mem();
 	}
 got_pg:
+	trace_page_alloc(page, order);
 	return page;
 }

mm/page-trace.h new file +16 -0

Index: linux-2.6-lttng/mm/page-trace.h
===================================================================
@@ -0,0 +1,16 @@
+#ifndef _PAGE_TRACE_H
+#define _PAGE_TRACE_H
+
+#include <linux/tracepoint.h>
+
+/*
+ * mm_page_alloc : page can be NULL.
+ */
+DEFINE_TRACE(page_alloc,
+	TPPROTO(struct page *page, unsigned int order),
+	TPARGS(page, order));
+DEFINE_TRACE(page_free,
+	TPPROTO(struct page *page, unsigned int order),
+	TPARGS(page, order));
+
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:05 UTC #56 lore raw

PATCH 12/15 LTTng instrumentation - hugetlb

Instrumentation of hugetlb activity (alloc/free/reserve).

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC William Lee Irwin III <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 mm/hugetlb-trace.h |   19 +++++++++++++++++++
 mm/hugetlb.c       |    8 +++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

mm/hugetlb.c +7 -1

Index: linux-2.6-lttng/mm/hugetlb.c
===================================================================
@@ -14,6 +14,7 @@
 #include <linux/mempolicy.h>
 #include <linux/cpuset.h>
 #include <linux/mutex.h>
+#include "hugetlb-trace.h"
 
 #include <asm/page.h>
 #include <asm/pgtable.h>

@@ -141,6 +142,7 @@ static void free_huge_page(struct page *
 	int nid = page_to_nid(page);
 	struct address_space *mapping;
 
+	trace_hugetlb_page_free(page);
 	mapping = (struct address_space *) page_private(page);
 	set_page_private(page, 0);
 	BUG_ON(page_count(page));

@@ -509,6 +511,7 @@ static struct page *alloc_huge_page(stru
 	if (!IS_ERR(page)) {
 		set_page_refcounted(page);
 		set_page_private(page, (unsigned long) mapping);
+		trace_hugetlb_page_alloc(page);
 	}
 	return page;
 }

@@ -1306,13 +1309,16 @@ int hugetlb_reserve_pages(struct inode *
 		return ret;
 	}
 	region_add(&inode->i_mapping->private_list, from, to);
+	trace_hugetlb_pages_reserve(inode, from, to);
 	return 0;
 }
 
 void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)
 {
-	long chg = region_truncate(&inode->i_mapping->private_list, offset);
+	long chg;
 
+	trace_hugetlb_pages_unreserve(inode, offset, freed);
+	chg = region_truncate(&inode->i_mapping->private_list, offset);
 	spin_lock(&inode->i_lock);
 	inode->i_blocks -= BLOCKS_PER_HUGEPAGE * freed;
 	spin_unlock(&inode->i_lock);

mm/hugetlb-trace.h new file +19 -0

Index: linux-2.6-lttng/mm/hugetlb-trace.h
===================================================================
@@ -0,0 +1,19 @@
+#ifndef _HUGETLB_TRACE_H
+#define _HUGETLB_TRACE_H
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(hugetlb_page_alloc,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_page_free,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_pages_reserve,
+	TPPROTO(struct inode *inode, long from, long to),
+	TPARGS(inode, from, to));
+DEFINE_TRACE(hugetlb_pages_unreserve,
+	TPPROTO(struct inode *inode, long offset, long freed),
+	TPARGS(inode, offset, freed));
+
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-11 14:30 UTC #57 lore raw

Re: [patch 12/15] LTTng instrumentation - hugetlb (update)

Instrumentation of hugetlb activity (alloc/free/reserve/grab/release).

Those tracepoints are used by LTTng.

Changelog :
- instrument page grab, buddy allocator alloc, page release.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC William Lee Irwin III <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 mm/hugetlb-trace.h |   28 ++++++++++++++++++++++++++++
 mm/hugetlb.c       |   41 +++++++++++++++++++++++++++++------------
 2 files changed, 57 insertions(+), 12 deletions(-)

mm/hugetlb.c +29 ~~-12~~

Index: linux-2.6-lttng/mm/hugetlb.c
===================================================================
@@ -14,6 +14,7 @@
 #include <linux/mempolicy.h>
 #include <linux/cpuset.h>
 #include <linux/mutex.h>
+#include "hugetlb-trace.h"
 
 #include <asm/page.h>
 #include <asm/pgtable.h>

@@ -123,6 +124,7 @@ static struct page *dequeue_huge_page_vm
 static void update_and_free_page(struct page *page)
 {
 	int i;
+	trace_hugetlb_page_release(page);
 	nr_huge_pages--;
 	nr_huge_pages_node[page_to_nid(page)]--;
 	for (i = 0; i < (HPAGE_SIZE / PAGE_SIZE); i++) {

@@ -141,6 +143,7 @@ static void free_huge_page(struct page *
 	int nid = page_to_nid(page);
 	struct address_space *mapping;
 
+	trace_hugetlb_page_free(page);
 	mapping = (struct address_space *) page_private(page);
 	set_page_private(page, 0);
 	BUG_ON(page_count(page));

@@ -205,7 +208,8 @@ static struct page *alloc_fresh_huge_pag
 	if (page) {
 		if (arch_prepare_hugepage(page)) {
 			__free_pages(page, HUGETLB_PAGE_ORDER);
-			return NULL;
+			page = NULL;
+			goto end;
 		}
 		set_compound_page_dtor(page, free_huge_page);
 		spin_lock(&hugetlb_lock);

@@ -214,7 +218,8 @@ static struct page *alloc_fresh_huge_pag
 		spin_unlock(&hugetlb_lock);
 		put_page(page); /* free it into the hugepage allocator */
 	}
-
+end:
+	trace_hugetlb_page_grab(page);
 	return page;
 }

@@ -288,7 +293,8 @@ static struct page *alloc_buddy_huge_pag
 	spin_lock(&hugetlb_lock);
 	if (surplus_huge_pages >= nr_overcommit_huge_pages) {
 		spin_unlock(&hugetlb_lock);
-		return NULL;
+		page = NULL;
+		goto end;
 	} else {
 		nr_huge_pages++;
 		surplus_huge_pages++;

@@ -321,7 +327,8 @@ static struct page *alloc_buddy_huge_pag
 		__count_vm_event(HTLB_BUDDY_PGALLOC_FAIL);
 	}
 	spin_unlock(&hugetlb_lock);
-
+end:
+	trace_hugetlb_buddy_pgalloc(page);
 	return page;
 }

@@ -510,6 +517,7 @@ static struct page *alloc_huge_page(stru
 		set_page_refcounted(page);
 		set_page_private(page, (unsigned long) mapping);
 	}
+	trace_hugetlb_page_alloc(page);
 	return page;
 }

@@ -1292,27 +1300,36 @@ out:
 
 int hugetlb_reserve_pages(struct inode *inode, long from, long to)
 {
-	long ret, chg;
+	int ret;
+	long chg;
 
 	chg = region_chg(&inode->i_mapping->private_list, from, to);
-	if (chg < 0)
-		return chg;
+	if (chg < 0) {
+		ret = chg;
+		goto end;
+	}
 
-	if (hugetlb_get_quota(inode->i_mapping, chg))
-		return -ENOSPC;
+	if (hugetlb_get_quota(inode->i_mapping, chg)) {
+		ret = -ENOSPC;
+		goto end;
+	}
 	ret = hugetlb_acct_memory(chg);
 	if (ret < 0) {
 		hugetlb_put_quota(inode->i_mapping, chg);
-		return ret;
+		goto end;
 	}
 	region_add(&inode->i_mapping->private_list, from, to);
-	return 0;
+end:
+	trace_hugetlb_pages_reserve(inode, from, to, ret);
+	return ret;
 }
 
 void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)
 {
-	long chg = region_truncate(&inode->i_mapping->private_list, offset);
+	long chg;
 
+	trace_hugetlb_pages_unreserve(inode, offset, freed);
+	chg = region_truncate(&inode->i_mapping->private_list, offset);
 	spin_lock(&inode->i_lock);
 	inode->i_blocks -= BLOCKS_PER_HUGEPAGE * freed;
 	spin_unlock(&inode->i_lock);

mm/hugetlb-trace.h new file +28 -0

Index: linux-2.6-lttng/mm/hugetlb-trace.h
===================================================================
@@ -0,0 +1,28 @@
+#ifndef _HUGETLB_TRACE_H
+#define _HUGETLB_TRACE_H
+
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(hugetlb_page_release,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_page_grab,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_buddy_pgalloc,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_page_alloc,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_page_free,
+	TPPROTO(struct page *page),
+	TPARGS(page));
+DEFINE_TRACE(hugetlb_pages_reserve,
+	TPPROTO(struct inode *inode, long from, long to, int ret),
+	TPARGS(inode, from, to, ret));
+DEFINE_TRACE(hugetlb_pages_unreserve,
+	TPPROTO(struct inode *inode, long offset, long freed),
+	TPARGS(inode, offset, freed));
+
+#endif

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 15:02 UTC #58 lore raw

PATCH 13/15 LTTng instrumentation - net

Network device activity instrumentation (xmit/receive). Allows to detect when a packet had arrived on the network card or when it is going to be sent. This is the instrumentation point outside of the drivers that is the closest to the hardware. It allows to detect the amount of time taken by a packet to go through the kernel between the system call and the actual delivery to the network card (given that system calls are instrumented).

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Andrew Morton <[email protected]>

CC Jeff Garzik <[email protected]>

CC Masami Hiramatsu <[email protected]>

CC 'Peter Zijlstra' <[email protected]>

CC "Frank Ch. Eigler" <[email protected]>

CC 'Ingo Molnar' <[email protected]>

CC 'Hideo AOKI' <[email protected]>

CC Takashi Nishiie <[email protected]>

CC 'Steven Rostedt' <[email protected]>

CC Eduard - Gabriel Munteanu <[email protected]>

---
 net/core/dev.c       |    4 ++++
 net/core/net-trace.h |   14 ++++++++++++++
 2 files changed, 18 insertions(+)

net/core/dev.c +4 -0

Index: linux-2.6-lttng/net/core/dev.c
===================================================================
@@ -122,6 +122,7 @@
 #include <linux/if_arp.h>
 #include <linux/if_vlan.h>
 
+#include "net-trace.h"
 #include "net-sysfs.h"
 
 /*

@@ -1699,6 +1700,8 @@ int dev_queue_xmit(struct sk_buff *skb)
 	}
 
 gso:
+	trace_net_dev_xmit(skb);
+
 	spin_lock_prefetch(&dev->queue_lock);
 
 	/* Disable soft irqs for various locks below. Also

@@ -2099,6 +2102,7 @@ int netif_receive_skb(struct sk_buff *sk
 
 	__get_cpu_var(netdev_rx_stat).total++;
 
+	trace_net_dev_receive(skb);
 	skb_reset_network_header(skb);
 	skb_reset_transport_header(skb);
 	skb->mac_len = skb->network_header - skb->mac_header;

net/core/net-trace.h new file +14 -0

Index: linux-2.6-lttng/net/core/net-trace.h
===================================================================
@@ -0,0 +1,14 @@
+#ifndef _NET_TRACE_H
+#define _NET_TRACE_H
+
+#include <net/sock.h>
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(net_dev_xmit,
+	TPPROTO(struct sk_buff *skb),
+	TPARGS(skb));
+DEFINE_TRACE(net_dev_receive,
+	TPPROTO(struct sk_buff *skb),
+	TPARGS(skb));
+
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

author

2008-07-09 14:59 UTC #59 lore raw

PATCH 14/15 LTTng instrumentation - ipv4

Keep track of interface up/down for ipv4. Allows to keep track of interface address changes in a trace.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC David S. Miller <[email protected]>

CC Alexey Kuznetsov <[email protected]>

CC:
CC: Masami Hiramatsu <[email protected]>
CC: 'Peter Zijlstra' <[email protected]>
CC: "Frank Ch. Eigler" <[email protected]>
CC: 'Ingo Molnar' <[email protected]>
CC: 'Hideo AOKI' <[email protected]>
CC: Takashi Nishiie <[email protected]>
CC: 'Steven Rostedt' <[email protected]>
CC: Eduard - Gabriel Munteanu <[email protected]>

---
 net/ipv4/devinet.c    |    3 +++
 net/ipv4/ipv4-trace.h |   14 ++++++++++++++
 2 files changed, 17 insertions(+)

net/ipv4/devinet.c +3 -0

Index: linux-2.6-lttng/net/ipv4/devinet.c
===================================================================
@@ -61,6 +61,7 @@
 #include <net/ip_fib.h>
 #include <net/rtnetlink.h>
 #include <net/net_namespace.h>
+#include "ipv4-trace.h"
 
 static struct ipv4_devconf ipv4_devconf = {
 	.data = {

@@ -257,6 +258,7 @@ static void __inet_del_ifa(struct in_dev
 		struct in_ifaddr **ifap1 = &ifa1->ifa_next;
 
 		while ((ifa = *ifap1) != NULL) {
+			trace_ipv4_addr_del(ifa);
 			if (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
 			    ifa1->ifa_scope <= ifa->ifa_scope)
 				last_prim = ifa;

@@ -363,6 +365,7 @@ static int __inet_insert_ifa(struct in_i
 			}
 			ifa->ifa_flags |= IFA_F_SECONDARY;
 		}
+		trace_ipv4_addr_add(ifa);
 	}
 
 	if (!(ifa->ifa_flags & IFA_F_SECONDARY)) {

net/ipv4/ipv4-trace.h new file +14 -0

Index: linux-2.6-lttng/net/ipv4/ipv4-trace.h
===================================================================
@@ -0,0 +1,14 @@
+#ifndef _IPV4_TRACE_H
+#define _IPV4_TRACE_H
+
+#include <linux/inetdevice.h>
+#include <linux/tracepoint.h>
+
+DEFINE_TRACE(ipv4_addr_add,
+	TPPROTO(struct in_ifaddr *ifa),
+	TPARGS(ifa));
+DEFINE_TRACE(ipv4_addr_del,
+	TPPROTO(struct in_ifaddr *ifa),
+	TPARGS(ifa));
+
+#endif

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Masami Hiramatsu

2008-07-09 17:03 UTC #60 lore raw

Hi Mathieu,

I couldn't find your 15th patch in my mailbox, neither lkml archive. Could you resend it?

Thank you,

Quoted 11 lines · Mathieu Desnoyers

Hi,
This is the 3rd round of tracepoints patch submisison. The instrumentation sites the more likely to come to a quick agreement has been selected in this first step. The tracepoint infrastructure, heavily inspired from the kernel markers, seems pretty solid and went through a thorough review by Masami.
This patchset applies over patch-v2.6.26-rc9-next-20080709.
Mathieu

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Mathieu Desnoyers

author

2008-07-09 17:11 UTC #61 lore raw

PATCH 15/15 LTTng instrumentation - ipv6

Quoted 7 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,
I couldn't find your 15th patch in my mailbox, neither lkml archive. Could you resend it?
Thank you,

Sure, I think quilt had some problem with it because of a ill formatted email. Here it is.

Instrument addr_add and del of network interfaces. Lets a tracer know the interface address changes.

Those tracepoints are used by LTTng.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Pekka Savola <[email protected]>