From: Tom Zanussi <[email protected]>
Simplify the existing event_command.parse() implementations by having
them make use of the helper functions previously introduced.
Link: https://lkml.kernel.org/r/0ed90e6dd9b8ef984e5af8b7a200f35a80d069ef.1641823001.git.zanussi@kernel.org
Signed-off-by: Tom Zanussi <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/trace/trace.h | 3 +-
kernel/trace/trace_eprobe.c | 3 +-
kernel/trace/trace_events_hist.c | 76 +++++------
kernel/trace/trace_events_trigger.c | 192 ++++++++--------------------
4 files changed, 86 insertions(+), 188 deletions(-)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index d038ddbf1bea..4f57566e6dc7 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1580,7 +1580,8 @@ extern void event_enable_trigger_free(struct event_trigger_ops *ops,
struct event_trigger_data *data);
extern int event_enable_trigger_parse(struct event_command *cmd_ops,
struct trace_event_file *file,
- char *glob, char *cmd, char *param);
+ char *glob, char *cmd,
+ char *param_and_filter);
extern int event_enable_register_trigger(char *glob,
struct event_trigger_data *data,
struct trace_event_file *file);
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 191db32dec46..f5c8dc1a5b5f 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -551,7 +551,8 @@ static struct event_trigger_ops eprobe_trigger_ops = {
static int eprobe_trigger_cmd_parse(struct event_command *cmd_ops,
struct trace_event_file *file,
- char *glob, char *cmd, char *param)
+ char *glob, char *cmd,
+ char *param_and_filter)
{
return -1;
}
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 5e6a988a8a51..f9f7fd9ecc3d 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2763,7 +2763,8 @@ static char *find_trigger_filter(struct hist_trigger_data *hist_data,
static struct event_command trigger_hist_cmd;
static int event_hist_trigger_parse(struct event_command *cmd_ops,
struct trace_event_file *file,
- char *glob, char *cmd, char *param);
+ char *glob, char *cmd,
+ char *param_and_filter);
static bool compatible_keys(struct hist_trigger_data *target_hist_data,
struct hist_trigger_data *hist_data,
@@ -6148,48 +6149,48 @@ static void hist_unreg_all(struct trace_event_file *file)
static int event_hist_trigger_parse(struct event_command *cmd_ops,
struct trace_event_file *file,
- char *glob, char *cmd, char *param)
+ char *glob, char *cmd,
+ char *param_and_filter)
{
unsigned int hist_trigger_bits = TRACING_MAP_BITS_DEFAULT;
struct event_trigger_data *trigger_data;
struct hist_trigger_attrs *attrs;
- struct event_trigger_ops *trigger_ops;
struct hist_trigger_data *hist_data;
+ char *param, *filter, *p, *start;
struct synth_event *se;
const char *se_name;
- bool remove = false;
- char *trigger, *p, *start;
+ int n_registered;
+ bool remove;
int ret = 0;
lockdep_assert_held(&event_mutex);
if (glob && strlen(glob)) {
hist_err_clear();
- last_cmd_set(file, param);
+ last_cmd_set(file, param_and_filter);
}
- if (!param)
- return -EINVAL;
+ remove = event_trigger_check_remove(glob);
- if (glob[0] == '!')
- remove = true;
+ if (event_trigger_empty_param(param_and_filter))
+ return -EINVAL;
/*
* separate the trigger from the filter (k:v [if filter])
* allowing for whitespace in the trigger
*/
- p = trigger = param;
+ p = param = param_and_filter;
do {
p = strstr(p, "if");
if (!p)
break;
- if (p == param)
+ if (p == param_and_filter)
return -EINVAL;
if (*(p - 1) != ' ' && *(p - 1) != '\t') {
p++;
continue;
}
- if (p >= param + strlen(param) - (sizeof("if") - 1) - 1)
+ if (p >= param_and_filter + strlen(param_and_filter) - (sizeof("if") - 1) - 1)
return -EINVAL;
if (*(p + sizeof("if") - 1) != ' ' && *(p + sizeof("if") - 1) != '\t') {
p++;
@@ -6199,24 +6200,24 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
} while (p);
if (!p)
- param = NULL;
+ filter = NULL;
else {
*(p - 1) = '\0';
- param = strstrip(p);
- trigger = strstrip(trigger);
+ filter = strstrip(p);
+ param = strstrip(param);
}
/*
* To simplify arithmetic expression parsing, replace occurrences of
* '.sym-offset' modifier with '.symXoffset'
*/
- start = strstr(trigger, ".sym-offset");
+ start = strstr(param, ".sym-offset");
while (start) {
*(start + 4) = 'X';
start = strstr(start + 11, ".sym-offset");
}
- attrs = parse_hist_trigger_attrs(file->tr, trigger);
+ attrs = parse_hist_trigger_attrs(file->tr, param);
if (IS_ERR(attrs))
return PTR_ERR(attrs);
@@ -6229,29 +6230,15 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
return PTR_ERR(hist_data);
}
- trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
-
- trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+ trigger_data = event_trigger_alloc(cmd_ops, cmd, param, hist_data);
if (!trigger_data) {
ret = -ENOMEM;
goto out_free;
}
- trigger_data->count = -1;
- trigger_data->ops = trigger_ops;
- trigger_data->cmd_ops = cmd_ops;
-
- INIT_LIST_HEAD(&trigger_data->list);
- RCU_INIT_POINTER(trigger_data->filter, NULL);
-
- trigger_data->private_data = hist_data;
-
- /* if param is non-empty, it's supposed to be a filter */
- if (param && cmd_ops->set_filter) {
- ret = cmd_ops->set_filter(param, trigger_data, file);
- if (ret < 0)
- goto out_free;
- }
+ ret = event_trigger_set_filter(cmd_ops, file, filter, trigger_data);
+ if (ret < 0)
+ goto out_free;
if (remove) {
if (!have_hist_trigger_match(trigger_data, file))
@@ -6271,18 +6258,14 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
goto out_free;
}
- ret = cmd_ops->reg(glob, trigger_data, file);
- /*
- * The above returns on success the # of triggers registered,
- * but if it didn't register any it returns zero. Consider no
- * triggers registered a failure too.
- */
- if (!ret) {
+ ret = event_trigger_register(cmd_ops, file, glob, cmd, param, trigger_data, &n_registered);
+ if (ret)
+ goto out_free;
+ if ((ret == 0) && (n_registered == 0)) {
if (!(attrs->pause || attrs->cont || attrs->clear))
ret = -ENOENT;
goto out_free;
- } else if (ret < 0)
- goto out_free;
+ }
if (get_named_trigger_data(trigger_data))
goto enable;
@@ -6316,8 +6299,7 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops,
out_unreg:
cmd_ops->unreg(glob+1, trigger_data, file);
out_free:
- if (cmd_ops->set_filter)
- cmd_ops->set_filter(NULL, trigger_data, NULL);
+ event_trigger_reset_filter(cmd_ops, trigger_data);
remove_hist_vars(hist_data);
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index d00fee705f9c..2ab5837bc141 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -967,7 +967,7 @@ int event_trigger_register(struct event_command *cmd_ops,
* @file: The trace_event_file associated with the event
* @glob: The raw string used to register the trigger
* @cmd: The cmd portion of the string used to register the trigger
- * @param: The params portion of the string used to register the trigger
+ * @param_and_filter: The param and filter portion of the string used to register the trigger
*
* Common implementation for event command parsing and trigger
* instantiation.
@@ -980,94 +980,52 @@ int event_trigger_register(struct event_command *cmd_ops,
static int
event_trigger_parse(struct event_command *cmd_ops,
struct trace_event_file *file,
- char *glob, char *cmd, char *param)
+ char *glob, char *cmd, char *param_and_filter)
{
struct event_trigger_data *trigger_data;
- struct event_trigger_ops *trigger_ops;
- char *trigger = NULL;
- char *number;
+ char *param, *filter;
+ bool remove;
int ret;
- /* separate the trigger from the filter (t:n [if filter]) */
- if (param && isdigit(param[0])) {
- trigger = strsep(¶m, " \t");
- if (param) {
- param = skip_spaces(param);
- if (!*param)
- param = NULL;
- }
- }
+ remove = event_trigger_check_remove(glob);
- trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
+ ret = event_trigger_separate_filter(param_and_filter, ¶m, &filter, false);
+ if (ret)
+ return ret;
ret = -ENOMEM;
- trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+ trigger_data = event_trigger_alloc(cmd_ops, cmd, param, file);
if (!trigger_data)
goto out;
- trigger_data->count = -1;
- trigger_data->ops = trigger_ops;
- trigger_data->cmd_ops = cmd_ops;
- trigger_data->private_data = file;
- INIT_LIST_HEAD(&trigger_data->list);
- INIT_LIST_HEAD(&trigger_data->named_list);
-
- if (glob[0] == '!') {
+ if (remove) {
cmd_ops->unreg(glob+1, trigger_data, file);
kfree(trigger_data);
ret = 0;
goto out;
}
- if (trigger) {
- number = strsep(&trigger, ":");
-
- ret = -EINVAL;
- if (!strlen(number))
- goto out_free;
-
- /*
- * We use the callback data field (which is a pointer)
- * as our counter.
- */
- ret = kstrtoul(number, 0, &trigger_data->count);
- if (ret)
- goto out_free;
- }
-
- if (!param) /* if param is non-empty, it's supposed to be a filter */
- goto out_reg;
-
- if (!cmd_ops->set_filter)
- goto out_reg;
+ ret = event_trigger_parse_num(param, trigger_data);
+ if (ret)
+ goto out_free;
- ret = cmd_ops->set_filter(param, trigger_data, file);
+ ret = event_trigger_set_filter(cmd_ops, file, filter, trigger_data);
if (ret < 0)
goto out_free;
- out_reg:
/* Up the trigger_data count to make sure reg doesn't free it on failure */
- event_trigger_init(trigger_ops, trigger_data);
- ret = cmd_ops->reg(glob, trigger_data, file);
- /*
- * The above returns on success the # of functions enabled,
- * but if it didn't find any functions it returns zero.
- * Consider no functions a failure too.
- */
- if (!ret) {
- cmd_ops->unreg(glob, trigger_data, file);
- ret = -ENOENT;
- } else if (ret > 0)
- ret = 0;
+ event_trigger_init(trigger_data->ops, trigger_data);
+
+ ret = event_trigger_register(cmd_ops, file, glob, cmd, param, trigger_data, NULL);
+ if (ret)
+ goto out_free;
/* Down the counter of trigger_data or free it if not used anymore */
- event_trigger_free(trigger_ops, trigger_data);
+ event_trigger_free(trigger_data->ops, trigger_data);
out:
return ret;
-
out_free:
- if (cmd_ops->set_filter)
- cmd_ops->set_filter(NULL, trigger_data, NULL);
+ event_trigger_reset_filter(cmd_ops, trigger_data);
kfree(trigger_data);
goto out;
}
@@ -1722,39 +1680,33 @@ static struct event_trigger_ops event_disable_count_trigger_ops = {
int event_enable_trigger_parse(struct event_command *cmd_ops,
struct trace_event_file *file,
- char *glob, char *cmd, char *param)
+ char *glob, char *cmd, char *param_and_filter)
{
struct trace_event_file *event_enable_file;
struct enable_trigger_data *enable_data;
struct event_trigger_data *trigger_data;
- struct event_trigger_ops *trigger_ops;
struct trace_array *tr = file->tr;
+ char *param, *filter;
+ bool enable, remove;
const char *system;
const char *event;
bool hist = false;
- char *trigger;
- char *number;
- bool enable;
int ret;
- if (!param)
- return -EINVAL;
+ remove = event_trigger_check_remove(glob);
- /* separate the trigger from the filter (s:e:n [if filter]) */
- trigger = strsep(¶m, " \t");
- if (!trigger)
+ if (event_trigger_empty_param(param_and_filter))
return -EINVAL;
- if (param) {
- param = skip_spaces(param);
- if (!*param)
- param = NULL;
- }
- system = strsep(&trigger, ":");
- if (!trigger)
+ ret = event_trigger_separate_filter(param_and_filter, ¶m, &filter, true);
+ if (ret)
+ return ret;
+
+ system = strsep(¶m, ":");
+ if (!param)
return -EINVAL;
- event = strsep(&trigger, ":");
+ event = strsep(¶m, ":");
ret = -EINVAL;
event_enable_file = find_event_file(tr, system, event);
@@ -1770,31 +1722,22 @@ int event_enable_trigger_parse(struct event_command *cmd_ops,
#else
enable = strcmp(cmd, ENABLE_EVENT_STR) == 0;
#endif
- trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
-
ret = -ENOMEM;
- trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
- if (!trigger_data)
- goto out;
-
enable_data = kzalloc(sizeof(*enable_data), GFP_KERNEL);
- if (!enable_data) {
- kfree(trigger_data);
+ if (!enable_data)
goto out;
- }
-
- trigger_data->count = -1;
- trigger_data->ops = trigger_ops;
- trigger_data->cmd_ops = cmd_ops;
- INIT_LIST_HEAD(&trigger_data->list);
- RCU_INIT_POINTER(trigger_data->filter, NULL);
enable_data->hist = hist;
enable_data->enable = enable;
enable_data->file = event_enable_file;
- trigger_data->private_data = enable_data;
- if (glob[0] == '!') {
+ trigger_data = event_trigger_alloc(cmd_ops, cmd, param, enable_data);
+ if (!trigger_data) {
+ kfree(enable_data);
+ goto out;
+ }
+
+ if (remove) {
cmd_ops->unreg(glob+1, trigger_data, file);
kfree(trigger_data);
kfree(enable_data);
@@ -1803,35 +1746,16 @@ int event_enable_trigger_parse(struct event_command *cmd_ops,
}
/* Up the trigger_data count to make sure nothing frees it on failure */
- event_trigger_init(trigger_ops, trigger_data);
-
- if (trigger) {
- number = strsep(&trigger, ":");
+ event_trigger_init(trigger_data->ops, trigger_data);
- ret = -EINVAL;
- if (!strlen(number))
- goto out_free;
-
- /*
- * We use the callback data field (which is a pointer)
- * as our counter.
- */
- ret = kstrtoul(number, 0, &trigger_data->count);
- if (ret)
- goto out_free;
- }
-
- if (!param) /* if param is non-empty, it's supposed to be a filter */
- goto out_reg;
-
- if (!cmd_ops->set_filter)
- goto out_reg;
+ ret = event_trigger_parse_num(param, trigger_data);
+ if (ret)
+ goto out_free;
- ret = cmd_ops->set_filter(param, trigger_data, file);
+ ret = event_trigger_set_filter(cmd_ops, file, filter, trigger_data);
if (ret < 0)
goto out_free;
- out_reg:
/* Don't let event modules unload while probe registered */
ret = trace_event_try_get_ref(event_enable_file->event_call);
if (!ret) {
@@ -1842,31 +1766,21 @@ int event_enable_trigger_parse(struct event_command *cmd_ops,
ret = trace_event_enable_disable(event_enable_file, 1, 1);
if (ret < 0)
goto out_put;
- ret = cmd_ops->reg(glob, trigger_data, file);
- /*
- * The above returns on success the # of functions enabled,
- * but if it didn't find any functions it returns zero.
- * Consider no functions a failure too.
- */
- if (!ret) {
- ret = -ENOENT;
- goto out_disable;
- } else if (ret < 0)
+
+ ret = event_trigger_register(cmd_ops, file, glob, cmd, param, trigger_data, NULL);
+ if (ret)
goto out_disable;
- /* Just return zero, not the number of enabled functions */
- ret = 0;
- event_trigger_free(trigger_ops, trigger_data);
+
+ event_trigger_free(trigger_data->ops, trigger_data);
out:
return ret;
-
out_disable:
trace_event_enable_disable(event_enable_file, 0, 1);
out_put:
trace_event_put_ref(event_enable_file->event_call);
out_free:
- if (cmd_ops->set_filter)
- cmd_ops->set_filter(NULL, trigger_data, NULL);
- event_trigger_free(trigger_ops, trigger_data);
+ event_trigger_reset_filter(cmd_ops, trigger_data);
+ event_trigger_free(trigger_data->ops, trigger_data);
kfree(enable_data);
goto out;
}
--
2.33.0
Hi
On 1/11/22 18:30, Steven Rostedt wrote:
> From: Tom Zanussi <[email protected]>
>
> Simplify the existing event_command.parse() implementations by having
> them make use of the helper functions previously introduced.
While testing rtla with all for-next changes, I noticed this patch breaks:
# rtla osnoise hist -d 5
Before this patch, osnoise hist is able to enable histograms and collect data.
After this patch it does not work.
The event I am creating the histogram is osnoise:sample_threshold, and the
trigger command is:
hist:keys=duration.buckets=1000,common_cpu:vals=hitcount:sort=hitcount:size=2048
I did some debug, and found that the histogram is working. The problem is that,
to read the histogram I pause it to have consistent data:
in tools/tracing/rtla/osnoise_hist.c:
osnoise_read_trace_hist() {
[...]
tracefs_hist_pause(tool->trace.inst, data->trace_hist);
content = tracefs_event_file_read(tool->trace.inst, "osnoise",
"sample_threshold",
"hist", NULL);
[...]
}
and, as far as I got, after this patch, pausing the histogram makes it to clear
up. If I comment the "tracefs_hist_pause" line, "rtla osnoise hist" start
working back again.
Thoughts?
-- Daniel
On Thu, 13 Jan 2022 18:03:07 +0100
Daniel Bristot de Oliveira <[email protected]> wrote:
> I did some debug, and found that the histogram is working. The problem is that,
> to read the histogram I pause it to have consistent data:
>
> in tools/tracing/rtla/osnoise_hist.c:
> osnoise_read_trace_hist() {
> [...]
> tracefs_hist_pause(tool->trace.inst, data->trace_hist);
>
> content = tracefs_event_file_read(tool->trace.inst, "osnoise",
> "sample_threshold",
> "hist", NULL);
> [...]
> }
>
> and, as far as I got, after this patch, pausing the histogram makes it to clear
> up. If I comment the "tracefs_hist_pause" line, "rtla osnoise hist" start
> working back again.
>
> Thoughts?
This is all messed up. I'm removing this patch completely.
Tom, can you fix this. The issue is that it's putting too much policy into
the helper functions, which is big no no.
Specifically, we have:
int event_trigger_register(struct event_command *cmd_ops,
struct trace_event_file *file,
char *glob,
char *cmd,
char *param,
struct event_trigger_data *trigger_data,
int *n_registered)
{
int ret;
if (n_registered)
*n_registered = 0;
ret = cmd_ops->reg(glob, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
if (!ret) {
cmd_ops->unreg(glob, trigger_data, file);
ret = -ENOENT;
} else if (ret > 0) {
if (n_registered)
*n_registered = ret;
/* Just return zero, not the number of enabled functions */
ret = 0;
}
return ret;
}
And in the case of pause, this *will* have ret = 0 on return. And what
happens is that it removes the trigger completely.
Look at the code in the histogram on the return:
ret = event_trigger_register(cmd_ops, file, glob, cmd, param, trigger_data, &n_registered);
if (ret < 0)
goto out_free;
if ((ret == 0) && (n_registered == 0)) {
if (!(attrs->pause || attrs->cont || attrs->clear))
ret = -ENOENT;
goto out_free;
}
It checks for 0 and 0 and only errors if it's not pause, cont, or clear.
Hence, all three are now broken due to this patch.
I will not be adding this to this merge window.
-- Steve
On Thu, 13 Jan 2022 16:20:58 -0500
Steven Rostedt <[email protected]> wrote:
> I will not be adding this to this merge window.
And it appears that "tracing: Remove redundant trigger_ops params" does not
apply cleanly without this patch, so I'm dropping that one too.
-- Steve
Hi Steve,
On Thu, 2022-01-13 at 16:20 -0500, Steven Rostedt wrote:
> On Thu, 13 Jan 2022 18:03:07 +0100
> Daniel Bristot de Oliveira <[email protected]> wrote:
>
> > I did some debug, and found that the histogram is working. The
> > problem is that,
> > to read the histogram I pause it to have consistent data:
> >
> > in tools/tracing/rtla/osnoise_hist.c:
> > osnoise_read_trace_hist() {
> > [...]
> > tracefs_hist_pause(tool->trace.inst, data->trace_hist);
> >
> > content = tracefs_event_file_read(tool->trace.inst,
> > "osnoise",
> > "sample_threshold",
> > "hist", NULL);
> > [...]
> > }
> >
> > and, as far as I got, after this patch, pausing the histogram makes
> > it to clear
> > up. If I comment the "tracefs_hist_pause" line, "rtla osnoise hist"
> > start
> > working back again.
> >
> > Thoughts?
>
> This is all messed up. I'm removing this patch completely.
>
> Tom, can you fix this. The issue is that it's putting too much policy
> into
> the helper functions, which is big no no.
>
> Specifically, we have:
>
> int event_trigger_register(struct event_command *cmd_ops,
> struct trace_event_file *file,
> char *glob,
> char *cmd,
> char *param,
> struct event_trigger_data *trigger_data,
> int *n_registered)
> {
> int ret;
>
> if (n_registered)
> *n_registered = 0;
>
> ret = cmd_ops->reg(glob, trigger_data, file);
> /*
> * The above returns on success the # of functions enabled,
> * but if it didn't find any functions it returns zero.
> * Consider no functions a failure too.
> */
> if (!ret) {
> cmd_ops->unreg(glob, trigger_data, file);
> ret = -ENOENT;
> } else if (ret > 0) {
> if (n_registered)
> *n_registered = ret;
> /* Just return zero, not the number of enabled
> functions */
> ret = 0;
> }
>
> return ret;
> }
>
>
> And in the case of pause, this *will* have ret = 0 on return. And
> what
> happens is that it removes the trigger completely.
>
> Look at the code in the histogram on the return:
>
> ret = event_trigger_register(cmd_ops, file, glob, cmd, param,
> trigger_data, &n_registered);
> if (ret < 0)
> goto out_free;
> if ((ret == 0) && (n_registered == 0)) {
> if (!(attrs->pause || attrs->cont || attrs->clear))
> ret = -ENOENT;
> goto out_free;
> }
>
> It checks for 0 and 0 and only errors if it's not pause, cont, or
> clear.
> Hence, all three are now broken due to this patch.
>
> I will not be adding this to this merge window.
Yes, you're right, event_trigger_register() is trying to do a little
too much, and shouldn't be doing the unreg().
Thanks for finding and figuring that out. I'll fix this and send a new
version after the merge window.
Tom
>
> -- Steve