Markers - remove extra format argument

Denys Vlasenko <vda.linux@googlemail.com> :

> 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 <mathieu.desnoyers@polymtl.ca>
CC: Denys Vlasenko <vda.linux@googlemail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
This commit is contained in:
Mathieu Desnoyers 2008-05-12 21:21:09 +02:00 committed by Thomas Gleixner
parent 3eefae994d
commit dc102a8fae
2 changed files with 19 additions and 22 deletions

View File

@ -44,8 +44,8 @@ struct marker {
*/ */
char state; /* Marker state. */ char state; /* Marker state. */
char ptype; /* probe type : 0 : single, 1 : multi */ char ptype; /* probe type : 0 : single, 1 : multi */
void (*call)(const struct marker *mdata, /* Probe wrapper */ /* Probe wrapper */
void *call_private, const char *fmt, ...); void (*call)(const struct marker *mdata, void *call_private, ...);
struct marker_probe_closure single; struct marker_probe_closure single;
struct marker_probe_closure *multi; struct marker_probe_closure *multi;
} __attribute__((aligned(8))); } __attribute__((aligned(8)));
@ -72,8 +72,7 @@ struct marker {
__mark_check_format(format, ## args); \ __mark_check_format(format, ## args); \
if (unlikely(__mark_##name.state)) { \ if (unlikely(__mark_##name.state)) { \
(*__mark_##name.call) \ (*__mark_##name.call) \
(&__mark_##name, call_private, \ (&__mark_##name, call_private, ## args);\
format, ## args); \
} \ } \
} while (0) } while (0)
@ -117,9 +116,9 @@ static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
extern marker_probe_func __mark_empty_function; extern marker_probe_func __mark_empty_function;
extern void marker_probe_cb(const struct marker *mdata, extern void marker_probe_cb(const struct marker *mdata,
void *call_private, const char *fmt, ...); void *call_private, ...);
extern void marker_probe_cb_noarg(const struct marker *mdata, extern void marker_probe_cb_noarg(const struct marker *mdata,
void *call_private, const char *fmt, ...); void *call_private, ...);
/* /*
* Connect a probe to a marker. * Connect a probe to a marker.

View File

@ -55,8 +55,8 @@ static DEFINE_MUTEX(markers_mutex);
struct marker_entry { struct marker_entry {
struct hlist_node hlist; struct hlist_node hlist;
char *format; char *format;
void (*call)(const struct marker *mdata, /* Probe wrapper */ /* Probe wrapper */
void *call_private, const char *fmt, ...); void (*call)(const struct marker *mdata, void *call_private, ...);
struct marker_probe_closure single; struct marker_probe_closure single;
struct marker_probe_closure *multi; struct marker_probe_closure *multi;
int refcount; /* Number of times armed. 0 if disarmed. */ int refcount; /* Number of times armed. 0 if disarmed. */
@ -91,15 +91,13 @@ EXPORT_SYMBOL_GPL(__mark_empty_function);
* marker_probe_cb Callback that prepares the variable argument list for probes. * marker_probe_cb Callback that prepares the variable argument list for probes.
* @mdata: pointer of type struct marker * @mdata: pointer of type struct marker
* @call_private: caller site private data * @call_private: caller site private data
* @fmt: format string
* @...: Variable argument list. * @...: Variable argument list.
* *
* Since we do not use "typical" pointer based RCU in the 1 argument case, we * Since we do not use "typical" pointer based RCU in the 1 argument case, we
* need to put a full smp_rmb() in this branch. This is why we do not use * need to put a full smp_rmb() in this branch. This is why we do not use
* rcu_dereference() for the pointer read. * rcu_dereference() for the pointer read.
*/ */
void marker_probe_cb(const struct marker *mdata, void *call_private, void marker_probe_cb(const struct marker *mdata, void *call_private, ...)
const char *fmt, ...)
{ {
va_list args; va_list args;
char ptype; char ptype;
@ -120,8 +118,9 @@ void marker_probe_cb(const struct marker *mdata, void *call_private,
/* Must read the ptr before private data. They are not data /* Must read the ptr before private data. They are not data
* dependant, so we put an explicit smp_rmb() here. */ * dependant, so we put an explicit smp_rmb() here. */
smp_rmb(); smp_rmb();
va_start(args, fmt); va_start(args, call_private);
func(mdata->single.probe_private, call_private, fmt, &args); func(mdata->single.probe_private, call_private, mdata->format,
&args);
va_end(args); va_end(args);
} else { } else {
struct marker_probe_closure *multi; struct marker_probe_closure *multi;
@ -136,9 +135,9 @@ void marker_probe_cb(const struct marker *mdata, void *call_private,
smp_read_barrier_depends(); smp_read_barrier_depends();
multi = mdata->multi; multi = mdata->multi;
for (i = 0; multi[i].func; i++) { for (i = 0; multi[i].func; i++) {
va_start(args, fmt); va_start(args, call_private);
multi[i].func(multi[i].probe_private, call_private, fmt, multi[i].func(multi[i].probe_private, call_private,
&args); mdata->format, &args);
va_end(args); va_end(args);
} }
} }
@ -150,13 +149,11 @@ EXPORT_SYMBOL_GPL(marker_probe_cb);
* marker_probe_cb Callback that does not prepare the variable argument list. * marker_probe_cb Callback that does not prepare the variable argument list.
* @mdata: pointer of type struct marker * @mdata: pointer of type struct marker
* @call_private: caller site private data * @call_private: caller site private data
* @fmt: format string
* @...: Variable argument list. * @...: Variable argument list.
* *
* Should be connected to markers "MARK_NOARGS". * Should be connected to markers "MARK_NOARGS".
*/ */
void marker_probe_cb_noarg(const struct marker *mdata, void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...)
void *call_private, const char *fmt, ...)
{ {
va_list args; /* not initialized */ va_list args; /* not initialized */
char ptype; char ptype;
@ -172,7 +169,8 @@ void marker_probe_cb_noarg(const struct marker *mdata,
/* Must read the ptr before private data. They are not data /* Must read the ptr before private data. They are not data
* dependant, so we put an explicit smp_rmb() here. */ * dependant, so we put an explicit smp_rmb() here. */
smp_rmb(); smp_rmb();
func(mdata->single.probe_private, call_private, fmt, &args); func(mdata->single.probe_private, call_private, mdata->format,
&args);
} else { } else {
struct marker_probe_closure *multi; struct marker_probe_closure *multi;
int i; int i;
@ -186,8 +184,8 @@ void marker_probe_cb_noarg(const struct marker *mdata,
smp_read_barrier_depends(); smp_read_barrier_depends();
multi = mdata->multi; multi = mdata->multi;
for (i = 0; multi[i].func; i++) for (i = 0; multi[i].func; i++)
multi[i].func(multi[i].probe_private, call_private, fmt, multi[i].func(multi[i].probe_private, call_private,
&args); mdata->format, &args);
} }
preempt_enable(); preempt_enable();
} }