Re: [PATCH v11 10/15] tracing: Add alternative synthetic event trace action syntax
Hi Tom, On Fri, Jan 11, 2019 at 10:25:40AM -0600, Tom Zanussi wrote: > Hi Namhyung, > > On Fri, 2019-01-11 at 15:07 +0900, Namhyung Kim wrote: > > On Wed, Jan 09, 2019 at 01:49:17PM -0600, Tom Zanussi wrote: > > > From: Tom Zanussi > > > > > > Add a 'trace(synthetic_event_name, params)' alternative to > > > synthetic_event_name(params). > > > > > > Currently, the syntax used for generating synthetic events is to > > > invoke synthetic_event_name(params) i.e. use the synthetic event > > > name > > > as a function call. > > > > > > Users requested a new form that more explicitly shows that the > > > synthetic event is in effect being traced. In this version, a new > > > 'trace()' keyword is used, and the synthetic event name is passed > > > in > > > as the first argument. > > > > > > Signed-off-by: Tom Zanussi > > > --- > > > Documentation/trace/histogram.rst | 21 > > > kernel/trace/trace.c | 1 + > > > kernel/trace/trace_events_hist.c | 42 > > > +++ > > > 3 files changed, 60 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/trace/histogram.rst > > > b/Documentation/trace/histogram.rst > > > index 79476c906b1a..4939bad1c1cd 100644 > > > --- a/Documentation/trace/histogram.rst > > > +++ b/Documentation/trace/histogram.rst > > > @@ -1874,6 +1874,7 @@ The available handlers are: > > > The available actions are: > > > > > >- (param list) - generate > > > synthetic event > > > + - trace(,(param list)) - generate > > > synthetic event > > > > Shouldn't it be > > > > "trace(,param list)" > > > > ? Otherwise it looks like we need two parentheses. > > Good point, I'll remove the params. > > > > > IMHO, it seems better for consistency using this new syntax only. > > Of course it should support the old syntax as well for compatibility > > (and maybe make it undocumented?). But I won't insist strongly.. > > > > OK, yeah, I really hate when things are undocumented, so I think > removing the documentation would be a step backward, but maybe changing > the emphasis to the trace() form would suffice. How about the below?: Looks good to me! Thanks, Namhyung > > > diff --git a/Documentation/trace/histogram.rst > b/Documentation/trace/histogram.rst > index e5bcef360997..0ea59d45aef1 100644 > --- a/Documentation/trace/histogram.rst > +++ b/Documentation/trace/histogram.rst > @@ -1873,46 +1873,45 @@ The available handlers are: > > The available actions are: > > - - (param list) - generate synthetic event >- trace(,param list) - generate synthetic event >- save(field,...)- save current event fields >- snapshot() - snapshot the trace buffer > > The following commonly-used handler.action pairs are available: > > - - onmatch(matching.event).(param list) > - > -or > - >- onmatch(matching.event).trace(,param list) > > -The 'onmatch(matching.event).(params)' hist > -trigger action is invoked whenever an event matches and the > -histogram entry would be added or updated. It causes the named > -synthetic event to be generated with the values given in the > +The 'onmatch(matching.event).trace(,param > +list)' hist trigger action is invoked whenever an event matches > +and the histogram entry would be added or updated. It causes the > +named synthetic event to be generated with the values given in the > 'param list'. The result is the generation of a synthetic event > that consists of the values contained in those variables at the > -time the invoking event was hit. > - > -There are two equivalent forms available for generating synthetic > -events. In the first form, the synthetic event name is used as if > -it were a function name. For example, if the synthetic event name > -is 'wakeup_latency', the wakeup_latency event would be generated > -by invoking it as if it were a function call, with the event field > -values passed in as arguments: wakeup_latency(arg1,arg2). The > -second form simply uses the 'trace' keyword as the function name > -and passes in the synthetic event name as the first argument, > -followed by the field values: trace(wakeup_latency,arg1,arg2). > - > -The 'param list' consists of one or more parameters which may be > -either variables or fields defined on either the 'matching.event' > -or the target event. The variables or fields specified in the > -param list may be either fully-qualified or unqualified. If a > -variable is specified as unqualified, it must be unique between > -the two events. A field name used as a param can be unqualified > -if it refers to the target event, but must be fully qualified if > -it refers to the matching event. A fully-qualified name is of the > -form 'system.event_name.$var_name' or 'system.event_name.field'. > +
Re: [PATCH v11 10/15] tracing: Add alternative synthetic event trace action syntax
Hi Namhyung, On Fri, 2019-01-11 at 15:07 +0900, Namhyung Kim wrote: > On Wed, Jan 09, 2019 at 01:49:17PM -0600, Tom Zanussi wrote: > > From: Tom Zanussi > > > > Add a 'trace(synthetic_event_name, params)' alternative to > > synthetic_event_name(params). > > > > Currently, the syntax used for generating synthetic events is to > > invoke synthetic_event_name(params) i.e. use the synthetic event > > name > > as a function call. > > > > Users requested a new form that more explicitly shows that the > > synthetic event is in effect being traced. In this version, a new > > 'trace()' keyword is used, and the synthetic event name is passed > > in > > as the first argument. > > > > Signed-off-by: Tom Zanussi > > --- > > Documentation/trace/histogram.rst | 21 > > kernel/trace/trace.c | 1 + > > kernel/trace/trace_events_hist.c | 42 > > +++ > > 3 files changed, 60 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/trace/histogram.rst > > b/Documentation/trace/histogram.rst > > index 79476c906b1a..4939bad1c1cd 100644 > > --- a/Documentation/trace/histogram.rst > > +++ b/Documentation/trace/histogram.rst > > @@ -1874,6 +1874,7 @@ The available handlers are: > > The available actions are: > > > >- (param list) - generate > > synthetic event > > + - trace(,(param list)) - generate > > synthetic event > > Shouldn't it be > > "trace(,param list)" > > ? Otherwise it looks like we need two parentheses. Good point, I'll remove the params. > > IMHO, it seems better for consistency using this new syntax only. > Of course it should support the old syntax as well for compatibility > (and maybe make it undocumented?). But I won't insist strongly.. > OK, yeah, I really hate when things are undocumented, so I think removing the documentation would be a step backward, but maybe changing the emphasis to the trace() form would suffice. How about the below?: diff --git a/Documentation/trace/histogram.rst b/Documentation/trace/histogram.rst index e5bcef360997..0ea59d45aef1 100644 --- a/Documentation/trace/histogram.rst +++ b/Documentation/trace/histogram.rst @@ -1873,46 +1873,45 @@ The available handlers are: The available actions are: - - (param list) - generate synthetic event - trace(,param list) - generate synthetic event - save(field,...)- save current event fields - snapshot() - snapshot the trace buffer The following commonly-used handler.action pairs are available: - - onmatch(matching.event).(param list) - -or - - onmatch(matching.event).trace(,param list) -The 'onmatch(matching.event).(params)' hist -trigger action is invoked whenever an event matches and the -histogram entry would be added or updated. It causes the named -synthetic event to be generated with the values given in the +The 'onmatch(matching.event).trace(,param +list)' hist trigger action is invoked whenever an event matches +and the histogram entry would be added or updated. It causes the +named synthetic event to be generated with the values given in the 'param list'. The result is the generation of a synthetic event that consists of the values contained in those variables at the -time the invoking event was hit. - -There are two equivalent forms available for generating synthetic -events. In the first form, the synthetic event name is used as if -it were a function name. For example, if the synthetic event name -is 'wakeup_latency', the wakeup_latency event would be generated -by invoking it as if it were a function call, with the event field -values passed in as arguments: wakeup_latency(arg1,arg2). The -second form simply uses the 'trace' keyword as the function name -and passes in the synthetic event name as the first argument, -followed by the field values: trace(wakeup_latency,arg1,arg2). - -The 'param list' consists of one or more parameters which may be -either variables or fields defined on either the 'matching.event' -or the target event. The variables or fields specified in the -param list may be either fully-qualified or unqualified. If a -variable is specified as unqualified, it must be unique between -the two events. A field name used as a param can be unqualified -if it refers to the target event, but must be fully qualified if -it refers to the matching event. A fully-qualified name is of the -form 'system.event_name.$var_name' or 'system.event_name.field'. +time the invoking event was hit. For example, if the synthetic +event name is 'wakeup_latency', a wakeup_latency event is +generated using onmatch(event).trace(wakeup_latency,arg1,arg2). + +There is also an equivalent alternative form available for +generating synthetic events. In this form, the synthetic event +
Re: [PATCH v11 10/15] tracing: Add alternative synthetic event trace action syntax
On Wed, Jan 09, 2019 at 01:49:17PM -0600, Tom Zanussi wrote: > From: Tom Zanussi > > Add a 'trace(synthetic_event_name, params)' alternative to > synthetic_event_name(params). > > Currently, the syntax used for generating synthetic events is to > invoke synthetic_event_name(params) i.e. use the synthetic event name > as a function call. > > Users requested a new form that more explicitly shows that the > synthetic event is in effect being traced. In this version, a new > 'trace()' keyword is used, and the synthetic event name is passed in > as the first argument. > > Signed-off-by: Tom Zanussi > --- > Documentation/trace/histogram.rst | 21 > kernel/trace/trace.c | 1 + > kernel/trace/trace_events_hist.c | 42 > +++ > 3 files changed, 60 insertions(+), 4 deletions(-) > > diff --git a/Documentation/trace/histogram.rst > b/Documentation/trace/histogram.rst > index 79476c906b1a..4939bad1c1cd 100644 > --- a/Documentation/trace/histogram.rst > +++ b/Documentation/trace/histogram.rst > @@ -1874,6 +1874,7 @@ The available handlers are: > The available actions are: > >- (param list) - generate synthetic event > + - trace(,(param list)) - generate synthetic event Shouldn't it be "trace(,param list)" ? Otherwise it looks like we need two parentheses. IMHO, it seems better for consistency using this new syntax only. Of course it should support the old syntax as well for compatibility (and maybe make it undocumented?). But I won't insist strongly.. >- save(field,...)- save current event fields >- snapshot() - snapshot the trace buffer > > @@ -1881,6 +1882,10 @@ The following commonly-used handler.action pairs are > available: > >- onmatch(matching.event).(param list) > > +or > + > + - onmatch(matching.event).trace(,(param list)) > + Ditto. > The 'onmatch(matching.event).(params)' hist > trigger action is invoked whenever an event matches and the > histogram entry would be added or updated. It causes the named > @@ -1889,6 +1894,16 @@ The following commonly-used handler.action pairs are > available: > that consists of the values contained in those variables at the > time the invoking event was hit. > > +There are two equivalent forms available for generating synthetic > +events. In the first form, the synthetic event name is used as if > +it were a function name. For example, if the synthetic event name > +is 'wakeup_latency', the wakeup_latency event would be generated > +by invoking it as if it were a function call, with the event field > +values passed in as arguments: wakeup_latency(arg1,arg2). The > +second form simply uses the 'trace' keyword as the function name > +and passes in the synthetic event name as the first argument, > +followed by the field values: trace(wakeup_latency,arg1,arg2). > + > The 'param list' consists of one or more parameters which may be > either variables or fields defined on either the 'matching.event' > or the target event. The variables or fields specified in the > @@ -1928,6 +1943,12 @@ The following commonly-used handler.action pairs are > available: >wakeup_new_test($testpid) if comm=="cyclictest"' >> \ >/sys/kernel/debug/tracing/events/sched/sched_wakeup_new/trigger > > +Or, equivalently, using the 'trace' keyword syntax: > + > +# echo 'hist:keys=$testpid:testpid=pid:onmatch(sched.sched_wakeup_new).\ > +trace(wakeup_new_test,$testpid) if comm=="cyclictest"' >> \ > +/sys/kernel/debug/tracing/events/sched/sched_wakeup_new/trigger > + > Creating and displaying a histogram based on those events is now > just a matter of using the fields and new synthetic event in the > tracing/events/synthetic directory, as usual:: > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 917677a9bcaa..aae0e4127afc 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -4900,6 +4900,7 @@ static const char readme_msg[] = > "\tonchange(var)- invoke action if var changes\n\n" > "\tThe available actions are:\n\n" > "\t(param list)- generate synthetic > event\n" > + "\ttrace(,(param list))- generate synthetic > event\n" Ditto. Thanks, Namhyung > "\tsave(field,...) - save current event > fields\n" > "\tsnapshot() - snapshot the trace > buffer\n" > #endif
[PATCH v11 10/15] tracing: Add alternative synthetic event trace action syntax
From: Tom Zanussi
Add a 'trace(synthetic_event_name, params)' alternative to
synthetic_event_name(params).
Currently, the syntax used for generating synthetic events is to
invoke synthetic_event_name(params) i.e. use the synthetic event name
as a function call.
Users requested a new form that more explicitly shows that the
synthetic event is in effect being traced. In this version, a new
'trace()' keyword is used, and the synthetic event name is passed in
as the first argument.
Signed-off-by: Tom Zanussi
---
Documentation/trace/histogram.rst | 21
kernel/trace/trace.c | 1 +
kernel/trace/trace_events_hist.c | 42 +++
3 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/Documentation/trace/histogram.rst
b/Documentation/trace/histogram.rst
index 79476c906b1a..4939bad1c1cd 100644
--- a/Documentation/trace/histogram.rst
+++ b/Documentation/trace/histogram.rst
@@ -1874,6 +1874,7 @@ The available handlers are:
The available actions are:
- (param list) - generate synthetic event
+ - trace(,(param list)) - generate synthetic event
- save(field,...)- save current event fields
- snapshot() - snapshot the trace buffer
@@ -1881,6 +1882,10 @@ The following commonly-used handler.action pairs are
available:
- onmatch(matching.event).(param list)
+or
+
+ - onmatch(matching.event).trace(,(param list))
+
The 'onmatch(matching.event).(params)' hist
trigger action is invoked whenever an event matches and the
histogram entry would be added or updated. It causes the named
@@ -1889,6 +1894,16 @@ The following commonly-used handler.action pairs are
available:
that consists of the values contained in those variables at the
time the invoking event was hit.
+There are two equivalent forms available for generating synthetic
+events. In the first form, the synthetic event name is used as if
+it were a function name. For example, if the synthetic event name
+is 'wakeup_latency', the wakeup_latency event would be generated
+by invoking it as if it were a function call, with the event field
+values passed in as arguments: wakeup_latency(arg1,arg2). The
+second form simply uses the 'trace' keyword as the function name
+and passes in the synthetic event name as the first argument,
+followed by the field values: trace(wakeup_latency,arg1,arg2).
+
The 'param list' consists of one or more parameters which may be
either variables or fields defined on either the 'matching.event'
or the target event. The variables or fields specified in the
@@ -1928,6 +1943,12 @@ The following commonly-used handler.action pairs are
available:
wakeup_new_test($testpid) if comm=="cyclictest"' >> \
/sys/kernel/debug/tracing/events/sched/sched_wakeup_new/trigger
+Or, equivalently, using the 'trace' keyword syntax:
+
+# echo 'hist:keys=$testpid:testpid=pid:onmatch(sched.sched_wakeup_new).\
+trace(wakeup_new_test,$testpid) if comm=="cyclictest"' >> \
+/sys/kernel/debug/tracing/events/sched/sched_wakeup_new/trigger
+
Creating and displaying a histogram based on those events is now
just a matter of using the fields and new synthetic event in the
tracing/events/synthetic directory, as usual::
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 917677a9bcaa..aae0e4127afc 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4900,6 +4900,7 @@ static const char readme_msg[] =
"\tonchange(var)- invoke action if var changes\n\n"
"\tThe available actions are:\n\n"
"\t(param list)- generate synthetic
event\n"
+ "\ttrace(,(param list))- generate synthetic
event\n"
"\tsave(field,...) - save current event
fields\n"
"\tsnapshot() - snapshot the trace
buffer\n"
#endif
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c3358ad63052..0622a2e27b11 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -427,6 +427,8 @@ struct action_data {
*/
unsigned intvar_ref_idx;
struct synth_event *synth_event;
+ booluse_trace_keyword;
+ char*synth_event_name;
union {
struct {
@@ -3724,6 +3726,8 @@ static void action_data_destroy(struct action_data *data)
if (data->synth_event)
data->synth_event->ref--;
+ kfree(data->synth_event_name);
+
kfree(data);
}
@@ -3804,6 +3808,7 @@ static int track_data_create(struct hist_trigger_data
*hist_data,
static int parse_action_params(char *params, struct action_data *data)
{
char *param,
