| Message ID | 20230720163056.2564824-3-vschneid@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | context_tracking,x86: Defer some IPIs until a user->kernel transition | expand |
On Thu, Jul 20, 2023 at 05:30:38PM +0100, Valentin Schneider wrote: > int filter_assign_type(const char *type) > { > - if (strstr(type, "__data_loc") && strstr(type, "char")) > - return FILTER_DYN_STRING; > + if (strstr(type, "__data_loc")) { > + if (strstr(type, "char")) > + return FILTER_DYN_STRING; > + if (strstr(type, "cpumask_t")) > + return FILTER_CPUMASK; > + } The closing bracket has the wrong indentation. > + /* Copy the cpulist between { and } */ > + tmp = kmalloc((i - maskstart) + 1, GFP_KERNEL); > + strscpy(tmp, str + maskstart, (i - maskstart) + 1); Need to check kmalloc() failure? And also free tmp? > + > + pred->mask = kzalloc(cpumask_size(), GFP_KERNEL); > + if (!pred->mask) > + goto err_mem; > + > + /* Now parse it */ > + if (cpulist_parse(tmp, pred->mask)) { > + parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i); > + goto err_free; > + } > + > + /* Move along */ > + i++; > + if (field->filter_type == FILTER_CPUMASK) > + pred->fn_num = FILTER_PRED_FN_CPUMASK; > +
On 26/07/23 12:41, Josh Poimboeuf wrote: > On Thu, Jul 20, 2023 at 05:30:38PM +0100, Valentin Schneider wrote: >> int filter_assign_type(const char *type) >> { >> - if (strstr(type, "__data_loc") && strstr(type, "char")) >> - return FILTER_DYN_STRING; >> + if (strstr(type, "__data_loc")) { >> + if (strstr(type, "char")) >> + return FILTER_DYN_STRING; >> + if (strstr(type, "cpumask_t")) >> + return FILTER_CPUMASK; >> + } > > The closing bracket has the wrong indentation. > >> + /* Copy the cpulist between { and } */ >> + tmp = kmalloc((i - maskstart) + 1, GFP_KERNEL); >> + strscpy(tmp, str + maskstart, (i - maskstart) + 1); > > Need to check kmalloc() failure? And also free tmp? > Heh, indeed, shoddy that :-) Thanks! >> + >> + pred->mask = kzalloc(cpumask_size(), GFP_KERNEL); >> + if (!pred->mask) >> + goto err_mem; >> + >> + /* Now parse it */ >> + if (cpulist_parse(tmp, pred->mask)) { >> + parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i); >> + goto err_free; >> + } >> + >> + /* Move along */ >> + i++; >> + if (field->filter_type == FILTER_CPUMASK) >> + pred->fn_num = FILTER_PRED_FN_CPUMASK; >> + > > -- > Josh
On Wed, 26 Jul 2023 12:41:48 -0700 Josh Poimboeuf <jpoimboe@kernel.org> wrote: > On Thu, Jul 20, 2023 at 05:30:38PM +0100, Valentin Schneider wrote: > > int filter_assign_type(const char *type) > > { > > - if (strstr(type, "__data_loc") && strstr(type, "char")) > > - return FILTER_DYN_STRING; > > + if (strstr(type, "__data_loc")) { > > + if (strstr(type, "char")) > > + return FILTER_DYN_STRING; > > + if (strstr(type, "cpumask_t")) > > + return FILTER_CPUMASK; > > + } > > The closing bracket has the wrong indentation. > > > + /* Copy the cpulist between { and } */ > > + tmp = kmalloc((i - maskstart) + 1, GFP_KERNEL); > > + strscpy(tmp, str + maskstart, (i - maskstart) + 1); > > Need to check kmalloc() failure? And also free tmp? I came to state the same thing. Also, when you do an empty for loop: for (; str[i] && str[i] != '}'; i++); Always put the semicolon on the next line, otherwise it is really easy to think that the next line is part of the for loop. That is, instead of the above, do: for (; str[i] && str[i] != '}'; i++) ; -- Steve > > > + > > + pred->mask = kzalloc(cpumask_size(), GFP_KERNEL); > > + if (!pred->mask) > > + goto err_mem; > > + > > + /* Now parse it */ > > + if (cpulist_parse(tmp, pred->mask)) { > > + parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i); > > + goto err_free; > > + } > > + > > + /* Move along */ > > + i++; > > + if (field->filter_type == FILTER_CPUMASK) > > + pred->fn_num = FILTER_PRED_FN_CPUMASK; > > + >
On 29/07/23 15:09, Steven Rostedt wrote: > On Wed, 26 Jul 2023 12:41:48 -0700 > Josh Poimboeuf <jpoimboe@kernel.org> wrote: > >> On Thu, Jul 20, 2023 at 05:30:38PM +0100, Valentin Schneider wrote: >> > int filter_assign_type(const char *type) >> > { >> > - if (strstr(type, "__data_loc") && strstr(type, "char")) >> > - return FILTER_DYN_STRING; >> > + if (strstr(type, "__data_loc")) { >> > + if (strstr(type, "char")) >> > + return FILTER_DYN_STRING; >> > + if (strstr(type, "cpumask_t")) >> > + return FILTER_CPUMASK; >> > + } >> >> The closing bracket has the wrong indentation. >> >> > + /* Copy the cpulist between { and } */ >> > + tmp = kmalloc((i - maskstart) + 1, GFP_KERNEL); >> > + strscpy(tmp, str + maskstart, (i - maskstart) + 1); >> >> Need to check kmalloc() failure? And also free tmp? > > I came to state the same thing. > > Also, when you do an empty for loop: > > for (; str[i] && str[i] != '}'; i++); > > Always put the semicolon on the next line, otherwise it is really easy > to think that the next line is part of the for loop. That is, instead > of the above, do: > > for (; str[i] && str[i] != '}'; i++) > ; > Interestingly I don't think I've ever encountered that variant, usually having an empty line (which this lacks) and the indentation level is enough to identify these - regardless, I'll change it. > > -- Steve > > >> >> > + >> > + pred->mask = kzalloc(cpumask_size(), GFP_KERNEL); >> > + if (!pred->mask) >> > + goto err_mem; >> > + >> > + /* Now parse it */ >> > + if (cpulist_parse(tmp, pred->mask)) { >> > + parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i); >> > + goto err_free; >> > + } >> > + >> > + /* Move along */ >> > + i++; >> > + if (field->filter_type == FILTER_CPUMASK) >> > + pred->fn_num = FILTER_PRED_FN_CPUMASK; >> > + >>
On Mon, 31 Jul 2023 12:19:51 +0100 Valentin Schneider <vschneid@redhat.com> wrote: > > > > Also, when you do an empty for loop: > > > > for (; str[i] && str[i] != '}'; i++); > > > > Always put the semicolon on the next line, otherwise it is really easy > > to think that the next line is part of the for loop. That is, instead > > of the above, do: > > > > for (; str[i] && str[i] != '}'; i++) > > ; > > > > Interestingly I don't think I've ever encountered that variant, usually > having an empty line (which this lacks) and the indentation level is enough > to identify these - regardless, I'll change it. Do a "git grep -B1 -e '^\s*;\s*$'" You'll find that it is quite common. Thanks, -- Steve
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index 3930e676436c9..302be73918336 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -807,6 +807,7 @@ enum { FILTER_RDYN_STRING, FILTER_PTR_STRING, FILTER_TRACE_FN, + FILTER_CPUMASK, FILTER_COMM, FILTER_CPU, FILTER_STACKTRACE, diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c index 91fc9990107f1..cb1863dfa280b 100644 --- a/kernel/trace/trace_events_filter.c +++ b/kernel/trace/trace_events_filter.c @@ -64,6 +64,7 @@ enum filter_pred_fn { FILTER_PRED_FN_PCHAR_USER, FILTER_PRED_FN_PCHAR, FILTER_PRED_FN_CPU, + FILTER_PRED_FN_CPUMASK, FILTER_PRED_FN_FUNCTION, FILTER_PRED_FN_, FILTER_PRED_TEST_VISITED, @@ -71,6 +72,7 @@ enum filter_pred_fn { struct filter_pred { struct regex *regex; + struct cpumask *mask; unsigned short *ops; struct ftrace_event_field *field; u64 val; @@ -94,6 +96,8 @@ struct filter_pred { C(TOO_MANY_OPEN, "Too many '('"), \ C(TOO_MANY_CLOSE, "Too few '('"), \ C(MISSING_QUOTE, "Missing matching quote"), \ + C(MISSING_BRACE_OPEN, "Missing '{'"), \ + C(MISSING_BRACE_CLOSE, "Missing '}'"), \ C(OPERAND_TOO_LONG, "Operand too long"), \ C(EXPECT_STRING, "Expecting string field"), \ C(EXPECT_DIGIT, "Expecting numeric field"), \ @@ -103,6 +107,7 @@ struct filter_pred { C(BAD_SUBSYS_FILTER, "Couldn't find or set field in one of a subsystem's events"), \ C(TOO_MANY_PREDS, "Too many terms in predicate expression"), \ C(INVALID_FILTER, "Meaningless filter expression"), \ + C(INVALID_CPULIST, "Invalid cpulist"), \ C(IP_FIELD_ONLY, "Only 'ip' field is supported for function trace"), \ C(INVALID_VALUE, "Invalid value (did you forget quotes)?"), \ C(NO_FUNCTION, "Function not found"), \ @@ -190,6 +195,7 @@ static void free_predicate(struct filter_pred *pred) { if (pred) { kfree(pred->regex); + kfree(pred->mask); kfree(pred); } } @@ -877,6 +883,26 @@ static int filter_pred_cpu(struct filter_pred *pred, void *event) } } +/* Filter predicate for cpumask field vs user-provided cpumask */ +static int filter_pred_cpumask(struct filter_pred *pred, void *event) +{ + u32 item = *(u32 *)(event + pred->offset); + int loc = item & 0xffff; + const struct cpumask *mask = (event + loc); + const struct cpumask *cmp = pred->mask; + + switch (pred->op) { + case OP_EQ: + return cpumask_equal(mask, cmp); + case OP_NE: + return !cpumask_equal(mask, cmp); + case OP_BAND: + return cpumask_intersects(mask, cmp); + default: + return 0; + } +} + /* Filter predicate for COMM. */ static int filter_pred_comm(struct filter_pred *pred, void *event) { @@ -1244,8 +1270,12 @@ static void filter_free_subsystem_filters(struct trace_subsystem_dir *dir, int filter_assign_type(const char *type) { - if (strstr(type, "__data_loc") && strstr(type, "char")) - return FILTER_DYN_STRING; + if (strstr(type, "__data_loc")) { + if (strstr(type, "char")) + return FILTER_DYN_STRING; + if (strstr(type, "cpumask_t")) + return FILTER_CPUMASK; + } if (strstr(type, "__rel_loc") && strstr(type, "char")) return FILTER_RDYN_STRING; @@ -1357,6 +1387,8 @@ static int filter_pred_fn_call(struct filter_pred *pred, void *event) return filter_pred_pchar(pred, event); case FILTER_PRED_FN_CPU: return filter_pred_cpu(pred, event); + case FILTER_PRED_FN_CPUMASK: + return filter_pred_cpumask(pred, event); case FILTER_PRED_FN_FUNCTION: return filter_pred_function(pred, event); case FILTER_PRED_TEST_VISITED: @@ -1568,6 +1600,67 @@ static int parse_pred(const char *str, void *data, strncpy(pred->regex->pattern, str + s, len); pred->regex->pattern[len] = 0; + } else if (!strncmp(str + i, "CPUS", 4)) { + unsigned int maskstart; + char *tmp; + + switch (field->filter_type) { + case FILTER_CPUMASK: + break; + default: + parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i); + goto err_free; + } + + switch (op) { + case OP_EQ: + case OP_NE: + case OP_BAND: + break; + default: + parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i); + goto err_free; + } + + /* Skip CPUS */ + i += 4; + if (str[i++] != '{') { + parse_error(pe, FILT_ERR_MISSING_BRACE_OPEN, pos + i); + goto err_free; + } + maskstart = i; + + /* Walk the cpulist until closing } */ + for (; str[i] && str[i] != '}'; i++); + if (str[i] != '}') { + parse_error(pe, FILT_ERR_MISSING_BRACE_CLOSE, pos + i); + goto err_free; + } + + if (maskstart == i) { + parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i); + goto err_free; + } + + /* Copy the cpulist between { and } */ + tmp = kmalloc((i - maskstart) + 1, GFP_KERNEL); + strscpy(tmp, str + maskstart, (i - maskstart) + 1); + + pred->mask = kzalloc(cpumask_size(), GFP_KERNEL); + if (!pred->mask) + goto err_mem; + + /* Now parse it */ + if (cpulist_parse(tmp, pred->mask)) { + parse_error(pe, FILT_ERR_INVALID_CPULIST, pos + i); + goto err_free; + } + + /* Move along */ + i++; + if (field->filter_type == FILTER_CPUMASK) + pred->fn_num = FILTER_PRED_FN_CPUMASK; + /* This is either a string, or an integer */ } else if (str[i] == '\'' || str[i] == '"') { char q = str[i];
The recently introduced ipi_send_cpumask trace event contains a cpumask field, but it currently cannot be used in filter expressions. Make event filtering aware of cpumask fields, and allow these to be filtered by a user-provided cpumask. The user-provided cpumask is to be given in cpulist format and wrapped as: "CPUS{$cpulist}". The use of curly braces instead of parentheses is to prevent predicate_parse() from parsing the contents of CPUS{...} as a full-fledged predicate subexpression. This enables e.g.: $ trace-cmd record -e 'ipi_send_cpumask' -f 'cpumask & CPUS{2,4,6,8-32}' Signed-off-by: Valentin Schneider <vschneid@redhat.com> --- include/linux/trace_events.h | 1 + kernel/trace/trace_events_filter.c | 97 +++++++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 2 deletions(-)