diff mbox series

[1/5] tracing: Add support for creating hist trigger variables from literal

Message ID 20210915195306.612966-2-kaleshsingh@google.com (mailing list archive)
State New
Headers show
Series tracing: Extend histogram triggers expression parsing | expand

Commit Message

Kalesh Singh Sept. 15, 2021, 7:52 p.m. UTC
Currently hist trigger expressions don't support the use of numeric
literals:
	e.g. echo 'hist:keys=common_pid:x=$y-1234'
		--> is not valid expression syntax

Having the ability to use numeric constants in hist triggers supports
a wider range of expressions for creating variables.

Add support for creating trace event histogram variables from numeric
literals.

	e.g. echo 'hist:keys=common_pid:x=1234,y=size-1024' >> event/trigger

A negative numeric constant is created, using unary minus operator
(parentheses are required).

	e.g. echo 'hist:keys=common_pid:z=-(2)' >> event/trigger

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
 kernel/trace/trace_events_hist.c | 80 +++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

Comments

Steven Rostedt Oct. 19, 2021, 8:48 p.m. UTC | #1
On Wed, 15 Sep 2021 19:52:45 +0000
Kalesh Singh <kaleshsingh@google.com> wrote:

> Currently hist trigger expressions don't support the use of numeric
> literals:
> 	e.g. echo 'hist:keys=common_pid:x=$y-1234'
> 		--> is not valid expression syntax  
> 
> Having the ability to use numeric constants in hist triggers supports
> a wider range of expressions for creating variables.

I'm not against the patch, but I'm curious to what use case this would be
useful for. In the cover letter it mentions the division and multiplication
for finding associated buckets, but what is the addition / subtraction used
for?

-- Steve


> 
> Add support for creating trace event histogram variables from numeric
> literals.
> 
> 	e.g. echo 'hist:keys=common_pid:x=1234,y=size-1024' >> event/trigger
> 
> A negative numeric constant is created, using unary minus operator
> (parentheses are required).
> 
> 	e.g. echo 'hist:keys=common_pid:z=-(2)' >> event/trigger
> 
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
>
Kalesh Singh Oct. 19, 2021, 9:21 p.m. UTC | #2
On Tue, Oct 19, 2021 at 1:48 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 15 Sep 2021 19:52:45 +0000
> Kalesh Singh <kaleshsingh@google.com> wrote:
>
> > Currently hist trigger expressions don't support the use of numeric
> > literals:
> >       e.g. echo 'hist:keys=common_pid:x=$y-1234'
> >               --> is not valid expression syntax
> >
> > Having the ability to use numeric constants in hist triggers supports
> > a wider range of expressions for creating variables.
>
> I'm not against the patch, but I'm curious to what use case this would be
> useful for. In the cover letter it mentions the division and multiplication
> for finding associated buckets, but what is the addition / subtraction used
> for?

Addition and subtraction are already supported operations. The use
case given in Documentation/trace/histogram.rst is for calculating
latencies. I don't have a use case where the constants may be needed
in the addition/subtraction, but for simplicity and completeness we
support them for all operations.

- Kalesh

>
> -- Steve
>
>
> >
> > Add support for creating trace event histogram variables from numeric
> > literals.
> >
> >       e.g. echo 'hist:keys=common_pid:x=1234,y=size-1024' >> event/trigger
> >
> > A negative numeric constant is created, using unary minus operator
> > (parentheses are required).
> >
> >       e.g. echo 'hist:keys=common_pid:z=-(2)' >> event/trigger
> >
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> >
Steven Rostedt Oct. 19, 2021, 9:26 p.m. UTC | #3
On Tue, 19 Oct 2021 14:21:04 -0700
Kalesh Singh <kaleshsingh@google.com> wrote:

> Addition and subtraction are already supported operations. The use
> case given in Documentation/trace/histogram.rst is for calculating
> latencies. I don't have a use case where the constants may be needed
> in the addition/subtraction, but for simplicity and completeness we
> support them for all operations.

Perhaps we should state that it's mostly added for completeness then.

Thanks,

-- Steve
Kalesh Singh Oct. 19, 2021, 9:49 p.m. UTC | #4
On Tue, Oct 19, 2021 at 2:26 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 19 Oct 2021 14:21:04 -0700
> Kalesh Singh <kaleshsingh@google.com> wrote:
>
> > Addition and subtraction are already supported operations. The use
> > case given in Documentation/trace/histogram.rst is for calculating
> > latencies. I don't have a use case where the constants may be needed
> > in the addition/subtraction, but for simplicity and completeness we
> > support them for all operations.
>
> Perhaps we should state that it's mostly added for completeness then.

Good point. I'll update the commit message to clarify it.

Thanks,
Kalesh
>
> Thanks,
>
> -- Steve
diff mbox series

Patch

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index a6061a69aa84..2802b211ccf3 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -66,7 +66,9 @@ 
 	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_STR_OPERAND,	"String type can not be an operand in expression"),
+	C(INVALID_STR_OPERAND,	"String type can not be an operand in expression"), \
+	C(TOO_MANY_CONSTS,	"Too many constants defined"),		\
+	C(EXPECT_NUMBER,	"Expecting numeric literal"),
 
 #undef C
 #define C(a, b)		HIST_ERR_##a
@@ -89,6 +91,8 @@  typedef u64 (*hist_field_fn_t) (struct hist_field *field,
 #define HIST_FIELD_OPERANDS_MAX	2
 #define HIST_FIELDS_MAX		(TRACING_MAP_FIELDS_MAX + TRACING_MAP_VARS_MAX)
 #define HIST_ACTIONS_MAX	8
+#define HIST_CONST_MAX		4
+#define HIST_CONST_DIGITS_MAX	21
 
 enum field_op_id {
 	FIELD_OP_NONE,
@@ -152,6 +156,9 @@  struct hist_field {
 	bool                            read_once;
 
 	unsigned int			var_str_idx;
+
+	/* Numeric literals are represented as u64 */
+	u64				constant;
 };
 
 static u64 hist_field_none(struct hist_field *field,
@@ -163,6 +170,15 @@  static u64 hist_field_none(struct hist_field *field,
 	return 0;
 }
 
+static u64 hist_field_const(struct hist_field *field,
+			   struct tracing_map_elt *elt,
+			   struct trace_buffer *buffer,
+			   struct ring_buffer_event *rbe,
+			   void *event)
+{
+	return field->constant;
+}
+
 static u64 hist_field_counter(struct hist_field *field,
 			      struct tracing_map_elt *elt,
 			      struct trace_buffer *buffer,
@@ -341,6 +357,7 @@  enum hist_field_flags {
 	HIST_FIELD_FL_CPU		= 1 << 15,
 	HIST_FIELD_FL_ALIAS		= 1 << 16,
 	HIST_FIELD_FL_BUCKET		= 1 << 17,
+	HIST_FIELD_FL_CONST		= 1 << 18,
 };
 
 struct var_defs {
@@ -410,6 +427,7 @@  struct hist_trigger_data {
 	struct field_var		*save_vars[SYNTH_FIELDS_MAX];
 	unsigned int			n_save_vars;
 	unsigned int			n_save_var_str;
+	unsigned int			n_constants;
 };
 
 struct action_data;
@@ -1516,6 +1534,12 @@  static void expr_field_str(struct hist_field *field, char *expr)
 {
 	if (field->flags & HIST_FIELD_FL_VAR_REF)
 		strcat(expr, "$");
+	else if (field->flags & HIST_FIELD_FL_CONST) {
+		char str[HIST_CONST_DIGITS_MAX];
+
+		snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
+		strcat(expr, str);
+	}
 
 	strcat(expr, hist_field_name(field, 0));
 
@@ -1689,6 +1713,15 @@  static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
 		goto out;
 	}
 
+	if (flags & HIST_FIELD_FL_CONST) {
+		hist_field->fn = hist_field_const;
+		hist_field->size = sizeof(u64);
+		hist_field->type = kstrdup("u64", GFP_KERNEL);
+		if (!hist_field->type)
+			goto free;
+		goto out;
+	}
+
 	if (flags & HIST_FIELD_FL_STACKTRACE) {
 		hist_field->fn = hist_field_none;
 		goto out;
@@ -2090,6 +2123,35 @@  static struct hist_field *create_alias(struct hist_trigger_data *hist_data,
 	return alias;
 }
 
+static struct hist_field *parse_const(struct hist_trigger_data *hist_data,
+				      char *str, char *var_name,
+				      unsigned long *flags)
+{
+	struct trace_array *tr = hist_data->event_file->tr;
+	struct hist_field *field = NULL;
+	u64 constant;
+
+	if (hist_data->n_constants >= HIST_CONST_MAX) {
+		hist_err(tr, HIST_ERR_TOO_MANY_CONSTS, errpos(str));
+		return NULL;
+	}
+
+	if (kstrtoull(str, 0, &constant)) {
+		hist_err(tr, HIST_ERR_EXPECT_NUMBER, errpos(str));
+		return NULL;
+	}
+
+	*flags |= HIST_FIELD_FL_CONST;
+	field = create_hist_field(hist_data, NULL, *flags, var_name);
+	if (!field)
+		return NULL;
+
+	field->constant = constant;
+	hist_data->n_constants++;
+
+	return field;
+}
+
 static struct hist_field *parse_atom(struct hist_trigger_data *hist_data,
 				     struct trace_event_file *file, char *str,
 				     unsigned long *flags, char *var_name)
@@ -2100,6 +2162,15 @@  static struct hist_field *parse_atom(struct hist_trigger_data *hist_data,
 	unsigned long buckets = 0;
 	int ret = 0;
 
+	if (isdigit(str[0])) {
+		hist_field = parse_const(hist_data, str, var_name, flags);
+		if (!hist_field) {
+			ret = -EINVAL;
+			goto out;
+		}
+		return hist_field;
+	}
+
 	s = strchr(str, '.');
 	if (s) {
 		s = strchr(++s, '.');
@@ -4950,6 +5021,8 @@  static void hist_field_debug_show_flags(struct seq_file *m,
 
 	if (flags & HIST_FIELD_FL_ALIAS)
 		seq_puts(m, "        HIST_FIELD_FL_ALIAS\n");
+	else if (flags & HIST_FIELD_FL_CONST)
+		seq_puts(m, "        HIST_FIELD_FL_CONST\n");
 }
 
 static int hist_field_debug_show(struct seq_file *m,
@@ -4971,6 +5044,9 @@  static int hist_field_debug_show(struct seq_file *m,
 			   field->var.idx);
 	}
 
+	if (field->flags & HIST_FIELD_FL_CONST)
+		seq_printf(m, "      constant: %llu\n", field->constant);
+
 	if (field->flags & HIST_FIELD_FL_ALIAS)
 		seq_printf(m, "      var_ref_idx (into hist_data->var_refs[]): %u\n",
 			   field->var_ref_idx);
@@ -5213,6 +5289,8 @@  static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
 
 	if (hist_field->flags & HIST_FIELD_FL_CPU)
 		seq_puts(m, "common_cpu");
+	else if (hist_field->flags & HIST_FIELD_FL_CONST)
+		seq_printf(m, "%llu", hist_field->constant);
 	else if (field_name) {
 		if (hist_field->flags & HIST_FIELD_FL_VAR_REF ||
 		    hist_field->flags & HIST_FIELD_FL_ALIAS)