From mboxrd@z Thu Jan 1 00:00:00 1970 From: nils.carlson@ericsson.com (Nils Carlson) Date: Mon, 11 Apr 2011 13:58:07 +0200 Subject: [ltt-dev] [UST PATCH] Markers: remove channel name from trace_mark() In-Reply-To: <20110410223025.GA3068@Krystal> References: <20110410223025.GA3068@Krystal> Message-ID: Hi, This patch looks good. My only thought is, should we rename at the same time, so maybe trace_mark -> ust_mark ? It would in some ways make more sense to have an api where everything is prefixed with ust_ and not trace_ ? /Nils On Mon, 11 Apr 2011, Mathieu Desnoyers wrote: > *** This is an instrumentation API change *** > > Given that UST will gradually move to a scheme where channels are > dynamically associated with markers on a per tracing session basis (and > thus associated dynamically rather than fixed statically), it does not > make sense to specify the "channel name" in addition to the marker name > in the trace_mark() arguments. > > API touched: > > GET_MARKER() > DEFINE_MARKER() > DEFINE_MARKER_TP() > trace_mark() > _trace_mark() > > I'm introducing this API change without changing the underlying > implementation, trying to minimize the impact of API changes by doing > them sooner than later. > > Signed-off-by: Mathieu Desnoyers > --- > include/ust/marker.h | 35 ++++++-------- > libust/marker.c | 6 +- > libust/tracectl.c | 2 > libustinstr-malloc/mallocwrap.c | 4 - > tests/basic/basic.c | 4 - > tests/basic_long/basic_long.c | 4 - > tests/benchmark/bench.c | 2 > tests/dlopen/dlopen.c | 4 - > tests/dlopen/libdummy.c | 2 > tests/fork/fork.c | 10 ++-- > tests/fork/fork2.c | 2 > tests/hello/hello.c | 4 - > tests/hello2/hello2.c | 4 - > tests/libustctl_function_tests/libustctl_function_tests.c | 4 - > tests/make_shared_lib/basic_lib.c | 2 > tests/make_shared_lib/prog.c | 2 > tests/same_line_marker/same_line_marker.c | 2 > tests/test-nevents/prog.c | 4 - > tests/tracepoint/benchmark/tracepoint_benchmark.c | 4 - > tests/tracepoint/tracepoint_test.c | 8 +-- > > diff --git a/include/ust/marker.h b/include/ust/marker.h > index 29e84cc..da738d1 100644 > --- a/include/ust/marker.h > +++ b/include/ust/marker.h > @@ -77,7 +77,7 @@ struct marker { > void *location; /* Address of marker in code */ > }; > > -#define GET_MARKER(channel, name) (__mark_##channel##_##name) > +#define GET_MARKER(name) (__mark_ust_##name) > > #define _DEFINE_MARKER(channel, name, tp_name_str, tp_cb, format, unique, m) \ > struct registers __marker_regs; \ > @@ -135,17 +135,17 @@ struct marker { > save_registers(&__marker_regs) > > > -#define DEFINE_MARKER(channel, name, format, unique, m) \ > - _DEFINE_MARKER(channel, name, NULL, NULL, format, unique, m) > +#define DEFINE_MARKER(name, format, unique, m) \ > + _DEFINE_MARKER(ust, name, NULL, NULL, format, unique, m) > > -#define DEFINE_MARKER_TP(channel, name, tp_name, tp_cb, format) \ > - _DEFINE_MARKER_TP(channel, name, #tp_name, tp_cb, format) > +#define DEFINE_MARKER_TP(name, tp_name, tp_cb, format) \ > + _DEFINE_MARKER_TP(ust, name, #tp_name, tp_cb, format) > > #define _DEFINE_MARKER_TP(channel, name, tp_name_str, tp_cb, format) \ > static const char __mstrtab_##channel##_##name[] \ > __attribute__((section("__markers_strings"))) \ > = #channel "\0" #name "\0" format; \ > - static struct marker GET_MARKER(channel, name) \ > + static struct marker __mark_##channel##_##name \ > __attribute__((section("__markers"))) = \ > { __mstrtab_##channel##_##name, \ > &__mstrtab_##channel##_##name[sizeof(#channel)], \ > @@ -155,7 +155,7 @@ struct marker { > NULL, tp_name_str, tp_cb }; \ > static struct marker * const __mark_ptr_##channel##_##name \ > __attribute__((used, section("__markers_ptrs"))) = \ > - &GET_MARKER(channel, name); > + &__mark_##channel##_##name; > > /* > * Make sure the alignment of the structure in the __markers section will > @@ -173,7 +173,7 @@ struct marker { > #define __trace_mark_counter(generic, channel, name, unique, call_private, format, args...) \ > do { \ > struct marker *__marker_counter_ptr; \ > - DEFINE_MARKER(channel, name, format, unique, __marker_counter_ptr); \ > + _DEFINE_MARKER(channel, name, NULL, NULL, format, unique, __marker_counter_ptr); \ > __mark_check_format(format, ## args); \ > if (!generic) { \ > if (unlikely(imv_read(__marker_counter_ptr->state))) \ > @@ -194,9 +194,9 @@ struct marker { > { \ > register_trace_##tp_name(tp_cb, call_private); \ > } \ > - DEFINE_MARKER_TP(channel, name, tp_name, tp_cb, format);\ > + _DEFINE_MARKER_TP(channel, name, #tp_name, tp_cb, format); \ > __mark_check_format(format, ## args); \ > - (*GET_MARKER(channel, name).call)(&GET_MARKER(channel, name), \ > + (*__mark_##channel##_##name.call)(&__mark_##channel##_##name, \ > call_private, &__marker_regs, ## args); \ > } while (0) > > @@ -205,7 +205,6 @@ extern void marker_update_probe_range(struct marker * const *begin, > > /** > * trace_mark - Marker using code patching > - * @channel: marker channel (where to send the data), not quoted. > * @name: marker name, not quoted. > * @format: format string > * @args...: variable argument list > @@ -213,12 +212,11 @@ extern void marker_update_probe_range(struct marker * const *begin, > * Places a marker using optimized code patching technique (imv_read()) > * to be enabled when immediate values are present. > */ > -#define trace_mark(channel, name, format, args...) \ > - __trace_mark(0, channel, name, NULL, format, ## args) > +#define trace_mark(name, format, args...) \ > + __trace_mark(0, ust, name, NULL, format, ## args) > > /** > * _trace_mark - Marker using variable read > - * @channel: marker channel (where to send the data), not quoted. > * @name: marker name, not quoted. > * @format: format string > * @args...: variable argument list > @@ -227,12 +225,11 @@ extern void marker_update_probe_range(struct marker * const *begin, > * enabled. Should be used for markers in code paths where instruction > * modification based enabling is not welcome. > */ > -#define _trace_mark(channel, name, format, args...) \ > - __trace_mark(1, channel, name, NULL, format, ## args) > +#define _trace_mark(name, format, args...) \ > + __trace_mark(1, ust, name, NULL, format, ## args) > > /** > * trace_mark_tp - Marker in a tracepoint callback > - * @channel: marker channel (where to send the data), not quoted. > * @name: marker name, not quoted. > * @tp_name: tracepoint name, not quoted. > * @tp_cb: tracepoint callback. Should have an associated global symbol so it > @@ -242,8 +239,8 @@ extern void marker_update_probe_range(struct marker * const *begin, > * > * Places a marker in a tracepoint callback. > */ > -#define trace_mark_tp(channel, name, tp_name, tp_cb, format, args...) \ > - __trace_mark_tp(channel, name, NULL, tp_name, tp_cb, format, ## args) > +#define trace_mark_tp(name, tp_name, tp_cb, format, args...) \ > + __trace_mark_tp(ust, name, NULL, tp_name, tp_cb, format, ## args) > > /** > * MARK_NOARGS - Format string for a marker with no argument. > diff --git a/libust/marker.c b/libust/marker.c > index a64b46f..4b23e53 100644 > --- a/libust/marker.c > +++ b/libust/marker.c > @@ -447,7 +447,7 @@ static struct marker_entry *add_marker(const char *channel, const char *name, > e->call = marker_probe_cb_noarg; > else > e->call = marker_probe_cb; > - trace_mark(metadata, core_marker_format, > + __trace_mark(0, metadata, core_marker_format, NULL, > "channel %s name %s format %s", > e->channel, e->name, e->format); > } else { > @@ -514,7 +514,7 @@ static int marker_set_format(struct marker_entry *entry, const char *format) > return -ENOMEM; > entry->format_allocated = 1; > > - trace_mark(metadata, core_marker_format, > + __trace_mark(0, metadata, core_marker_format, NULL, > "channel %s name %s format %s", > entry->channel, entry->name, entry->format); > return 0; > @@ -781,7 +781,7 @@ int marker_probe_register(const char *channel, const char *name, > goto error_unregister_channel; > entry->event_id = ret; > ret = 0; > - trace_mark(metadata, core_marker_id, > + __trace_mark(0, metadata, core_marker_id, NULL, > "channel %s name %s event_id %hu " > "int #1u%zu long #1u%zu pointer #1u%zu " > "size_t #1u%zu alignment #1u%u", > diff --git a/libust/tracectl.c b/libust/tracectl.c > index 96053b7..e84a35a 100644 > --- a/libust/tracectl.c > +++ b/libust/tracectl.c > @@ -1576,7 +1576,7 @@ static void __attribute__((destructor)) keepalive() > > void ust_potential_exec(void) > { > - trace_mark(ust, potential_exec, MARK_NOARGS); > + trace_mark(potential_exec, MARK_NOARGS); > > DBG("test"); > > diff --git a/libustinstr-malloc/mallocwrap.c b/libustinstr-malloc/mallocwrap.c > index f5d5ce3..c473567 100644 > --- a/libustinstr-malloc/mallocwrap.c > +++ b/libustinstr-malloc/mallocwrap.c > @@ -75,7 +75,7 @@ void *malloc(size_t size) > > retval = plibc_malloc(size); > > - trace_mark(ust, malloc, "size %d ptr %p", (int)size, retval); > + trace_mark(malloc, "size %d ptr %p", (int)size, retval); > > return retval; > } > @@ -92,7 +92,7 @@ void free(void *ptr) > } > } > > - trace_mark(ust, free, "ptr %p", ptr); > + trace_mark(free, "ptr %p", ptr); > > plibc_free(ptr); > } > diff --git a/tests/basic/basic.c b/tests/basic/basic.c > index a810877..293feba 100644 > --- a/tests/basic/basic.c > +++ b/tests/basic/basic.c > @@ -29,8 +29,8 @@ int main() > printf("Basic test program\n"); > > for(i=0; i<50; i++) { > - trace_mark(ust, bar, "str %s", "FOOBAZ"); > - trace_mark(ust, bar2, "number1 %d number2 %d", 53, 9800); > + trace_mark(bar, "str %s", "FOOBAZ"); > + trace_mark(bar2, "number1 %d number2 %d", 53, 9800); > usleep(100000); > } > > diff --git a/tests/basic_long/basic_long.c b/tests/basic_long/basic_long.c > index 6cf44b6..06ee2f5 100644 > --- a/tests/basic_long/basic_long.c > +++ b/tests/basic_long/basic_long.c > @@ -25,8 +25,8 @@ int main() > printf("Basic test program\n"); > > for(;;) { > - trace_mark(ust, bar, "str %s", "FOOBAZ"); > - trace_mark(ust, bar2, "number1 %d number2 %d", 53, 9800); > + trace_mark(bar, "str %s", "FOOBAZ"); > + trace_mark(bar2, "number1 %d number2 %d", 53, 9800); > usleep(1000000); > } > > diff --git a/tests/benchmark/bench.c b/tests/benchmark/bench.c > index bc6a389..4e9c355 100644 > --- a/tests/benchmark/bench.c > +++ b/tests/benchmark/bench.c > @@ -29,7 +29,7 @@ void do_stuff(void) > time(NULL); > > #ifdef MARKER > - trace_mark(ust, event, "event %d", v); > + trace_mark(event, "event %d", v); > #endif > > } > diff --git a/tests/dlopen/dlopen.c b/tests/dlopen/dlopen.c > index 28d0ee6..d367580 100644 > --- a/tests/dlopen/dlopen.c > +++ b/tests/dlopen/dlopen.c > @@ -28,7 +28,7 @@ int main() > { > int (*fptr)(); > > - trace_mark(ust, from_main_before_lib, "%s", "Event occured in the main program before" > + trace_mark(from_main_before_lib, "%s", "Event occured in the main program before" > " the opening of the library\n"); > void *lib_handle = dlopen("libdummy.so", RTLD_LAZY); > > @@ -47,7 +47,7 @@ int main() > (*fptr)(); > dlclose(lib_handle); > > - trace_mark(ust, from_main_after_lib,"%s", "Event occured in the main program after " > + trace_mark(from_main_after_lib,"%s", "Event occured in the main program after " > "the library has been closed\n"); > > return 0; > diff --git a/tests/dlopen/libdummy.c b/tests/dlopen/libdummy.c > index 45507c0..16f7b4d 100644 > --- a/tests/dlopen/libdummy.c > +++ b/tests/dlopen/libdummy.c > @@ -19,5 +19,5 @@ > > void exported_function() > { > - trace_mark(ust, from_library, "%s", "Event occured in library function"); > + trace_mark(from_library, "%s", "Event occured in library function"); > } > diff --git a/tests/fork/fork.c b/tests/fork/fork.c > index a80518d..3b84644 100644 > --- a/tests/fork/fork.c > +++ b/tests/fork/fork.c > @@ -32,7 +32,7 @@ int main(int argc, char **argv, char *env[]) > } > > printf("Fork test program, parent pid is %d\n", getpid()); > - trace_mark(ust, before_fork, MARK_NOARGS); > + trace_mark(before_fork, MARK_NOARGS); > > /* Sleep here to make sure the consumer is initialized before we fork */ > sleep(1); > @@ -47,9 +47,9 @@ int main(int argc, char **argv, char *env[]) > > printf("Child pid is %d\n", getpid()); > > - trace_mark(ust, after_fork_child, MARK_NOARGS); > + trace_mark(after_fork_child, MARK_NOARGS); > > - trace_mark(ust, before_exec, "pid %d", getpid()); > + trace_mark(before_exec, "pid %d", getpid()); > > result = execve(argv[1], args, env); > if(result == -1) { > @@ -57,10 +57,10 @@ int main(int argc, char **argv, char *env[]) > return 1; > } > > - trace_mark(ust, after_exec, "pid %d", getpid()); > + trace_mark(after_exec, "pid %d", getpid()); > } > else { > - trace_mark(ust, after_fork_parent, MARK_NOARGS); > + trace_mark(after_fork_parent, MARK_NOARGS); > } > > return 0; > diff --git a/tests/fork/fork2.c b/tests/fork/fork2.c > index a290044..8a14e1a 100644 > --- a/tests/fork/fork2.c > +++ b/tests/fork/fork2.c > @@ -24,7 +24,7 @@ int main() > { > printf("IN FORK2\n"); > > - trace_mark(ust, after_exec, MARK_NOARGS); > + trace_mark(after_exec, MARK_NOARGS); > > return 0; > } > diff --git a/tests/hello/hello.c b/tests/hello/hello.c > index c0b541f..6ba2e61 100644 > --- a/tests/hello/hello.c > +++ b/tests/hello/hello.c > @@ -71,8 +71,8 @@ int main() > > sleep(1); > for(i=0; i<50; i++) { > - trace_mark(ust, bar, "str %s", "FOOBAZ"); > - trace_mark(ust, bar2, "number1 %d number2 %d", 53, 9800); > + trace_mark(bar, "str %s", "FOOBAZ"); > + trace_mark(bar2, "number1 %d number2 %d", 53, 9800); > trace_hello_tptest(i); > usleep(100000); > } > diff --git a/tests/hello2/hello2.c b/tests/hello2/hello2.c > index 7dc1b97..175a140 100644 > --- a/tests/hello2/hello2.c > +++ b/tests/hello2/hello2.c > @@ -37,8 +37,8 @@ int main() > printf("Hello, World!\n"); > > for(i=0; i<500; i++) { > - trace_mark(ust, bar, "str %d", i); > - trace_mark(ust, bar2, "number1 %d number2 %d", (int)53, (int)9800); > + trace_mark(bar, "str %d", i); > + trace_mark(bar2, "number1 %d number2 %d", (int)53, (int)9800); > } > > // ltt_trace_stop("auto"); > diff --git a/tests/libustctl_function_tests/libustctl_function_tests.c b/tests/libustctl_function_tests/libustctl_function_tests.c > index 7406243..cf184e6 100644 > --- a/tests/libustctl_function_tests/libustctl_function_tests.c > +++ b/tests/libustctl_function_tests/libustctl_function_tests.c > @@ -179,8 +179,8 @@ int main(int argc, char **argv) > child_pid = fork(); > if (child_pid) { > for(i=0; i<10; i++) { > - trace_mark(ust, bar, "str %s", "FOOBAZ"); > - trace_mark(ust, bar2, "number1 %d number2 %d", 53, 9800); > + trace_mark(bar, "str %s", "FOOBAZ"); > + trace_mark(bar2, "number1 %d number2 %d", 53, 9800); > usleep(100000); > } > > diff --git a/tests/make_shared_lib/basic_lib.c b/tests/make_shared_lib/basic_lib.c > index 2c1366e..97874f3 100644 > --- a/tests/make_shared_lib/basic_lib.c > +++ b/tests/make_shared_lib/basic_lib.c > @@ -3,7 +3,7 @@ > > void myfunc(void) > { > - trace_mark(ust, in_lib, MARK_NOARGS); > + trace_mark(in_lib, MARK_NOARGS); > printf("testfunc\n"); > } > > diff --git a/tests/make_shared_lib/prog.c b/tests/make_shared_lib/prog.c > index c1f5ac8..777f4c6 100644 > --- a/tests/make_shared_lib/prog.c > +++ b/tests/make_shared_lib/prog.c > @@ -5,6 +5,6 @@ extern myfunc(void); > int main(void) > { > myfunc(); > - trace_mark(ust, in_prog, MARK_NOARGS); > + trace_mark(in_prog, MARK_NOARGS); > return 0; > } > diff --git a/tests/same_line_marker/same_line_marker.c b/tests/same_line_marker/same_line_marker.c > index d9c0f22..51cf5c3 100644 > --- a/tests/same_line_marker/same_line_marker.c > +++ b/tests/same_line_marker/same_line_marker.c > @@ -19,6 +19,6 @@ > > int main() > { > - trace_mark(ust, same_line_event, "%s","An event occured in the same line"); trace_mark(ust, same_line_event, "%s","An event occured in the same line"); > + trace_mark(same_line_event, "%s","An event occured in the same line"); trace_mark(same_line_event, "%s","An event occured in the same line"); > return 0; > } > diff --git a/tests/test-nevents/prog.c b/tests/test-nevents/prog.c > index b2350cc..4e70915 100644 > --- a/tests/test-nevents/prog.c > +++ b/tests/test-nevents/prog.c > @@ -30,8 +30,8 @@ int main() > int i; > > for(i=0; i - trace_mark(ust, an_event, "%d", i); > - trace_mark(ust, another_event, "%s", "Hello, World!"); > + trace_mark(an_event, "%d", i); > + trace_mark(another_event, "%s", "Hello, World!"); > } > > return 0; > diff --git a/tests/tracepoint/benchmark/tracepoint_benchmark.c b/tests/tracepoint/benchmark/tracepoint_benchmark.c > index 8af4b84..c8ba8c1 100644 > --- a/tests/tracepoint/benchmark/tracepoint_benchmark.c > +++ b/tests/tracepoint/benchmark/tracepoint_benchmark.c > @@ -34,7 +34,7 @@ DEFINE_TRACE(ust_event); > > void tp_probe(void *data, unsigned int p1); > > -DEFINE_MARKER_TP(ust, event, ust_event, tp_probe, "p1 %u"); > +DEFINE_MARKER_TP(event, ust_event, tp_probe, "p1 %u"); > > /* > * Probe 1 --> ust_event > @@ -43,7 +43,7 @@ void tp_probe(void *data, unsigned int p1) > { > struct marker *marker; > > - marker = &GET_MARKER(ust, event); > + marker = &GET_MARKER(event); > ltt_specialized_trace(marker, data, &p1, sizeof(p1), sizeof(p1)); > } > > diff --git a/tests/tracepoint/tracepoint_test.c b/tests/tracepoint/tracepoint_test.c > index cd3939c..6a5f691 100644 > --- a/tests/tracepoint/tracepoint_test.c > +++ b/tests/tracepoint/tracepoint_test.c > @@ -47,7 +47,7 @@ void tp_probe4(void *data, unsigned int p4) > { > int i; > for (i = 0; i < 100; i++) { > - trace_mark_tp(ust, event2, ust_event2, tp_probe4, "probe4 %u", p4); > + trace_mark_tp(event2, ust_event2, tp_probe4, "probe4 %u", p4); > } > } > > @@ -60,7 +60,7 @@ void tp_probe3(void *data, unsigned int p3) > { > struct message *msg; > msg = (struct message*) data; > - trace_mark_tp(ust, event_msg, ust_event_msg, > + trace_mark_tp(event_msg, ust_event_msg, > tp_probe3, "probe %s", msg->payload); > } > > @@ -72,7 +72,7 @@ void tp_probe2(void *data, unsigned int p2) > { > int i; > for (i = 0; i < 5; i++) { > - trace_mark_tp(ust, event, ust_event, tp_probe2, "probe %u", 13); > + trace_mark_tp(event, ust_event, tp_probe2, "probe %u", 13); > } > } > > @@ -84,7 +84,7 @@ void tp_probe(void *data, unsigned int p1) > { > int i; > for (i = 0; i < 5; i++) { > - trace_mark_tp(ust, event, ust_event, tp_probe, "probe %u", p1); > + trace_mark_tp(event, ust_event, tp_probe, "probe %u", p1); > } > } > > -- > Mathieu Desnoyers > Operating System Efficiency R&D Consultant > EfficiOS Inc. > http://www.efficios.com >