[RFC PATCH 0/1] tracing: support dynamic string field types for synthetic events

[RFC PATCH 0/1] tracing: support dynamic string field types for synthetic events

[Axel Rasmussen]

Hi Steven and Tom,

In this thread: https://lkml.org/lkml/2020/9/17/1015 we discussed how to plumb
dynamic strings into synthetic events. Tom, you proposed adding a new dynamic
string type to synthetic event definition like "char foo[]".

I'm sending this patch because it may be simpler than implementing that (I'm
not too familiar with the tracing infrastructure, apologies if this is not
true), and in my testing it seems sufficient to address my use case. I tested
both setting up a synthetic event as Steven described in the other thread, as
well as doing an analogous thing with a small bpftrace program, and both work as
expected with this patch.

This is because I happen to know there's an upper bound on the length of the
string in question, so I can just define a "char memcg_path[256]" in the
synthetic event, and I can be sure the string won't be truncated.

Let me know what you think. Happy to drop this and wait for Tom's suggested
approach instead.

Axel Rasmussen (1):
  tracing: support dynamic string field types for synthetic events

 kernel/trace/trace_events_hist.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

--
2.28.0.681.g6f77f65b4e-goog

[RFC PATCH 1/1] tracing: support dynamic string field types for synthetic events

[Axel Rasmussen]

It's typical [1] to define tracepoint string fields as "const char *",
using the __string, __assign_str, and __get_str helpers. For synthetic
event definitions, the only available mechanism to define a string type
is a fixed-size char array ("char[]") [2].

Without this patch, since the type strings aren't identical, and the
sizes don't match (since one is an array, and the other is a "dynamic
string" integer), they are considered incompatible [3].

This patch modifies that check, so as to let us setup synthetic events,
and plumb through string values from typical tracepoints. It turns out
this is already handled correctly, as long as the check during
definition parsing doesn't prevent it.

