diff mbox series

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

Message ID 20230302171755.1821653-3-mark.rutland@arm.com
State New
Headers
Series tracing/hist: add a modulus operator |

Commit Message

Mark Rutland March 2, 2023, 5:17 p.m. UTC 
  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(-)

Comments

Steven Rostedt March 18, 2023, 7:19 p.m. UTC | #1 
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


> +}
> +
Mark Rutland March 22, 2023, 1:40 p.m. UTC | #2 
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.
diff mbox series

Patch

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: