* [PATCH v2 0/2] tracing: Allow execnames to be passed as args for synthetic events @ 2021-07-22 14:27 Steven Rostedt 2021-07-22 14:27 ` [PATCH v2 1/2] tracing: Have histogram types be constant when possible Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Steven Rostedt @ 2021-07-22 14:27 UTC (permalink / raw) To: linux-kernel Cc: Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu, Namhyung Kim Changes since v1: - Added patch to have all types be possible const (Masami Hiramatsu) Steven Rostedt (VMware) (2): tracing: Have histogram types be constant when possible tracing: Allow execnames to be passed as args for synthetic events ---- kernel/trace/trace_events_hist.c | 78 ++++++++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 22 deletions(-) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] tracing: Have histogram types be constant when possible 2021-07-22 14:27 [PATCH v2 0/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt @ 2021-07-22 14:27 ` Steven Rostedt 2021-07-22 15:24 ` Masami Hiramatsu 2021-07-22 22:11 ` Tom Zanussi 2021-07-22 14:27 ` [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt 2021-07-22 15:14 ` [PATCH v2 0/2] " Steven Rostedt 2 siblings, 2 replies; 18+ messages in thread From: Steven Rostedt @ 2021-07-22 14:27 UTC (permalink / raw) To: linux-kernel Cc: Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu, Namhyung Kim From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Instead of kstrdup("const", GFP_KERNEL), have the hist_field type simply assign the constant hist_field->type = "const"; And when the value passed to it is a variable, use "kstrdup_const(var, GFP_KERNEL);" which will just copy the value if the variable is already a constant. This saves on having to allocate when not needed. All frees of the hist_field->type will need to use kfree_const(). Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/trace_events_hist.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 34325f41ebc0..5c9082201bc2 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1589,7 +1589,9 @@ static void __destroy_hist_field(struct hist_field *hist_field) kfree(hist_field->var.name); kfree(hist_field->name); - kfree(hist_field->type); + + /* Can likely be a const */ + kfree_const(hist_field->type); kfree(hist_field->system); kfree(hist_field->event_name); @@ -1646,9 +1648,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, if (flags & HIST_FIELD_FL_HITCOUNT) { hist_field->fn = hist_field_counter; hist_field->size = sizeof(u64); - hist_field->type = kstrdup("u64", GFP_KERNEL); - if (!hist_field->type) - goto free; + hist_field->type = "u64"; goto out; } @@ -1662,7 +1662,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, hist_field->fn = hist_field_log2; hist_field->operands[0] = create_hist_field(hist_data, field, fl, NULL); hist_field->size = hist_field->operands[0]->size; - hist_field->type = kstrdup(hist_field->operands[0]->type, GFP_KERNEL); + hist_field->type = kstrdup_const(hist_field->operands[0]->type, GFP_KERNEL); if (!hist_field->type) goto free; goto out; @@ -1671,18 +1671,14 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, if (flags & HIST_FIELD_FL_TIMESTAMP) { hist_field->fn = hist_field_timestamp; hist_field->size = sizeof(u64); - hist_field->type = kstrdup("u64", GFP_KERNEL); - if (!hist_field->type) - goto free; + hist_field->type = "u64"; goto out; } if (flags & HIST_FIELD_FL_CPU) { hist_field->fn = hist_field_cpu; hist_field->size = sizeof(int); - hist_field->type = kstrdup("unsigned int", GFP_KERNEL); - if (!hist_field->type) - goto free; + hist_field->type = "unsigned int"; goto out; } @@ -1695,7 +1691,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, flags |= HIST_FIELD_FL_STRING; hist_field->size = MAX_FILTER_STR_VAL; - hist_field->type = kstrdup(field->type, GFP_KERNEL); + hist_field->type = kstrdup_const(field->type, GFP_KERNEL); if (!hist_field->type) goto free; @@ -1708,7 +1704,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, } else { hist_field->size = field->size; hist_field->is_signed = field->is_signed; - hist_field->type = kstrdup(field->type, GFP_KERNEL); + hist_field->type = kstrdup_const(field->type, GFP_KERNEL); if (!hist_field->type) goto free; @@ -1794,7 +1790,7 @@ static int init_var_ref(struct hist_field *ref_field, } } - ref_field->type = kstrdup(var_field->type, GFP_KERNEL); + ref_field->type = kstrdup_const(var_field->type, GFP_KERNEL); if (!ref_field->type) { err = -ENOMEM; goto free; @@ -2163,7 +2159,7 @@ static struct hist_field *parse_unary(struct hist_trigger_data *hist_data, expr->operands[0] = operand1; expr->operator = FIELD_OP_UNARY_MINUS; expr->name = expr_str(expr, 0); - expr->type = kstrdup(operand1->type, GFP_KERNEL); + expr->type = kstrdup_const(operand1->type, GFP_KERNEL); if (!expr->type) { ret = -ENOMEM; goto free; @@ -2289,7 +2285,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data, expr->operands[1] = operand2; expr->operator = field_op; expr->name = expr_str(expr, 0); - expr->type = kstrdup(operand1->type, GFP_KERNEL); + expr->type = kstrdup_const(operand1->type, GFP_KERNEL); if (!expr->type) { ret = -ENOMEM; goto free; @@ -2677,10 +2673,10 @@ static struct hist_field *create_var(struct hist_trigger_data *hist_data, var->var.hist_data = var->hist_data = hist_data; var->size = size; var->var.name = kstrdup(name, GFP_KERNEL); - var->type = kstrdup(type, GFP_KERNEL); + var->type = kstrdup_const(type, GFP_KERNEL); if (!var->var.name || !var->type) { + kfree_const(var->type); kfree(var->var.name); - kfree(var->type); kfree(var); var = ERR_PTR(-ENOMEM); } -- 2.30.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] tracing: Have histogram types be constant when possible 2021-07-22 14:27 ` [PATCH v2 1/2] tracing: Have histogram types be constant when possible Steven Rostedt @ 2021-07-22 15:24 ` Masami Hiramatsu 2021-07-22 22:11 ` Tom Zanussi 1 sibling, 0 replies; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-22 15:24 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu, Namhyung Kim On Thu, 22 Jul 2021 10:27:06 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > Instead of kstrdup("const", GFP_KERNEL), have the hist_field type simply > assign the constant hist_field->type = "const"; And when the value passed > to it is a variable, use "kstrdup_const(var, GFP_KERNEL);" which will just > copy the value if the variable is already a constant. This saves on having > to allocate when not needed. > > All frees of the hist_field->type will need to use kfree_const(). > Thanks! This looks good to me. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace_events_hist.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 34325f41ebc0..5c9082201bc2 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1589,7 +1589,9 @@ static void __destroy_hist_field(struct hist_field *hist_field) > > kfree(hist_field->var.name); > kfree(hist_field->name); > - kfree(hist_field->type); > + > + /* Can likely be a const */ > + kfree_const(hist_field->type); > > kfree(hist_field->system); > kfree(hist_field->event_name); > @@ -1646,9 +1648,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > if (flags & HIST_FIELD_FL_HITCOUNT) { > hist_field->fn = hist_field_counter; > hist_field->size = sizeof(u64); > - hist_field->type = kstrdup("u64", GFP_KERNEL); > - if (!hist_field->type) > - goto free; > + hist_field->type = "u64"; > goto out; > } > > @@ -1662,7 +1662,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > hist_field->fn = hist_field_log2; > hist_field->operands[0] = create_hist_field(hist_data, field, fl, NULL); > hist_field->size = hist_field->operands[0]->size; > - hist_field->type = kstrdup(hist_field->operands[0]->type, GFP_KERNEL); > + hist_field->type = kstrdup_const(hist_field->operands[0]->type, GFP_KERNEL); > if (!hist_field->type) > goto free; > goto out; > @@ -1671,18 +1671,14 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > if (flags & HIST_FIELD_FL_TIMESTAMP) { > hist_field->fn = hist_field_timestamp; > hist_field->size = sizeof(u64); > - hist_field->type = kstrdup("u64", GFP_KERNEL); > - if (!hist_field->type) > - goto free; > + hist_field->type = "u64"; > goto out; > } > > if (flags & HIST_FIELD_FL_CPU) { > hist_field->fn = hist_field_cpu; > hist_field->size = sizeof(int); > - hist_field->type = kstrdup("unsigned int", GFP_KERNEL); > - if (!hist_field->type) > - goto free; > + hist_field->type = "unsigned int"; > goto out; > } > > @@ -1695,7 +1691,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > flags |= HIST_FIELD_FL_STRING; > > hist_field->size = MAX_FILTER_STR_VAL; > - hist_field->type = kstrdup(field->type, GFP_KERNEL); > + hist_field->type = kstrdup_const(field->type, GFP_KERNEL); > if (!hist_field->type) > goto free; > > @@ -1708,7 +1704,7 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > } else { > hist_field->size = field->size; > hist_field->is_signed = field->is_signed; > - hist_field->type = kstrdup(field->type, GFP_KERNEL); > + hist_field->type = kstrdup_const(field->type, GFP_KERNEL); > if (!hist_field->type) > goto free; > > @@ -1794,7 +1790,7 @@ static int init_var_ref(struct hist_field *ref_field, > } > } > > - ref_field->type = kstrdup(var_field->type, GFP_KERNEL); > + ref_field->type = kstrdup_const(var_field->type, GFP_KERNEL); > if (!ref_field->type) { > err = -ENOMEM; > goto free; > @@ -2163,7 +2159,7 @@ static struct hist_field *parse_unary(struct hist_trigger_data *hist_data, > expr->operands[0] = operand1; > expr->operator = FIELD_OP_UNARY_MINUS; > expr->name = expr_str(expr, 0); > - expr->type = kstrdup(operand1->type, GFP_KERNEL); > + expr->type = kstrdup_const(operand1->type, GFP_KERNEL); > if (!expr->type) { > ret = -ENOMEM; > goto free; > @@ -2289,7 +2285,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data, > expr->operands[1] = operand2; > expr->operator = field_op; > expr->name = expr_str(expr, 0); > - expr->type = kstrdup(operand1->type, GFP_KERNEL); > + expr->type = kstrdup_const(operand1->type, GFP_KERNEL); > if (!expr->type) { > ret = -ENOMEM; > goto free; > @@ -2677,10 +2673,10 @@ static struct hist_field *create_var(struct hist_trigger_data *hist_data, > var->var.hist_data = var->hist_data = hist_data; > var->size = size; > var->var.name = kstrdup(name, GFP_KERNEL); > - var->type = kstrdup(type, GFP_KERNEL); > + var->type = kstrdup_const(type, GFP_KERNEL); > if (!var->var.name || !var->type) { > + kfree_const(var->type); > kfree(var->var.name); > - kfree(var->type); > kfree(var); > var = ERR_PTR(-ENOMEM); > } > -- > 2.30.2 -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] tracing: Have histogram types be constant when possible 2021-07-22 14:27 ` [PATCH v2 1/2] tracing: Have histogram types be constant when possible Steven Rostedt 2021-07-22 15:24 ` Masami Hiramatsu @ 2021-07-22 22:11 ` Tom Zanussi 1 sibling, 0 replies; 18+ messages in thread From: Tom Zanussi @ 2021-07-22 22:11 UTC (permalink / raw) To: Steven Rostedt, linux-kernel Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu, Namhyung Kim On Thu, 2021-07-22 at 10:27 -0400, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > Instead of kstrdup("const", GFP_KERNEL), have the hist_field type > simply > assign the constant hist_field->type = "const"; And when the value > passed > to it is a variable, use "kstrdup_const(var, GFP_KERNEL);" which will > just > copy the value if the variable is already a constant. This saves on > having > to allocate when not needed. > > All frees of the hist_field->type will need to use kfree_const(). > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Looks good to me. Thanks, Reviewed-by: <zanussi@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-22 14:27 [PATCH v2 0/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt 2021-07-22 14:27 ` [PATCH v2 1/2] tracing: Have histogram types be constant when possible Steven Rostedt @ 2021-07-22 14:27 ` Steven Rostedt 2021-07-22 16:19 ` Masami Hiramatsu 2021-07-22 22:09 ` Tom Zanussi 2021-07-22 15:14 ` [PATCH v2 0/2] " Steven Rostedt 2 siblings, 2 replies; 18+ messages in thread From: Steven Rostedt @ 2021-07-22 14:27 UTC (permalink / raw) To: linux-kernel Cc: Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu, Namhyung Kim From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Allow common_pid.execname to be saved in a variable in one histogram to be passed to another histogram that can pass it as a parameter to a synthetic event. ># echo 'hist:keys=pid:__arg__1=common_timestamp.usecs:arg2=common_pid.execname' \ > events/sched/sched_waking/trigger ># echo 'wakeup_lat s32 pid; u64 delta; char wake_comm[]' > synthetic_events ># echo 'hist:keys=next_pid:pid=next_pid,delta=common_timestamp.usecs-$__arg__1,exec=$arg2'\ ':onmatch(sched.sched_waking).trace(wakeup_lat,$pid,$delta,$exec)' \ > events/sched/sched_switch/trigger The above is a wake up latency synthetic event setup that passes the execname of the common_pid that woke the task to the scheduling of that task, which triggers a synthetic event that passes the original execname as a parameter to display it. ># echo 1 > events/synthetic/enable ># cat trace <idle>-0 [006] d..4 186.863801: wakeup_lat: pid=1306 delta=65 wake_comm=kworker/u16:3 <idle>-0 [000] d..4 186.863858: wakeup_lat: pid=163 delta=27 wake_comm=<idle> <idle>-0 [001] d..4 186.863903: wakeup_lat: pid=1307 delta=36 wake_comm=kworker/u16:4 <idle>-0 [000] d..4 186.863927: wakeup_lat: pid=163 delta=5 wake_comm=<idle> <idle>-0 [006] d..4 186.863957: wakeup_lat: pid=1306 delta=24 wake_comm=kworker/u16:3 sshd-1306 [006] d..4 186.864051: wakeup_lat: pid=61 delta=62 wake_comm=<idle> <idle>-0 [000] d..4 186.965030: wakeup_lat: pid=609 delta=18 wake_comm=<idle> <idle>-0 [006] d..4 186.987582: wakeup_lat: pid=1306 delta=65 wake_comm=kworker/u16:3 <idle>-0 [000] d..4 186.987639: wakeup_lat: pid=163 delta=27 wake_comm=<idle> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/trace_events_hist.c | 46 +++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 5c9082201bc2..14b840de1326 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1395,17 +1395,17 @@ static int hist_trigger_elt_data_alloc(struct tracing_map_elt *elt) struct hist_trigger_data *hist_data = elt->map->private_data; unsigned int size = TASK_COMM_LEN; struct hist_elt_data *elt_data; - struct hist_field *key_field; + struct hist_field *hist_field; unsigned int i, n_str; elt_data = kzalloc(sizeof(*elt_data), GFP_KERNEL); if (!elt_data) return -ENOMEM; - for_each_hist_key_field(i, hist_data) { - key_field = hist_data->fields[i]; + for_each_hist_field(i, hist_data) { + hist_field = hist_data->fields[i]; - if (key_field->flags & HIST_FIELD_FL_EXECNAME) { + if (hist_field->flags & HIST_FIELD_FL_EXECNAME) { elt_data->comm = kzalloc(size, GFP_KERNEL); if (!elt_data->comm) { kfree(elt_data); @@ -3703,6 +3703,41 @@ static int create_val_field(struct hist_trigger_data *hist_data, return __create_val_field(hist_data, val_idx, file, NULL, field_str, 0); } +static const char *no_comm = "(no comm)"; + +static u64 hist_field_execname(struct hist_field *hist_field, + struct tracing_map_elt *elt, + struct trace_buffer *buffer, + struct ring_buffer_event *rbe, + void *event) +{ + struct hist_elt_data *elt_data; + + if (WARN_ON_ONCE(!elt)) + return (u64)(unsigned long)no_comm; + + elt_data = elt->private_data; + + if (WARN_ON_ONCE(!elt_data->comm)) + return (u64)(unsigned long)no_comm; + + return (u64)(unsigned long)(elt_data->comm); +} + +/* Convert a var that points to common_pid.execname to a string */ +static void update_var_execname(struct hist_field *hist_field) +{ + hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | + HIST_FIELD_FL_EXECNAME; + hist_field->size = MAX_FILTER_STR_VAL; + hist_field->is_signed = 0; + + kfree_const(hist_field->type); + hist_field->type = "char[]"; + + hist_field->fn = hist_field_execname; +} + static int create_var_field(struct hist_trigger_data *hist_data, unsigned int val_idx, struct trace_event_file *file, @@ -3727,6 +3762,9 @@ static int create_var_field(struct hist_trigger_data *hist_data, ret = __create_val_field(hist_data, val_idx, file, var_name, expr_str, flags); + if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_EXECNAME) + update_var_execname(hist_data->fields[val_idx]); + if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_STRING) hist_data->fields[val_idx]->var_str_idx = hist_data->n_var_str++; -- 2.30.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-22 14:27 ` [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt @ 2021-07-22 16:19 ` Masami Hiramatsu 2021-07-22 16:32 ` Steven Rostedt 2021-07-22 22:09 ` Tom Zanussi 1 sibling, 1 reply; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-22 16:19 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu, Namhyung Kim Hi Steve, On Thu, 22 Jul 2021 10:27:07 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > Allow common_pid.execname to be saved in a variable in one histogram to be > passed to another histogram that can pass it as a parameter to a synthetic > event. > > ># echo 'hist:keys=pid:__arg__1=common_timestamp.usecs:arg2=common_pid.execname' \ > > events/sched/sched_waking/trigger > ># echo 'wakeup_lat s32 pid; u64 delta; char wake_comm[]' > synthetic_events > ># echo 'hist:keys=next_pid:pid=next_pid,delta=common_timestamp.usecs-$__arg__1,exec=$arg2'\ > ':onmatch(sched.sched_waking).trace(wakeup_lat,$pid,$delta,$exec)' \ > > events/sched/sched_switch/trigger > > The above is a wake up latency synthetic event setup that passes the execname > of the common_pid that woke the task to the scheduling of that task, which > triggers a synthetic event that passes the original execname as a > parameter to display it. > > ># echo 1 > events/synthetic/enable > ># cat trace > <idle>-0 [006] d..4 186.863801: wakeup_lat: pid=1306 delta=65 wake_comm=kworker/u16:3 > <idle>-0 [000] d..4 186.863858: wakeup_lat: pid=163 delta=27 wake_comm=<idle> > <idle>-0 [001] d..4 186.863903: wakeup_lat: pid=1307 delta=36 wake_comm=kworker/u16:4 > <idle>-0 [000] d..4 186.863927: wakeup_lat: pid=163 delta=5 wake_comm=<idle> > <idle>-0 [006] d..4 186.863957: wakeup_lat: pid=1306 delta=24 wake_comm=kworker/u16:3 > sshd-1306 [006] d..4 186.864051: wakeup_lat: pid=61 delta=62 wake_comm=<idle> > <idle>-0 [000] d..4 186.965030: wakeup_lat: pid=609 delta=18 wake_comm=<idle> > <idle>-0 [006] d..4 186.987582: wakeup_lat: pid=1306 delta=65 wake_comm=kworker/u16:3 > <idle>-0 [000] d..4 186.987639: wakeup_lat: pid=163 delta=27 wake_comm=<idle> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace_events_hist.c | 46 +++++++++++++++++++++++++++++--- > 1 file changed, 42 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 5c9082201bc2..14b840de1326 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1395,17 +1395,17 @@ static int hist_trigger_elt_data_alloc(struct tracing_map_elt *elt) > struct hist_trigger_data *hist_data = elt->map->private_data; > unsigned int size = TASK_COMM_LEN; > struct hist_elt_data *elt_data; > - struct hist_field *key_field; > + struct hist_field *hist_field; > unsigned int i, n_str; > > elt_data = kzalloc(sizeof(*elt_data), GFP_KERNEL); > if (!elt_data) > return -ENOMEM; > > - for_each_hist_key_field(i, hist_data) { > - key_field = hist_data->fields[i]; > + for_each_hist_field(i, hist_data) { > + hist_field = hist_data->fields[i]; > > - if (key_field->flags & HIST_FIELD_FL_EXECNAME) { > + if (hist_field->flags & HIST_FIELD_FL_EXECNAME) { > elt_data->comm = kzalloc(size, GFP_KERNEL); > if (!elt_data->comm) { > kfree(elt_data); > @@ -3703,6 +3703,41 @@ static int create_val_field(struct hist_trigger_data *hist_data, > return __create_val_field(hist_data, val_idx, file, NULL, field_str, 0); > } > > +static const char *no_comm = "(no comm)"; > + > +static u64 hist_field_execname(struct hist_field *hist_field, > + struct tracing_map_elt *elt, > + struct trace_buffer *buffer, > + struct ring_buffer_event *rbe, > + void *event) > +{ > + struct hist_elt_data *elt_data; > + > + if (WARN_ON_ONCE(!elt)) > + return (u64)(unsigned long)no_comm; > + > + elt_data = elt->private_data; > + > + if (WARN_ON_ONCE(!elt_data->comm)) > + return (u64)(unsigned long)no_comm; > + > + return (u64)(unsigned long)(elt_data->comm); > +} > + > +/* Convert a var that points to common_pid.execname to a string */ > +static void update_var_execname(struct hist_field *hist_field) > +{ > + hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | > + HIST_FIELD_FL_EXECNAME; > + hist_field->size = MAX_FILTER_STR_VAL; > + hist_field->is_signed = 0; > + > + kfree_const(hist_field->type); > + hist_field->type = "char[]"; > + > + hist_field->fn = hist_field_execname; > +} Hmm, this is a bit ad-hoc. Can't this be done in the create_hist_field()? If you check 'var_name' and flags & HIST_FIELD_FL_EXECNAME, you can do the same thing I think. Thank you, > + > static int create_var_field(struct hist_trigger_data *hist_data, > unsigned int val_idx, > struct trace_event_file *file, > @@ -3727,6 +3762,9 @@ static int create_var_field(struct hist_trigger_data *hist_data, > > ret = __create_val_field(hist_data, val_idx, file, var_name, expr_str, flags); > > + if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_EXECNAME) > + update_var_execname(hist_data->fields[val_idx]); > + > if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_STRING) > hist_data->fields[val_idx]->var_str_idx = hist_data->n_var_str++; > > -- > 2.30.2 -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-22 16:19 ` Masami Hiramatsu @ 2021-07-22 16:32 ` Steven Rostedt 2021-07-23 1:11 ` Masami Hiramatsu 0 siblings, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2021-07-22 16:32 UTC (permalink / raw) To: Masami Hiramatsu Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Fri, 23 Jul 2021 01:19:35 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > > +/* Convert a var that points to common_pid.execname to a string */ > > +static void update_var_execname(struct hist_field *hist_field) > > +{ > > + hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | > > + HIST_FIELD_FL_EXECNAME; > > + hist_field->size = MAX_FILTER_STR_VAL; > > + hist_field->is_signed = 0; > > + > > + kfree_const(hist_field->type); > > + hist_field->type = "char[]"; > > + > > + hist_field->fn = hist_field_execname; > > +} > > Hmm, this is a bit ad-hoc. > > Can't this be done in the create_hist_field()? If you check 'var_name' and > flags & HIST_FIELD_FL_EXECNAME, you can do the same thing I think. Hi Masami, I originally tried that, but then found that it converted the pid over to it as well. So this must be done only for vars, and not only that, it needs to be done in a single place, because I was spending hours debugging it. I found this to be the least intrusive solution. Maybe Tom has a better idea, but I don't have any more time to work on it, and I really want this feature for the next merge window. If you can make it work, and have time to play with it, I'm happy to take an alternative :-) -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-22 16:32 ` Steven Rostedt @ 2021-07-23 1:11 ` Masami Hiramatsu 2021-07-23 1:22 ` Masami Hiramatsu 2021-07-23 1:24 ` Steven Rostedt 0 siblings, 2 replies; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-23 1:11 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Thu, 22 Jul 2021 12:32:34 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 23 Jul 2021 01:19:35 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > +/* Convert a var that points to common_pid.execname to a string */ > > > +static void update_var_execname(struct hist_field *hist_field) > > > +{ > > > + hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | > > > + HIST_FIELD_FL_EXECNAME; > > > + hist_field->size = MAX_FILTER_STR_VAL; > > > + hist_field->is_signed = 0; > > > + > > > + kfree_const(hist_field->type); > > > + hist_field->type = "char[]"; > > > + > > > + hist_field->fn = hist_field_execname; > > > +} > > > > Hmm, this is a bit ad-hoc. > > > > Can't this be done in the create_hist_field()? If you check 'var_name' and > > flags & HIST_FIELD_FL_EXECNAME, you can do the same thing I think. > > Hi Masami, > > I originally tried that, but then found that it converted the pid over > to it as well. So this must be done only for vars, and not only that, it > needs to be done in a single place, because I was spending hours > debugging it. I understand. As far as I can see the code, it looks a bit complicated. To simplify it, I need to understand the spec for "hist_field" for keys and for vars. And maybe need to split both case. > I found this to be the least intrusive solution. > > Maybe Tom has a better idea, but I don't have any more time to work on > it, and I really want this feature for the next merge window. > > If you can make it work, and have time to play with it, I'm happy to > take an alternative :-) Me neither at least this moment, need more investigation. Let me try. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-23 1:11 ` Masami Hiramatsu @ 2021-07-23 1:22 ` Masami Hiramatsu 2021-07-23 1:26 ` Steven Rostedt 2021-07-23 1:24 ` Steven Rostedt 1 sibling, 1 reply; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-23 1:22 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Fri, 23 Jul 2021 10:11:33 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Thu, 22 Jul 2021 12:32:34 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Fri, 23 Jul 2021 01:19:35 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > +/* Convert a var that points to common_pid.execname to a string */ > > > > +static void update_var_execname(struct hist_field *hist_field) > > > > +{ > > > > + hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | > > > > + HIST_FIELD_FL_EXECNAME; > > > > + hist_field->size = MAX_FILTER_STR_VAL; > > > > + hist_field->is_signed = 0; > > > > + > > > > + kfree_const(hist_field->type); > > > > + hist_field->type = "char[]"; > > > > + > > > > + hist_field->fn = hist_field_execname; > > > > +} > > > > > > Hmm, this is a bit ad-hoc. > > > > > > Can't this be done in the create_hist_field()? If you check 'var_name' and > > > flags & HIST_FIELD_FL_EXECNAME, you can do the same thing I think. > > > > Hi Masami, > > > > I originally tried that, but then found that it converted the pid over > > to it as well. So this must be done only for vars, and not only that, it > > needs to be done in a single place, because I was spending hours > > debugging it. > > I understand. As far as I can see the code, it looks a bit complicated. > To simplify it, I need to understand the spec for "hist_field" > for keys and for vars. And maybe need to split both case. > > > I found this to be the least intrusive solution. > > > > Maybe Tom has a better idea, but I don't have any more time to work on > > it, and I really want this feature for the next merge window. > > > > If you can make it work, and have time to play with it, I'm happy to > > take an alternative :-) > > Me neither at least this moment, need more investigation. Let me try. But anyway, maybe I need this weekend to make a time. So, as far as it works OK, I'm OK for this patch. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> BTW, please update the ftracetest testcases for hist triggers. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-23 1:22 ` Masami Hiramatsu @ 2021-07-23 1:26 ` Steven Rostedt 0 siblings, 0 replies; 18+ messages in thread From: Steven Rostedt @ 2021-07-23 1:26 UTC (permalink / raw) To: Masami Hiramatsu Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Fri, 23 Jul 2021 10:22:11 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > But anyway, maybe I need this weekend to make a time. > So, as far as it works OK, I'm OK for this patch. > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks! I'll probably wont apply it till next week anyway because I'm currently debugging other issues :-p > > BTW, please update the ftracetest testcases for hist triggers. Will do! Thanks for the review. -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-23 1:11 ` Masami Hiramatsu 2021-07-23 1:22 ` Masami Hiramatsu @ 2021-07-23 1:24 ` Steven Rostedt 2021-07-24 10:31 ` Masami Hiramatsu 1 sibling, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2021-07-23 1:24 UTC (permalink / raw) To: Masami Hiramatsu Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Fri, 23 Jul 2021 10:11:33 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > I understand. As far as I can see the code, it looks a bit complicated. > To simplify it, I need to understand the spec for "hist_field" > for keys and for vars. And maybe need to split both case. I'll give you a hint that took me a bit to figure out. 1) The execname is saved at the start of the histogram and not by one of the ->fn() functions. It's saved by hist_trigger_elt_data_init() if the elt_data->comm is allocated. That function is part of the "tracing_map_ops" which gets assigned by tracing_map_create() (in tracing_map.c) as the "elt_init" function, which is called when getting a new elt element by get_free_elt(). 2) That elt_data->comm is only allocated if it finds a "hist_field" that has HIST_FIELD_FL_EXECNAME flag set. It currently only looks for that flag in the "keys" fields, which means that .execname is useless for everything else. This patch changed it to search all hist_fields so that it can find that flag if a variable has it set (which I added). > > > I found this to be the least intrusive solution. > > > > Maybe Tom has a better idea, but I don't have any more time to work on > > it, and I really want this feature for the next merge window. > > > > If you can make it work, and have time to play with it, I'm happy to > > take an alternative :-) > > Me neither at least this moment, need more investigation. Let me try. Great! I can hold off on adding this. Or I can add it, and if you come up with a better solution, we can just swap it. -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-23 1:24 ` Steven Rostedt @ 2021-07-24 10:31 ` Masami Hiramatsu 2021-07-25 2:18 ` Masami Hiramatsu 0 siblings, 1 reply; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-24 10:31 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim Hi Steve, On Thu, 22 Jul 2021 21:24:38 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 23 Jul 2021 10:11:33 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > I understand. As far as I can see the code, it looks a bit complicated. > > To simplify it, I need to understand the spec for "hist_field" > > for keys and for vars. And maybe need to split both case. > > I'll give you a hint that took me a bit to figure out. > > 1) The execname is saved at the start of the histogram and not by one > of the ->fn() functions. > > It's saved by hist_trigger_elt_data_init() if the elt_data->comm is > allocated. That function is part of the "tracing_map_ops" which gets > assigned by tracing_map_create() (in tracing_map.c) as the "elt_init" > function, which is called when getting a new elt element by > get_free_elt(). > > 2) That elt_data->comm is only allocated if it finds a "hist_field" > that has HIST_FIELD_FL_EXECNAME flag set. It currently only looks for > that flag in the "keys" fields, which means that .execname is useless > for everything else. This patch changed it to search all hist_fields so > that it can find that flag if a variable has it set (which I added). Thanks for the hints, but actually, that part looks good to me. So, what I pointed was the part of update_var_execname(). Below diff is what I intended. This moves HIST_FIELD_FL_EXECNAME setup in the create_hist_field() as same as other flags, and removed the add-hoc update_var_execname() fixup function. I confirmed it passed the ftracetest trigger testcases and your example code. Thank you, --- kernel/trace/trace_events_hist.c | 69 ++++++++++++++------------------ 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 14b840de1326..2fab91a22628 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -492,6 +492,27 @@ struct snapshot_context { void *key; }; +static const char *no_comm = "(no comm)"; + +static u64 hist_field_execname(struct hist_field *hist_field, + struct tracing_map_elt *elt, + struct trace_buffer *buffer, + struct ring_buffer_event *rbe, + void *event) +{ + struct hist_elt_data *elt_data; + + if (WARN_ON_ONCE(!elt)) + return (u64)(unsigned long)no_comm; + + elt_data = elt->private_data; + + if (WARN_ON_ONCE(!elt_data->comm)) + return (u64)(unsigned long)no_comm; + + return (u64)(unsigned long)(elt_data->comm); +} + static void track_data_free(struct track_data *track_data) { struct hist_elt_data *elt_data; @@ -1682,6 +1703,16 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, goto out; } + if ((flags & HIST_FIELD_FL_EXECNAME) && var_name) { + flags |= HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR; + hist_field->size = MAX_FILTER_STR_VAL; + hist_field->is_signed = 0; + + hist_field->type = "char[]"; + hist_field->fn = hist_field_execname; + goto out; + } + if (WARN_ON_ONCE(!field)) goto out; @@ -3703,41 +3734,6 @@ static int create_val_field(struct hist_trigger_data *hist_data, return __create_val_field(hist_data, val_idx, file, NULL, field_str, 0); } -static const char *no_comm = "(no comm)"; - -static u64 hist_field_execname(struct hist_field *hist_field, - struct tracing_map_elt *elt, - struct trace_buffer *buffer, - struct ring_buffer_event *rbe, - void *event) -{ - struct hist_elt_data *elt_data; - - if (WARN_ON_ONCE(!elt)) - return (u64)(unsigned long)no_comm; - - elt_data = elt->private_data; - - if (WARN_ON_ONCE(!elt_data->comm)) - return (u64)(unsigned long)no_comm; - - return (u64)(unsigned long)(elt_data->comm); -} - -/* Convert a var that points to common_pid.execname to a string */ -static void update_var_execname(struct hist_field *hist_field) -{ - hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | - HIST_FIELD_FL_EXECNAME; - hist_field->size = MAX_FILTER_STR_VAL; - hist_field->is_signed = 0; - - kfree_const(hist_field->type); - hist_field->type = "char[]"; - - hist_field->fn = hist_field_execname; -} - static int create_var_field(struct hist_trigger_data *hist_data, unsigned int val_idx, struct trace_event_file *file, @@ -3762,9 +3758,6 @@ static int create_var_field(struct hist_trigger_data *hist_data, ret = __create_val_field(hist_data, val_idx, file, var_name, expr_str, flags); - if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_EXECNAME) - update_var_execname(hist_data->fields[val_idx]); - if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_STRING) hist_data->fields[val_idx]->var_str_idx = hist_data->n_var_str++; -- 2.25.1 -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-24 10:31 ` Masami Hiramatsu @ 2021-07-25 2:18 ` Masami Hiramatsu 2021-07-25 3:45 ` Masami Hiramatsu 0 siblings, 1 reply; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-25 2:18 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim Hi, On Sat, 24 Jul 2021 19:31:45 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > Hi Steve, > > On Thu, 22 Jul 2021 21:24:38 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Fri, 23 Jul 2021 10:11:33 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > I understand. As far as I can see the code, it looks a bit complicated. > > > To simplify it, I need to understand the spec for "hist_field" > > > for keys and for vars. And maybe need to split both case. > > > > I'll give you a hint that took me a bit to figure out. > > > > 1) The execname is saved at the start of the histogram and not by one > > of the ->fn() functions. > > > > It's saved by hist_trigger_elt_data_init() if the elt_data->comm is > > allocated. That function is part of the "tracing_map_ops" which gets > > assigned by tracing_map_create() (in tracing_map.c) as the "elt_init" > > function, which is called when getting a new elt element by > > get_free_elt(). > > > > 2) That elt_data->comm is only allocated if it finds a "hist_field" > > that has HIST_FIELD_FL_EXECNAME flag set. It currently only looks for > > that flag in the "keys" fields, which means that .execname is useless > > for everything else. This patch changed it to search all hist_fields so > > that it can find that flag if a variable has it set (which I added). > > Thanks for the hints, but actually, that part looks good to me. > > So, what I pointed was the part of update_var_execname(). Below diff > is what I intended. > This moves HIST_FIELD_FL_EXECNAME setup in the create_hist_field() > as same as other flags, and removed the add-hoc update_var_execname() > fixup function. > > I confirmed it passed the ftracetest trigger testcases and your > example code. > > Thank you, > I found a bug in this change. [..] > @@ -1682,6 +1703,16 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > goto out; > } > > + if ((flags & HIST_FIELD_FL_EXECNAME) && var_name) { > + flags |= HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR; Here, we don't need to check 'var_name' and remove HIST_FIELD_FL_VAR, since it must be set in the flag. if (flags & HIST_FIELD_FL_EXECNAME) { flags |= HIST_FIELD_FL_STRING; > + hist_field->size = MAX_FILTER_STR_VAL; > + hist_field->is_signed = 0; > + > + hist_field->type = "char[]"; > + hist_field->fn = hist_field_execname; > + goto out; > + } > + Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-25 2:18 ` Masami Hiramatsu @ 2021-07-25 3:45 ` Masami Hiramatsu 2021-07-26 13:28 ` Steven Rostedt 0 siblings, 1 reply; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-25 3:45 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Sun, 25 Jul 2021 11:18:30 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > Hi, > > On Sat, 24 Jul 2021 19:31:45 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Hi Steve, > > > > On Thu, 22 Jul 2021 21:24:38 -0400 > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > On Fri, 23 Jul 2021 10:11:33 +0900 > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > I understand. As far as I can see the code, it looks a bit complicated. > > > > To simplify it, I need to understand the spec for "hist_field" > > > > for keys and for vars. And maybe need to split both case. > > > > > > I'll give you a hint that took me a bit to figure out. > > > > > > 1) The execname is saved at the start of the histogram and not by one > > > of the ->fn() functions. > > > > > > It's saved by hist_trigger_elt_data_init() if the elt_data->comm is > > > allocated. That function is part of the "tracing_map_ops" which gets > > > assigned by tracing_map_create() (in tracing_map.c) as the "elt_init" > > > function, which is called when getting a new elt element by > > > get_free_elt(). > > > > > > 2) That elt_data->comm is only allocated if it finds a "hist_field" > > > that has HIST_FIELD_FL_EXECNAME flag set. It currently only looks for > > > that flag in the "keys" fields, which means that .execname is useless > > > for everything else. This patch changed it to search all hist_fields so > > > that it can find that flag if a variable has it set (which I added). > > > > Thanks for the hints, but actually, that part looks good to me. > > > > So, what I pointed was the part of update_var_execname(). Below diff > > is what I intended. > > This moves HIST_FIELD_FL_EXECNAME setup in the create_hist_field() > > as same as other flags, and removed the add-hoc update_var_execname() > > fixup function. > > > > I confirmed it passed the ftracetest trigger testcases and your > > example code. > > > > Thank you, > > > > I found a bug in this change. > > [..] > > @@ -1682,6 +1703,16 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > > goto out; > > } > > > > + if ((flags & HIST_FIELD_FL_EXECNAME) && var_name) { > > + flags |= HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR; > > Here, we don't need to check 'var_name' and remove HIST_FIELD_FL_VAR, since it must be set in the flag. > > if (flags & HIST_FIELD_FL_EXECNAME) { > flags |= HIST_FIELD_FL_STRING; And with this change, hist trigger can correctly detect a string type in the operand in the expression and rejects it. Thank you, From 5280d1efe4415a621cf69a1dc4861ab928b0ff1c Mon Sep 17 00:00:00 2001 From: Masami Hiramatsu <mhiramat@kernel.org> Date: Sun, 25 Jul 2021 12:34:00 +0900 Subject: [PATCH] tracing: Reject string operand in the histogram expression Since the string type can not be the target of the addition / subtraction operation, it must be rejected. Without this fix, the string type silently converted to digits. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- kernel/trace/trace_events_hist.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 76e3100a4840..3eea60e2da48 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -65,7 +65,8 @@ C(INVALID_SORT_MODIFIER,"Invalid sort modifier"), \ C(EMPTY_SORT_FIELD, "Empty sort field"), \ C(TOO_MANY_SORT_FIELDS, "Too many sort fields (Max = 2)"), \ - C(INVALID_SORT_FIELD, "Sort field must be a key or a val"), + C(INVALID_SORT_FIELD, "Sort field must be a key or a val"), \ + C(INVALID_STR_OPERAND, "String type can not be an operand in expression"), #undef C #define C(a, b) HIST_ERR_##a @@ -2183,6 +2184,13 @@ static struct hist_field *parse_unary(struct hist_trigger_data *hist_data, ret = PTR_ERR(operand1); goto free; } + if (operand1->flags & HIST_FIELD_FL_STRING) { + /* String type can not be the operand of unary operator. */ + hist_err(file->tr, HIST_ERR_INVALID_STR_OPERAND, errpos(str)); + destroy_hist_field(operand1, 0); + ret = -EINVAL; + goto free; + } expr->flags |= operand1->flags & (HIST_FIELD_FL_TIMESTAMP | HIST_FIELD_FL_TIMESTAMP_USECS); @@ -2284,6 +2292,11 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data, operand1 = NULL; goto free; } + if (operand1->flags & HIST_FIELD_FL_STRING) { + hist_err(file->tr, HIST_ERR_INVALID_STR_OPERAND, errpos(operand1_str)); + ret = -EINVAL; + goto free; + } /* rest of string could be another expression e.g. b+c in a+b+c */ operand_flags = 0; @@ -2293,6 +2306,11 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data, operand2 = NULL; goto free; } + if (operand2->flags & HIST_FIELD_FL_STRING) { + hist_err(file->tr, HIST_ERR_INVALID_STR_OPERAND, errpos(str)); + ret = -EINVAL; + goto free; + } ret = check_expr_operands(file->tr, operand1, operand2); if (ret) -- 2.25.1 -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-25 3:45 ` Masami Hiramatsu @ 2021-07-26 13:28 ` Steven Rostedt 2021-07-27 22:54 ` Masami Hiramatsu 0 siblings, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2021-07-26 13:28 UTC (permalink / raw) To: Masami Hiramatsu Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Sun, 25 Jul 2021 12:45:02 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > And with this change, hist trigger can correctly detect a string type in > the operand in the expression and rejects it. > > Thank you, > > >From 5280d1efe4415a621cf69a1dc4861ab928b0ff1c Mon Sep 17 00:00:00 2001 > From: Masami Hiramatsu <mhiramat@kernel.org> > Date: Sun, 25 Jul 2021 12:34:00 +0900 > Subject: [PATCH] tracing: Reject string operand in the histogram expression > > Since the string type can not be the target of the addition / subtraction > operation, it must be rejected. Without this fix, the string type silently > converted to digits. Masami, can you send this as a normal patch. My scripts do not work (nor do I plan on having them ever do so) with patches embedded in threads or non-patch emails. Thanks, -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-26 13:28 ` Steven Rostedt @ 2021-07-27 22:54 ` Masami Hiramatsu 0 siblings, 0 replies; 18+ messages in thread From: Masami Hiramatsu @ 2021-07-27 22:54 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Tom Zanussi, Namhyung Kim On Mon, 26 Jul 2021 09:28:18 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sun, 25 Jul 2021 12:45:02 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > And with this change, hist trigger can correctly detect a string type in > > the operand in the expression and rejects it. > > > > Thank you, > > > > >From 5280d1efe4415a621cf69a1dc4861ab928b0ff1c Mon Sep 17 00:00:00 2001 > > From: Masami Hiramatsu <mhiramat@kernel.org> > > Date: Sun, 25 Jul 2021 12:34:00 +0900 > > Subject: [PATCH] tracing: Reject string operand in the histogram expression > > > > Since the string type can not be the target of the addition / subtraction > > operation, it must be rejected. Without this fix, the string type silently > > converted to digits. > > Masami, can you send this as a normal patch. My scripts do not work > (nor do I plan on having them ever do so) with patches embedded in > threads or non-patch emails. OK, let me resend it. Thanks, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-22 14:27 ` [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt 2021-07-22 16:19 ` Masami Hiramatsu @ 2021-07-22 22:09 ` Tom Zanussi 1 sibling, 0 replies; 18+ messages in thread From: Tom Zanussi @ 2021-07-22 22:09 UTC (permalink / raw) To: Steven Rostedt, linux-kernel Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu, Namhyung Kim Hi Steve, On Thu, 2021-07-22 at 10:27 -0400, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > Allow common_pid.execname to be saved in a variable in one histogram > to be > passed to another histogram that can pass it as a parameter to a > synthetic > event. > > ># echo > 'hist:keys=pid:__arg__1=common_timestamp.usecs:arg2=common_pid.execna > me' \ > > events/sched/sched_waking/trigger > ># echo 'wakeup_lat s32 pid; u64 delta; char wake_comm[]' > > synthetic_events > ># echo > 'hist:keys=next_pid:pid=next_pid,delta=common_timestamp.usecs- > $__arg__1,exec=$arg2'\ > ':onmatch(sched.sched_waking).trace(wakeup_lat,$pid,$delta,$exec)' \ > > events/sched/sched_switch/trigger > > The above is a wake up latency synthetic event setup that passes the > execname > of the common_pid that woke the task to the scheduling of that task, > which > triggers a synthetic event that passes the original execname as a > parameter to display it. > > ># echo 1 > events/synthetic/enable > ># cat trace > <idle>-0 [006] d..4 186.863801: wakeup_lat: pid=1306 > delta=65 wake_comm=kworker/u16:3 > <idle>-0 [000] d..4 186.863858: wakeup_lat: pid=163 > delta=27 wake_comm=<idle> > <idle>-0 [001] d..4 186.863903: wakeup_lat: pid=1307 > delta=36 wake_comm=kworker/u16:4 > <idle>-0 [000] d..4 186.863927: wakeup_lat: pid=163 > delta=5 wake_comm=<idle> > <idle>-0 [006] d..4 186.863957: wakeup_lat: pid=1306 > delta=24 wake_comm=kworker/u16:3 > sshd-1306 [006] d..4 186.864051: wakeup_lat: pid=61 > delta=62 wake_comm=<idle> > <idle>-0 [000] d..4 186.965030: wakeup_lat: pid=609 > delta=18 wake_comm=<idle> > <idle>-0 [006] d..4 186.987582: wakeup_lat: pid=1306 > delta=65 wake_comm=kworker/u16:3 > <idle>-0 [000] d..4 186.987639: wakeup_lat: pid=163 > delta=27 wake_comm=<idle> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace_events_hist.c | 46 +++++++++++++++++++++++++++++- > -- > 1 file changed, 42 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 5c9082201bc2..14b840de1326 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1395,17 +1395,17 @@ static int hist_trigger_elt_data_alloc(struct > tracing_map_elt *elt) > struct hist_trigger_data *hist_data = elt->map->private_data; > unsigned int size = TASK_COMM_LEN; > struct hist_elt_data *elt_data; > - struct hist_field *key_field; > + struct hist_field *hist_field; > unsigned int i, n_str; > > elt_data = kzalloc(sizeof(*elt_data), GFP_KERNEL); > if (!elt_data) > return -ENOMEM; > > - for_each_hist_key_field(i, hist_data) { > - key_field = hist_data->fields[i]; > + for_each_hist_field(i, hist_data) { > + hist_field = hist_data->fields[i]; > > - if (key_field->flags & HIST_FIELD_FL_EXECNAME) { > + if (hist_field->flags & HIST_FIELD_FL_EXECNAME) { > elt_data->comm = kzalloc(size, GFP_KERNEL); > if (!elt_data->comm) { > kfree(elt_data); > @@ -3703,6 +3703,41 @@ static int create_val_field(struct > hist_trigger_data *hist_data, > return __create_val_field(hist_data, val_idx, file, NULL, > field_str, 0); > } > > +static const char *no_comm = "(no comm)"; > + > +static u64 hist_field_execname(struct hist_field *hist_field, > + struct tracing_map_elt *elt, > + struct trace_buffer *buffer, > + struct ring_buffer_event *rbe, > + void *event) > +{ > + struct hist_elt_data *elt_data; > + > + if (WARN_ON_ONCE(!elt)) > + return (u64)(unsigned long)no_comm; > + > + elt_data = elt->private_data; > + > + if (WARN_ON_ONCE(!elt_data->comm)) > + return (u64)(unsigned long)no_comm; > + > + return (u64)(unsigned long)(elt_data->comm); > +} > + > +/* Convert a var that points to common_pid.execname to a string */ > +static void update_var_execname(struct hist_field *hist_field) > +{ > + hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | > + HIST_FIELD_FL_EXECNAME; > + hist_field->size = MAX_FILTER_STR_VAL; > + hist_field->is_signed = 0; > + > + kfree_const(hist_field->type); > + hist_field->type = "char[]"; > + > + hist_field->fn = hist_field_execname; > +} > + > static int create_var_field(struct hist_trigger_data *hist_data, > unsigned int val_idx, > struct trace_event_file *file, > @@ -3727,6 +3762,9 @@ static int create_var_field(struct > hist_trigger_data *hist_data, > > ret = __create_val_field(hist_data, val_idx, file, var_name, > expr_str, flags); > > + if (!ret && hist_data->fields[val_idx]->flags & > HIST_FIELD_FL_EXECNAME) > + update_var_execname(hist_data->fields[val_idx]); > + > if (!ret && hist_data->fields[val_idx]->flags & > HIST_FIELD_FL_STRING) > hist_data->fields[val_idx]->var_str_idx = hist_data- > >n_var_str++; > Yeah, the common_pid.execname thing is kind of a strange beast, and I think this is actually a nice and reasonable way to deal with it for this case. Thanks, Reviewed-by: <zanussi@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] tracing: Allow execnames to be passed as args for synthetic events 2021-07-22 14:27 [PATCH v2 0/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt 2021-07-22 14:27 ` [PATCH v2 1/2] tracing: Have histogram types be constant when possible Steven Rostedt 2021-07-22 14:27 ` [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt @ 2021-07-22 15:14 ` Steven Rostedt 2 siblings, 0 replies; 18+ messages in thread From: Steven Rostedt @ 2021-07-22 15:14 UTC (permalink / raw) To: linux-kernel Cc: Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu, Namhyung Kim On Thu, 22 Jul 2021 10:27:05 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > Changes since v1: > > - Added patch to have all types be possible const (Masami Hiramatsu) Actually, this should have been v3 and the above is changes since v2. :-/ -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-07-27 22:54 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-22 14:27 [PATCH v2 0/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt 2021-07-22 14:27 ` [PATCH v2 1/2] tracing: Have histogram types be constant when possible Steven Rostedt 2021-07-22 15:24 ` Masami Hiramatsu 2021-07-22 22:11 ` Tom Zanussi 2021-07-22 14:27 ` [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events Steven Rostedt 2021-07-22 16:19 ` Masami Hiramatsu 2021-07-22 16:32 ` Steven Rostedt 2021-07-23 1:11 ` Masami Hiramatsu 2021-07-23 1:22 ` Masami Hiramatsu 2021-07-23 1:26 ` Steven Rostedt 2021-07-23 1:24 ` Steven Rostedt 2021-07-24 10:31 ` Masami Hiramatsu 2021-07-25 2:18 ` Masami Hiramatsu 2021-07-25 3:45 ` Masami Hiramatsu 2021-07-26 13:28 ` Steven Rostedt 2021-07-27 22:54 ` Masami Hiramatsu 2021-07-22 22:09 ` Tom Zanussi 2021-07-22 15:14 ` [PATCH v2 0/2] " Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).