[1] grep -r "__string" include/trace/events/
[2] See synth_field_is_string in kernel/trace/trace_events_synth.c
[3] See check_synth_field in kernel/trace/trace_events_hist.c

Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
---
 kernel/trace/trace_events_hist.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 0b933546142e..e064feb3cc65 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -3280,9 +3280,18 @@ static int check_synth_field(struct synth_event *event,
 	field = event->fields[field_pos];
 
 	if (strcmp(field->type, hist_field->type) != 0) {
-		if (field->size != hist_field->size ||
-		    field->is_signed != hist_field->is_signed)
-			return -EINVAL;
+		/*
+		 * If both are kinds of strings, they match. We can't use
+		 * is_string_field for the hist_field, as it's only sort of
+		 * partially initialized at this point.
+		 */
+		if (strstr(field->type, "char[") == NULL ||
+		    strstr(hist_field->type, "char[") == NULL) {
+			/* They still match if size and signedness match. */
+			if (field->size != hist_field->size ||
+			    field->is_signed != hist_field->is_signed)
+				return -EINVAL;
+		}
 	}
 
 	return 0;
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [RFC PATCH 0/1] tracing: support dynamic string field types for synthetic events

[Steven Rostedt]

On Fri, 25 Sep 2020 12:08:05 -0700
Axel Rasmussen <axelrasmussen@google.com> wrote:

> Hi Steven and Tom,
> 
> In this thread: https://lkml.org/lkml/2020/9/17/1015 we discussed how to plumb
> dynamic strings into synthetic events. Tom, you proposed adding a new dynamic
> string type to synthetic event definition like "char foo[]".

Thanks Axel,

As this is Tom's code, I'll let him give his review.

-- Steve

Re: [RFC PATCH 0/1] tracing: support dynamic string field types for synthetic events

[Tom Zanussi]

Hi Axel,

On Fri, 2020-09-25 at 12:08 -0700, Axel Rasmussen wrote:
> Hi Steven and Tom,
> 
> In this thread: https://lkml.org/lkml/2020/9/17/1015 we discussed how
> to plumb
> dynamic strings into synthetic events. Tom, you proposed adding a new
> dynamic
> string type to synthetic event definition like "char foo[]".
> 
> I'm sending this patch because it may be simpler than implementing
> that (I'm
> not too familiar with the tracing infrastructure, apologies if this
> is not
> true), and in my testing it seems sufficient to address my use case.
> I tested
> both setting up a synthetic event as Steven described in the other
> thread, as
> well as doing an analogous thing with a small bpftrace program, and
> both work as
> expected with this patch.
> 
> This is because I happen to know there's an upper bound on the length
> of the
> string in question, so I can just define a "char memcg_path[256]" in
> the
> synthetic event, and I can be sure the string won't be truncated.
> 
> Let me know what you think. Happy to drop this and wait for Tom's
> suggested
> approach instead.

Changing check_synth_field() is one of the things that will need to
change for this to work - I'm working on a patch but am kind of in the
middle of it, if you can wait - I expect to be able to post something
Monday...

Thanks,

Tom

> 
> Axel Rasmussen (1):
>   tracing: support dynamic string field types for synthetic events
> 
>  kernel/trace/trace_events_hist.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> --
> 2.28.0.681.g6f77f65b4e-goog
> 

Re: [RFC PATCH 1/1] tracing: support dynamic string field types for synthetic events

[Tom Zanussi]

Hi Axel,

On Fri, 2020-09-25 at 12:08 -0700, Axel Rasmussen wrote:
> It's typical [1] to define tracepoint string fields as "const char *",
> using the __string, __assign_str, and __get_str helpers. For synthetic
> event definitions, the only available mechanism to define a string type
> is a fixed-size char array ("char[]") [2].
> 
> Without this patch, since the type strings aren't identical, and the
> sizes don't match (since one is an array, and the other is a "dynamic
> string" integer), they are considered incompatible [3].
> 
> This patch modifies that check, so as to let us setup synthetic events,
> and plumb through string values from typical tracepoints. It turns out
> this is already handled correctly, as long as the check during
> definition parsing doesn't prevent it.
> 
> [1] grep -r "__string" include/trace/events/
> [2] See synth_field_is_string in kernel/trace/trace_events_synth.c
> [3] See check_synth_field in kernel/trace/trace_events_hist.c
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  kernel/trace/trace_events_hist.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 0b933546142e..e064feb3cc65 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -3280,9 +3280,18 @@ static int check_synth_field(struct synth_event *event,
>  	field = event->fields[field_pos];
>  
>  	if (strcmp(field->type, hist_field->type) != 0) {
> -		if (field->size != hist_field->size ||
> -		    field->is_signed != hist_field->is_signed)
> -			return -EINVAL;

One thing is that this check doesn't just apply to strings, so dropping
this will skip those other cases.  In any case, the patch I'm working
on will handle this properly along with the other changes.

Thanks,

Tom

> +		/*
> +		 * If both are kinds of strings, they match. We can't use
> +		 * is_string_field for the hist_field, as it's only sort of
> +		 * partially initialized at this point.
> +		 */
> +		if (strstr(field->type, "char[") == NULL ||
> +		    strstr(hist_field->type, "char[") == NULL) {
> +			/* They still match if size and signedness match. */
> +			if (field->size != hist_field->size ||
> +			    field->is_signed != hist_field->is_signed)
> +				return -EINVAL;
> +		}
>  	}
>  
>  	return 0;

Re: [RFC PATCH 0/1] tracing: support dynamic string field types for synthetic events

[Axel Rasmussen]

On Fri, Sep 25, 2020 at 12:48 PM Tom Zanussi <zanussi@kernel.org> wrote:
>
> Hi Axel,
>
> On Fri, 2020-09-25 at 12:08 -0700, Axel Rasmussen wrote:
> > Hi Steven and Tom,
> >
> > In this thread: https://lkml.org/lkml/2020/9/17/1015 we discussed how
> > to plumb
> > dynamic strings into synthetic events. Tom, you proposed adding a new
> > dynamic
> > string type to synthetic event definition like "char foo[]".
> >
> > I'm sending this patch because it may be simpler than implementing
> > that (I'm
> > not too familiar with the tracing infrastructure, apologies if this
> > is not
> > true), and in my testing it seems sufficient to address my use case.
> > I tested
> > both setting up a synthetic event as Steven described in the other
> > thread, as
> > well as doing an analogous thing with a small bpftrace program, and
> > both work as
> > expected with this patch.
> >
> > This is because I happen to know there's an upper bound on the length
> > of the
> > string in question, so I can just define a "char memcg_path[256]" in
> > the
> > synthetic event, and I can be sure the string won't be truncated.
> >
> > Let me know what you think. Happy to drop this and wait for Tom's
> > suggested
> > approach instead.
>
> Changing check_synth_field() is one of the things that will need to
> change for this to work - I'm working on a patch but am kind of in the
> middle of it, if you can wait - I expect to be able to post something
> Monday...
>
> Thanks,
>
> Tom

Absolutely, no problem waiting. I didn't mean to come off as being
impatient; mostly since I had already hacked this together a few days
ago, I figured it was worth an e-mail in case it could save you some
effort. I have no problem dropping it. :) Thanks for working on this!

>
> >
> > Axel Rasmussen (1):
> >   tracing: support dynamic string field types for synthetic events
> >
> >  kernel/trace/trace_events_hist.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > --
> > 2.28.0.681.g6f77f65b4e-goog
> >
>