[PATCH 0/2] tracing/hist: add a modulus operator

[PATCH 0/2] tracing/hist: add a modulus operator

[Mark Rutland]

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

[PATCH 1/2] tracing/hist: simplify contains_operator()

[Mark Rutland]

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

[PATCH 2/2] tracing/hist: add modulus operator

[Mark Rutland]

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

Re: [PATCH 2/2] tracing/hist: add modulus operator

[kernel test robot]

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

Re: [PATCH 2/2] tracing/hist: add modulus operator

[kernel test robot]

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

Re: [PATCH 1/2] tracing/hist: simplify contains_operator()

[Steven Rostedt]

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;
>  	}
>  

Re: [PATCH 2/2] tracing/hist: add modulus operator

[Steven Rostedt]

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


> +}
> +

Re: [PATCH 1/2] tracing/hist: simplify contains_operator()

[Mark Rutland]

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.

Re: [PATCH 2/2] tracing/hist: add modulus operator

[Mark Rutland]

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.