linux-toradex.git/kernel/marker.c, branch v2.6.27.27

markers: fix markers read barrier for multiple probes

2008-07-30T16:41:45+00:00

Paul pointed out two incorrect read barriers in the marker handler code in
the path where multiple probes are connected.  Those are ordering reads of
"ptype" (single or multi probe marker), "multi" array pointer, and "multi"
array data access.

It should be ordered like this :

read ptype
smp_rmb()
read multi array pointer
smp_read_barrier_depends()
access data referenced by multi array pointer

The code with a single probe connected (optimized case, does not have to
allocate an array) has correct memory ordering.

It applies to kernel 2.6.26.x, 2.6.25.x and linux-next.

Signed-off-by: Mathieu Desnoyers 
Cc: "Paul E. McKenney" 
Cc: 		[2.6.25.x, 2.6.26.x]
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

markers: use rcu_barrier_sched() and call_rcu_sched()

2008-07-25T17:53:45+00:00

rcu_barrier_sched() and call_rcu_sched() were introduced in 2.6.26 for the
Markers.  Change the marker code to use them.

It can be seen as a fix since the marker code was using an ugly,
temporary, #ifdef hack to work around CONFIG_PREEMPT_RCU.

Signed-off-by: Mathieu Desnoyers 
Acked-by: Paul McKenney 
Cc: Ingo Molnar 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

Markers - remove extra format argument

2008-05-23T20:25:27+00:00

Denys Vlasenko  :

> Not in this patch, but I noticed:
>
> #define __trace_mark(name, call_private, format, args...)               \
>         do {                                                            \
>                 static const char __mstrtab_##name[]                    \
>                 __attribute__((section("__markers_strings")))           \
>                 = #name "\0" format;                                    \
>                 static struct marker __mark_##name                      \
>                 __attribute__((section("__markers"), aligned(8))) =     \
>                 { __mstrtab_##name, &__mstrtab_##name[sizeof(#name)],   \
>                 0, 0, marker_probe_cb,                                  \
>                 { __mark_empty_function, NULL}, NULL };                 \
>                 __mark_check_format(format, ## args);                   \
>                 if (unlikely(__mark_##name.state)) {                    \
>                         (*__mark_##name.call)                           \
>                                 (&__mark_##name, call_private,          \
>                                 format, ## args);                       \
>                 }                                                       \
>         } while (0)
>
> In this call:
>
>                         (*__mark_##name.call)                           \
>                                 (&__mark_##name, call_private,          \
>                                 format, ## args);                       \
>
> you make gcc allocate duplicate format string. You can use
> &__mstrtab_##name[sizeof(#name)] instead since it holds the same string,
> or drop ", format," above and "const char *fmt" from here:
>
>         void (*call)(const struct marker *mdata,        /* Probe wrapper */
>                 void *call_private, const char *fmt, ...);
>
> since mdata->format is the same and all callees which need it can take it there.

Very good point. I actually thought about dropping it, since it would
remove an unnecessary argument from the stack. And actually, since I now
have the marker_probe_cb sitting between the marker site and the
callbacks, there is no API change required. Thanks :)

Mathieu

Signed-off-by: Mathieu Desnoyers 
CC: Denys Vlasenko 
Signed-off-by: Ingo Molnar 
Signed-off-by: Thomas Gleixner

make marker_debug static

2008-04-30T15:29:49+00:00

With the needlessly global marker_debug being static gcc can optimize the
unused code away.

Signed-off-by: Adrian Bunk 
Acked-by: Mathieu Desnoyers 
Cc: Ingo Molnar 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

kernel: explicitly include required header files under kernel/

2008-04-29T15:06:04+00:00

Following an experimental deletion of the unnecessary directive

 #include 

from the header file , these files under kernel/ were exposed
as needing to include one of  or , so explicit
includes were added where necessary.

Signed-off-by: Robert P. J. Day 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

markers: use synchronize_sched()

2008-04-02T22:28:19+00:00

Markers do not mix well with CONFIG_PREEMPT_RCU because it uses
preempt_disable/enable() and not rcu_read_lock/unlock for minimal
intrusiveness.  We would need call_sched and sched_barrier primitives.

Currently, the modification (connection and disconnection) of probes
from markers requires changes to the data structure done in RCU-style :
a new data structure is created, the pointer is changed atomically, a
quiescent state is reached and then the old data structure is freed.

The quiescent state is reached once all the currently running
preempt_disable regions are done running.  We use the call_rcu mechanism
to execute kfree() after such quiescent state has been reached.
However, the new CONFIG_PREEMPT_RCU version of call_rcu and rcu_barrier
does not guarantee that all preempt_disable code regions have finished,
hence the race.

The "proper" way to do this is to use rcu_read_lock/unlock, but we don't
want to use it to minimize intrusiveness on the traced system.  (we do
not want the marker code to call into much of the OS code, because it
would quickly restrict what can and cannot be instrumented, such as the
scheduler).

The temporary fix, until we get call_rcu_sched and rcu_barrier_sched in
mainline, is to use synchronize_sched before each call_rcu calls, so we
wait for the quiescent state in the system call code path.  It will slow
down batch marker enable/disable, but will make sure the race is gone.

Signed-off-by: Mathieu Desnoyers 
Acked-by: Paul E. McKenney 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

markers: remove ACCESS_ONCE

2008-03-25T02:22:19+00:00

As Paul pointed out, the ACCESS_ONCE are not needed because we already have
the explicit surrounding memory barriers.

Signed-off-by: Mathieu Desnoyers 
Cc: Christoph Hellwig 
Cc: Mike Mason 
Cc: Dipankar Sarma 
Cc: David Smith 
Cc: "Paul E. McKenney" 
Cc: Steven Rostedt 
Cc: Adrian Bunk 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

markers: update preempt_disable. call_rcu, rcu_barrier comments

2008-03-25T02:22:19+00:00

Add comments requested by Andrew.

Updated comments about synchronize_sched().  Since we use call_rcu and
rcu_barrier now, these comments were out of sync with the code.

Signed-off-by: Mathieu Desnoyers 
Cc: Christoph Hellwig 
Cc: Mike Mason 
Cc: Dipankar Sarma 
Cc: David Smith 
Cc: "Paul E. McKenney" 
Cc: Steven Rostedt 
Cc: Adrian Bunk 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

markers: don't risk NULL deref in marker

2008-03-05T00:35:14+00:00

get_marker() may return NULL, so test for it.

[akpm@linux-foundation.org: coding-style fixes]
Signed-off-by: Jesper Juhl 
Acked-by: Mathieu Desnoyers 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds

markers: fix sparse warnings in markers.c

2008-02-24T01:12:14+00:00

char can be unsigned
kernel/marker.c:64:20: error: dubious one-bit signed bitfield
kernel/marker.c:65:14: error: dubious one-bit signed bitfield

Signed-off-by: Harvey Harrison 
Acked-by: Mathieu Desnoyers 
Signed-off-by: Andrew Morton 
Signed-off-by: Linus Torvalds