* [PATCH 0/2] tracing/hist: add a modulus operator @ 2023-03-02 17:17 Mark Rutland 2023-03-02 17:17 ` [PATCH 1/2] tracing/hist: simplify contains_operator() Mark Rutland 2023-03-02 17:17 ` [PATCH 2/2] tracing/hist: add modulus operator Mark Rutland 0 siblings, 2 replies; 9+ messages in thread From: Mark Rutland @ 2023-03-02 17:17 UTC (permalink / raw) To: linux-kernel; +Cc: linux-trace-kernel, mark.rutland, mhiramat, rostedt, zanussi I've been analysing some usercopy code, and wanted to cpature the spread of sizes and alignment of copies for a workload. I found that histrogram triggers were great for cpaturing the size, but there's not currently a way to capture the alignment short of recording the entire base pointer and post-processing the histogram, which is less than ideal. These patches add a modulus operator to histogram expressions, so which allows the alignment of pointers to be captured directly. The first patch is a preparatory refactoring to the expression parsing code, and the second actually adds the new operator. Thanks, Mark. Mark Rutland (2): tracing/hist: simplify contains_operator() tracing/hist: add modulus operator Documentation/trace/histogram.rst | 4 +- kernel/trace/trace_events_hist.c | 109 ++++++++++++++++-------------- 2 files changed, 62 insertions(+), 51 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] tracing/hist: simplify contains_operator() 2023-03-02 17:17 [PATCH 0/2] tracing/hist: add a modulus operator Mark Rutland @ 2023-03-02 17:17 ` Mark Rutland 2023-03-18 19:12 ` Steven Rostedt 2023-03-02 17:17 ` [PATCH 2/2] tracing/hist: add modulus operator Mark Rutland 1 sibling, 1 reply; 9+ messages in thread From: Mark Rutland @ 2023-03-02 17:17 UTC (permalink / raw) To: linux-kernel; +Cc: linux-trace-kernel, mark.rutland, mhiramat, rostedt, zanussi In a subsequent patch we'll add additional operators for histogram expressions. In preparation for adding additional operators, this patch refactors contains_operator() to consider each operator within a precedence group independently by using the 'sep' pointer as the current rightmost operator, and removing the separate op pointers. Within each precedence group, this allows operators to be checked independently with a consistent pattern: op = strrchr(str, $OP_CHAR); if (op > *sep) { *sep = op; field_op = $FIELD_OP_TYPE; } This makes it easy to add new operators of the same precedence without needing to check multiple pointers, and without needing a final switch statement to recover the relevant pointer. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tom Zanussi <zanussi@kernel.org> Cc: linux-trace-kernel@vger.kernel.org --- kernel/trace/trace_events_hist.c | 80 ++++++++++++-------------------- 1 file changed, 30 insertions(+), 50 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 10d36f751fcd..a308da2cde2f 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1813,13 +1813,15 @@ static char *expr_str(struct hist_field *field, unsigned int level) static int contains_operator(char *str, char **sep) { enum field_op_id field_op = FIELD_OP_NONE; - char *minus_op, *plus_op, *div_op, *mult_op; + char *op; + *sep = NULL; /* - * Report the last occurrence of the operators first, so that the - * expression is evaluated left to right. This is important since - * subtraction and division are not associative. + * For operators of the same precedence report the last occurrence of + * the operators first, so that the expression is evaluated left to + * right. This is important since subtraction and division are not + * associative. * * e.g * 64/8/4/2 is 1, i.e 64/8/4/2 = ((64/8)/4)/2 @@ -1830,68 +1832,46 @@ static int contains_operator(char *str, char **sep) * First, find lower precedence addition and subtraction * since the expression will be evaluated recursively. */ - minus_op = strrchr(str, '-'); - if (minus_op) { + op = strrchr(str, '-'); + if (op > *sep) { + *sep = op; + /* * Unary minus is not supported in sub-expressions. If * present, it is always the next root operator. */ - if (minus_op == str) { - field_op = FIELD_OP_UNARY_MINUS; - goto out; - } + if (op == str) + return FIELD_OP_UNARY_MINUS; field_op = FIELD_OP_MINUS; } - plus_op = strrchr(str, '+'); - if (plus_op || minus_op) { - /* - * For operators of the same precedence use to rightmost as the - * root, so that the expression is evaluated left to right. - */ - if (plus_op > minus_op) - field_op = FIELD_OP_PLUS; - goto out; + op = strrchr(str, '+'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_PLUS; } /* - * Multiplication and division have higher precedence than addition and - * subtraction. + * If we've found a low-precedence operator, we're done. */ - div_op = strrchr(str, '/'); - if (div_op) - field_op = FIELD_OP_DIV; + if (*sep) + return field_op; - mult_op = strrchr(str, '*'); /* - * For operators of the same precedence use to rightmost as the - * root, so that the expression is evaluated left to right. + * Second, consider the higher precedence multiplication and division + * operators. */ - if (mult_op > div_op) - field_op = FIELD_OP_MULT; + op = strrchr(str, '/'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_DIV; + } -out: - if (sep) { - switch (field_op) { - case FIELD_OP_UNARY_MINUS: - case FIELD_OP_MINUS: - *sep = minus_op; - break; - case FIELD_OP_PLUS: - *sep = plus_op; - break; - case FIELD_OP_DIV: - *sep = div_op; - break; - case FIELD_OP_MULT: - *sep = mult_op; - break; - case FIELD_OP_NONE: - default: - *sep = NULL; - break; - } + op = strrchr(str, '*'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_MULT; } return field_op; -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing/hist: simplify contains_operator() 2023-03-02 17:17 ` [PATCH 1/2] tracing/hist: simplify contains_operator() Mark Rutland @ 2023-03-18 19:12 ` Steven Rostedt 2023-03-22 13:13 ` Mark Rutland 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2023-03-18 19:12 UTC (permalink / raw) To: Mark Rutland; +Cc: linux-kernel, linux-trace-kernel, mhiramat, zanussi On Thu, 2 Mar 2023 17:17:54 +0000 Mark Rutland <mark.rutland@arm.com> wrote: FYI, we follow Linus's preference that subjects start with a capital letter. Unless of course you are a socialist and dislike capitalism? tracing/hist: Simplify contains_operator() > In a subsequent patch we'll add additional operators for histogram > expressions. Refrain from using "subsequent patch", instead say: Simplify contains_operator() in order to support additional operators for histogram expressions. > > In preparation for adding additional operators, this patch refactors > contains_operator() to consider each operator within a precedence group > independently by using the 'sep' pointer as the current rightmost > operator, and removing the separate op pointers. > > Within each precedence group, this allows operators to be checked > independently with a consistent pattern: > > op = strrchr(str, $OP_CHAR); > if (op > *sep) { > *sep = op; > field_op = $FIELD_OP_TYPE; > } > > This makes it easy to add new operators of the same precedence without > needing to check multiple pointers, and without needing a final switch > statement to recover the relevant pointer. > > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Steven Rostedt (Google) <rostedt@goodmis.org> > Cc: Tom Zanussi <zanussi@kernel.org> > Cc: linux-trace-kernel@vger.kernel.org > --- > kernel/trace/trace_events_hist.c | 80 ++++++++++++-------------------- > 1 file changed, 30 insertions(+), 50 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 10d36f751fcd..a308da2cde2f 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1813,13 +1813,15 @@ static char *expr_str(struct hist_field *field, unsigned int level) > static int contains_operator(char *str, char **sep) > { > enum field_op_id field_op = FIELD_OP_NONE; > - char *minus_op, *plus_op, *div_op, *mult_op; > + char *op; > > + *sep = NULL; Hmm! > > /* > - * Report the last occurrence of the operators first, so that the > - * expression is evaluated left to right. This is important since > - * subtraction and division are not associative. > + * For operators of the same precedence report the last occurrence of > + * the operators first, so that the expression is evaluated left to > + * right. This is important since subtraction and division are not > + * associative. > * > * e.g > * 64/8/4/2 is 1, i.e 64/8/4/2 = ((64/8)/4)/2 > @@ -1830,68 +1832,46 @@ static int contains_operator(char *str, char **sep) > * First, find lower precedence addition and subtraction > * since the expression will be evaluated recursively. > */ > - minus_op = strrchr(str, '-'); > - if (minus_op) { > + op = strrchr(str, '-'); > + if (op > *sep) { Why compare to *sep if it is always NULL? Oh! But later in the code we have: if (contains_operator(field, NULL) || is_var_ref(field)) I wonder how *sep = NULL will handle that? -- Steve > + *sep = op; > + > /* > * Unary minus is not supported in sub-expressions. If > * present, it is always the next root operator. > */ > - if (minus_op == str) { > - field_op = FIELD_OP_UNARY_MINUS; > - goto out; > - } > + if (op == str) > + return FIELD_OP_UNARY_MINUS; > > field_op = FIELD_OP_MINUS; > } > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing/hist: simplify contains_operator() 2023-03-18 19:12 ` Steven Rostedt @ 2023-03-22 13:13 ` Mark Rutland 0 siblings, 0 replies; 9+ messages in thread From: Mark Rutland @ 2023-03-22 13:13 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel, linux-trace-kernel, mhiramat, zanussi On Sat, Mar 18, 2023 at 03:12:08PM -0400, Steven Rostedt wrote: > On Thu, 2 Mar 2023 17:17:54 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > FYI, we follow Linus's preference that subjects start with a capital > letter. Unless of course you are a socialist and dislike capitalism? > > tracing/hist: Simplify contains_operator() > Sorry; I always get this wrong since many other trees do everything lower case (or support total commit message anarchy). I'll go fix that up. > > > In a subsequent patch we'll add additional operators for histogram > > expressions. > > Refrain from using "subsequent patch", instead say: > > Simplify contains_operator() in order to support additional operators > for histogram expressions. Sure. > > > > > In preparation for adding additional operators, this patch refactors > > contains_operator() to consider each operator within a precedence group > > independently by using the 'sep' pointer as the current rightmost > > operator, and removing the separate op pointers. > > > > Within each precedence group, this allows operators to be checked > > independently with a consistent pattern: > > > > op = strrchr(str, $OP_CHAR); > > if (op > *sep) { > > *sep = op; > > field_op = $FIELD_OP_TYPE; > > } > > > > This makes it easy to add new operators of the same precedence without > > needing to check multiple pointers, and without needing a final switch > > statement to recover the relevant pointer. > > > > There should be no functional change as a result of this patch. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Steven Rostedt (Google) <rostedt@goodmis.org> > > Cc: Tom Zanussi <zanussi@kernel.org> > > Cc: linux-trace-kernel@vger.kernel.org > > --- > > kernel/trace/trace_events_hist.c | 80 ++++++++++++-------------------- > > 1 file changed, 30 insertions(+), 50 deletions(-) > > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > index 10d36f751fcd..a308da2cde2f 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -1813,13 +1813,15 @@ static char *expr_str(struct hist_field *field, unsigned int level) > > static int contains_operator(char *str, char **sep) > > { > > enum field_op_id field_op = FIELD_OP_NONE; > > - char *minus_op, *plus_op, *div_op, *mult_op; > > + char *op; > > > > + *sep = NULL; > > Hmm! Ugh, sorry, I had completely glossed over the: if (sep) { ... // assignments to *sep here ... } ... in the existing code. I'll go rework that... > > > > > /* > > - * Report the last occurrence of the operators first, so that the > > - * expression is evaluated left to right. This is important since > > - * subtraction and division are not associative. > > + * For operators of the same precedence report the last occurrence of > > + * the operators first, so that the expression is evaluated left to > > + * right. This is important since subtraction and division are not > > + * associative. > > * > > * e.g > > * 64/8/4/2 is 1, i.e 64/8/4/2 = ((64/8)/4)/2 > > @@ -1830,68 +1832,46 @@ static int contains_operator(char *str, char **sep) > > * First, find lower precedence addition and subtraction > > * since the expression will be evaluated recursively. > > */ > > - minus_op = strrchr(str, '-'); > > - if (minus_op) { > > + op = strrchr(str, '-'); > > + if (op > *sep) { > > Why compare to *sep if it is always NULL? As in the commit message, that was just so that every check for an operator had the same shape. I can certainly drop this for the first check and just have: op = strrchr(str, '-'); if (op) { ... } > > Oh! But later in the code we have: > > if (contains_operator(field, NULL) || is_var_ref(field)) > > I wonder how *sep = NULL will handle that? Yep, I got this wrong. I'll go rejig that. Thanks, Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] tracing/hist: add modulus operator 2023-03-02 17:17 [PATCH 0/2] tracing/hist: add a modulus operator Mark Rutland 2023-03-02 17:17 ` [PATCH 1/2] tracing/hist: simplify contains_operator() Mark Rutland @ 2023-03-02 17:17 ` Mark Rutland 2023-03-02 21:41 ` kernel test robot ` (2 more replies) 1 sibling, 3 replies; 9+ messages in thread From: Mark Rutland @ 2023-03-02 17:17 UTC (permalink / raw) To: linux-kernel; +Cc: linux-trace-kernel, mark.rutland, mhiramat, rostedt, zanussi Currently historgram field expressions can use addition ('+'), substraction ('-'), division ('/'), and multiplication ('*') operators. It would be helpful to also have a modulus ('%') operator. This is helpful for capturing the alignment of pointers. For example, on arm64 with CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, we can record the size and alignment of copies to user with: | # echo 'p:copy_to_user __arch_copy_to_user to=$arg1 from=$arg2 n=$arg3' >> /sys/kernel/tracing/kprobe_events | # echo 'hist keys=n,to%8:vals=hitcount:sort=n,to%8' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger | # cat /sys/kernel/tracing/events/kprobes/copy_to_user/hist | # event histogram | # | # trigger info: hist:keys=n,to%8:vals=hitcount:sort=n,to%8:size=2048 [active] | # | | { n: 1, to%8: 1 } hitcount: 5 | { n: 8, to%8: 0 } hitcount: 3 | { n: 16, to%8: 0 } hitcount: 2 | { n: 32, to%8: 0 } hitcount: 1 | { n: 36, to%8: 0 } hitcount: 1 | { n: 128, to%8: 0 } hitcount: 4 | { n: 336, to%8: 0 } hitcount: 1 | { n: 832, to%8: 0 } hitcount: 3 | | Totals: | Hits: 20 | Entries: 8 | Dropped: 0 Add a modulus operator, with the same precedence as multiplication and division, matching C's operator precedence. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tom Zanussi <zanussi@kernel.org> Cc: linux-trace-kernel@vger.kernel.org --- Documentation/trace/histogram.rst | 4 ++-- kernel/trace/trace_events_hist.c | 35 +++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/Documentation/trace/histogram.rst b/Documentation/trace/histogram.rst index f95459aa984f..534fb190ebe0 100644 --- a/Documentation/trace/histogram.rst +++ b/Documentation/trace/histogram.rst @@ -1771,8 +1771,8 @@ using the same key and variable from yet another event:: # echo 'hist:key=pid:wakeupswitch_lat=$wakeup_lat+$switchtime_lat ...' >> event3/trigger -Expressions support the use of addition, subtraction, multiplication and -division operators (+-\*/). +Expressions support the use of addition, subtraction, multiplication, +division, modulus operators (+-\*/%). Note if division by zero cannot be detected at parse time (i.e. the divisor is not a constant), the result will be -1. diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index a308da2cde2f..629896aaed54 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -103,6 +103,7 @@ enum field_op_id { FIELD_OP_UNARY_MINUS, FIELD_OP_DIV, FIELD_OP_MULT, + FIELD_OP_MOD, }; enum hist_field_fn { @@ -131,6 +132,7 @@ enum hist_field_fn { HIST_FIELD_FN_PLUS, HIST_FIELD_FN_DIV, HIST_FIELD_FN_MULT, + HIST_FIELD_FN_MOD, HIST_FIELD_FN_DIV_POWER2, HIST_FIELD_FN_DIV_NOT_POWER2, HIST_FIELD_FN_DIV_MULT_SHIFT, @@ -436,6 +438,21 @@ static u64 hist_field_mult(struct hist_field *hist_field, return val1 * val2; } +static u64 hist_field_mod(struct hist_field *hist_field, + struct tracing_map_elt *elt, + struct trace_buffer *buffer, + struct ring_buffer_event *rbe, + void *event) +{ + struct hist_field *operand1 = hist_field->operands[0]; + struct hist_field *operand2 = hist_field->operands[1]; + + u64 val1 = hist_fn_call(operand1, elt, buffer, rbe, event); + u64 val2 = hist_fn_call(operand2, elt, buffer, rbe, event); + + return val1 % val2; +} + static u64 hist_field_unary_minus(struct hist_field *hist_field, struct tracing_map_elt *elt, struct trace_buffer *buffer, @@ -1796,6 +1813,9 @@ static char *expr_str(struct hist_field *field, unsigned int level) case FIELD_OP_MULT: strcat(expr, "*"); break; + case FIELD_OP_MOD: + strcat(expr, "%"); + break; default: kfree(expr); return NULL; @@ -1859,8 +1879,8 @@ static int contains_operator(char *str, char **sep) return field_op; /* - * Second, consider the higher precedence multiplication and division - * operators. + * Second, consider the higher precedence multiplication, division, and + * modulus operators. */ op = strrchr(str, '/'); if (op > *sep) { @@ -1874,6 +1894,12 @@ static int contains_operator(char *str, char **sep) field_op = FIELD_OP_MULT; } + op = strrchr(str, '%'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_MOD; + } + return field_op; } @@ -2698,6 +2724,9 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data, case FIELD_OP_MULT: op_fn = HIST_FIELD_FN_MULT; break; + case FIELD_OP_MOD: + op_fn = HIST_FIELD_FN_MOD; + break; default: ret = -EINVAL; goto free_operands; @@ -4289,6 +4318,8 @@ static u64 hist_fn_call(struct hist_field *hist_field, return hist_field_div(hist_field, elt, buffer, rbe, event); case HIST_FIELD_FN_MULT: return hist_field_mult(hist_field, elt, buffer, rbe, event); + case HIST_FIELD_FN_MOD: + return hist_field_mod(hist_field, elt, buffer, rbe, event); case HIST_FIELD_FN_DIV_POWER2: return div_by_power_of_two(hist_field, elt, buffer, rbe, event); case HIST_FIELD_FN_DIV_NOT_POWER2: -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/hist: add modulus operator 2023-03-02 17:17 ` [PATCH 2/2] tracing/hist: add modulus operator Mark Rutland @ 2023-03-02 21:41 ` kernel test robot 2023-03-03 12:56 ` kernel test robot 2023-03-18 19:19 ` Steven Rostedt 2 siblings, 0 replies; 9+ messages in thread From: kernel test robot @ 2023-03-02 21:41 UTC (permalink / raw) To: Mark Rutland; +Cc: oe-kbuild-all Hi Mark, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on rostedt-trace/for-next v6.2 next-20230302] [cannot apply to rostedt-trace/for-next-urgent] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mark-Rutland/tracing-hist-simplify-contains_operator/20230303-012033 patch link: https://lore.kernel.org/r/20230302171755.1821653-3-mark.rutland%40arm.com patch subject: [PATCH 2/2] tracing/hist: add modulus operator config: i386-randconfig-a016 (https://download.01.org/0day-ci/archive/20230303/202303030527.tCXUA4Xo-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/a2602e97de77834320fed5f928662f154f72d7ce git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Mark-Rutland/tracing-hist-simplify-contains_operator/20230303-012033 git checkout a2602e97de77834320fed5f928662f154f72d7ce # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=i386 olddefconfig make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303030527.tCXUA4Xo-lkp@intel.com/ All errors (new ones prefixed by >>): ld: kernel/trace/trace_events_hist.o: in function `hist_field_mod': >> kernel/trace/trace_events_hist.c:454: undefined reference to `__umoddi3' vim +454 kernel/trace/trace_events_hist.c 441 442 static u64 hist_field_mod(struct hist_field *hist_field, 443 struct tracing_map_elt *elt, 444 struct trace_buffer *buffer, 445 struct ring_buffer_event *rbe, 446 void *event) 447 { 448 struct hist_field *operand1 = hist_field->operands[0]; 449 struct hist_field *operand2 = hist_field->operands[1]; 450 451 u64 val1 = hist_fn_call(operand1, elt, buffer, rbe, event); 452 u64 val2 = hist_fn_call(operand2, elt, buffer, rbe, event); 453 > 454 return val1 % val2; 455 } 456 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/hist: add modulus operator 2023-03-02 17:17 ` [PATCH 2/2] tracing/hist: add modulus operator Mark Rutland 2023-03-02 21:41 ` kernel test robot @ 2023-03-03 12:56 ` kernel test robot 2023-03-18 19:19 ` Steven Rostedt 2 siblings, 0 replies; 9+ messages in thread From: kernel test robot @ 2023-03-03 12:56 UTC (permalink / raw) To: Mark Rutland; +Cc: oe-kbuild-all Hi Mark, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on rostedt-trace/for-next v6.2 next-20230303] [cannot apply to rostedt-trace/for-next-urgent] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mark-Rutland/tracing-hist-simplify-contains_operator/20230303-012033 patch link: https://lore.kernel.org/r/20230302171755.1821653-3-mark.rutland%40arm.com patch subject: [PATCH 2/2] tracing/hist: add modulus operator config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230303/202303032025.sa7xWAu3-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/a2602e97de77834320fed5f928662f154f72d7ce git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Mark-Rutland/tracing-hist-simplify-contains_operator/20230303-012033 git checkout a2602e97de77834320fed5f928662f154f72d7ce # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=i386 olddefconfig make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303032025.sa7xWAu3-lkp@intel.com/ All errors (new ones prefixed by >>): ld: kernel/trace/trace_events_hist.o: in function `hist_fn_call': >> trace_events_hist.c:(.text+0xa007): undefined reference to `__umoddi3' -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/hist: add modulus operator 2023-03-02 17:17 ` [PATCH 2/2] tracing/hist: add modulus operator Mark Rutland 2023-03-02 21:41 ` kernel test robot 2023-03-03 12:56 ` kernel test robot @ 2023-03-18 19:19 ` Steven Rostedt 2023-03-22 13:40 ` Mark Rutland 2 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2023-03-18 19:19 UTC (permalink / raw) To: Mark Rutland; +Cc: linux-kernel, linux-trace-kernel, mhiramat, zanussi On Thu, 2 Mar 2023 17:17:55 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > @@ -436,6 +438,21 @@ static u64 hist_field_mult(struct hist_field *hist_field, > return val1 * val2; > } > > +static u64 hist_field_mod(struct hist_field *hist_field, > + struct tracing_map_elt *elt, > + struct trace_buffer *buffer, > + struct ring_buffer_event *rbe, > + void *event) > +{ > + struct hist_field *operand1 = hist_field->operands[0]; > + struct hist_field *operand2 = hist_field->operands[1]; > + > + u64 val1 = hist_fn_call(operand1, elt, buffer, rbe, event); > + u64 val2 = hist_fn_call(operand2, elt, buffer, rbe, event); > + > + return val1 % val2; Is modulus operations on 64 bit integers valid on 32 bit architectures? Don't we need to do something like: div64_u64_rem(val1, val2, &rem); return rem; ? -- Steve > +} > + ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/hist: add modulus operator 2023-03-18 19:19 ` Steven Rostedt @ 2023-03-22 13:40 ` Mark Rutland 0 siblings, 0 replies; 9+ messages in thread From: Mark Rutland @ 2023-03-22 13:40 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel, linux-trace-kernel, mhiramat, zanussi On Sat, Mar 18, 2023 at 03:19:02PM -0400, Steven Rostedt wrote: > On Thu, 2 Mar 2023 17:17:55 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > @@ -436,6 +438,21 @@ static u64 hist_field_mult(struct hist_field *hist_field, > > return val1 * val2; > > } > > > > +static u64 hist_field_mod(struct hist_field *hist_field, > > + struct tracing_map_elt *elt, > > + struct trace_buffer *buffer, > > + struct ring_buffer_event *rbe, > > + void *event) > > +{ > > + struct hist_field *operand1 = hist_field->operands[0]; > > + struct hist_field *operand2 = hist_field->operands[1]; > > + > > + u64 val1 = hist_fn_call(operand1, elt, buffer, rbe, event); > > + u64 val2 = hist_fn_call(operand2, elt, buffer, rbe, event); > > + > > + return val1 % val2; > > Is modulus operations on 64 bit integers valid on 32 bit architectures? > > Don't we need to do something like: > > div64_u64_rem(val1, val2, &rem); > return rem; > > ? Yes, we do; the build bots have also started shouting at me for this. I'll go fold that in... Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-03-22 13:40 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-02 17:17 [PATCH 0/2] tracing/hist: add a modulus operator Mark Rutland 2023-03-02 17:17 ` [PATCH 1/2] tracing/hist: simplify contains_operator() Mark Rutland 2023-03-18 19:12 ` Steven Rostedt 2023-03-22 13:13 ` Mark Rutland 2023-03-02 17:17 ` [PATCH 2/2] tracing/hist: add modulus operator Mark Rutland 2023-03-02 21:41 ` kernel test robot 2023-03-03 12:56 ` kernel test robot 2023-03-18 19:19 ` Steven Rostedt 2023-03-22 13:40 ` Mark Rutland
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.