| Message ID | 163767457857.543504.6091850281066240970.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 512d7be113c632268b4b68d73dacc4a40d6661f9 |
| Headers | show |
| Series | [v2] libtraceevent: Add __rel_loc relative location attribute support | expand |
On Tue, 23 Nov 2021 22:36:18 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > +++ b/src/event-parse.h > @@ -125,6 +125,7 @@ enum tep_format_flags { > TEP_FIELD_IS_LONG = 32, > TEP_FIELD_IS_FLAG = 64, > TEP_FIELD_IS_SYMBOLIC = 128, > + TEP_FIELD_IS_RELATIVE = 256, > }; > > struct tep_format_field { > @@ -153,12 +154,12 @@ struct tep_print_arg_atom { > > struct tep_print_arg_string { > char *string; > - int offset; > + struct tep_format_field *field; > }; > > struct tep_print_arg_bitmask { > char *bitmask; > - int offset; > + struct tep_format_field *field; > }; > We need to be careful about modifying content in event-parse.h. Because this is exposed outside he library. Although, I don't think this should be an issue. We probably need to move some content out of that file if it isn't used in any of the exposed APIs :-/ And since both libtracefs and trace-cmd do not reference any of the tep_print_arg* fields, we probably should remove them before somebody does. -- Steve
On Wed, 24 Nov 2021 18:07:08 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Tue, 23 Nov 2021 22:36:18 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > +++ b/src/event-parse.h > > @@ -125,6 +125,7 @@ enum tep_format_flags { > > TEP_FIELD_IS_LONG = 32, > > TEP_FIELD_IS_FLAG = 64, > > TEP_FIELD_IS_SYMBOLIC = 128, > > + TEP_FIELD_IS_RELATIVE = 256, > > }; > > > > struct tep_format_field { > > @@ -153,12 +154,12 @@ struct tep_print_arg_atom { > > > > struct tep_print_arg_string { > > char *string; > > - int offset; > > + struct tep_format_field *field; > > }; > > > > struct tep_print_arg_bitmask { > > char *bitmask; > > - int offset; > > + struct tep_format_field *field; > > }; > > > > We need to be careful about modifying content in event-parse.h. Because > this is exposed outside he library. Oh, I thought this is an internal header, because it is under src/ directory. However, indeed "TEP_FIELD_IS_*" referred from perf-tools, so this might be need to be exposed. > > Although, I don't think this should be an issue. > > We probably need to move some content out of that file if it isn't used in > any of the exposed APIs :-/ > > And since both libtracefs and trace-cmd do not reference any of the > tep_print_arg* fields, we probably should remove them before somebody does. Yes, tep_print_arg seems to be used only in the event-parse.c. Thank you, > > -- Steve
On Fri, 26 Nov 2021 21:20:26 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > > We need to be careful about modifying content in event-parse.h. Because > > this is exposed outside he library. > > Oh, I thought this is an internal header, because it is under src/ directory. > However, indeed "TEP_FIELD_IS_*" referred from perf-tools, so this might be > need to be exposed. Yeah, we need to fix this. The issue was that this is loaded with legacy code. The code was created in trace-cmd, ported to the kernel into tools/lib/traceevent, then pulled out into it's own directory. The poor library had so many foster parents, that it wasn't ready to go out on its own. I'll fix some of this before posting a new version. -- Steve
On Tue, Nov 23, 2021 at 10:36:18PM +0900, Masami Hiramatsu wrote: > Add '__rel_loc' new dynamic data location attribute which encodes > the data location from the next to the field itself. This is similar > to the '__data_loc' but the location offset is not from the event > entry but from the next of the field. > > This patch adds '__rel_loc' decoding support in the libtraceevent. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > --- > Changes in v2: > - Fix to adjust '*offset' instead of 'offset' in dynamic_offset_field() > --- > src/event-parse.c | 54 +++++++++++++++++++++++++++++++++------------------- > src/event-parse.h | 5 +++-- > src/parse-filter.c | 5 ++++- > 3 files changed, 41 insertions(+), 23 deletions(-) > > diff --git a/src/event-parse.c b/src/event-parse.c > index 70637586bc9a..a88c73a8d0fb 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c [..] > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg, > free_token(token); > return process_int_array(event, arg, tok); > } > - if (strcmp(token, "__get_str") == 0) { > + if (strcmp(token, "__get_str") == 0 || > + strcmp(token, "__get_rel_str") == 0) { Should user_events use __get_rel_str vs __get_str for the print_fmt? Both __dyn_loc and __rel_loc use __get_str currently. Thanks, -Beau
On Mon, 29 Nov 2021 12:15:07 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: > > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg, > > free_token(token); > > return process_int_array(event, arg, tok); > > } > > - if (strcmp(token, "__get_str") == 0) { > > + if (strcmp(token, "__get_str") == 0 || > > + strcmp(token, "__get_rel_str") == 0) { > > Should user_events use __get_rel_str vs __get_str for the print_fmt? > Both __dyn_loc and __rel_loc use __get_str currently. I'm guessing that it should use the get_rel_str(), as get_str() will use the absolute offset and not the relative one. -- Steve
On Mon, Nov 29, 2021 at 03:27:04PM -0500, Steven Rostedt wrote: > On Mon, 29 Nov 2021 12:15:07 -0800 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg, > > > free_token(token); > > > return process_int_array(event, arg, tok); > > > } > > > - if (strcmp(token, "__get_str") == 0) { > > > + if (strcmp(token, "__get_str") == 0 || > > > + strcmp(token, "__get_rel_str") == 0) { > > > > Should user_events use __get_rel_str vs __get_str for the print_fmt? > > Both __dyn_loc and __rel_loc use __get_str currently. > > I'm guessing that it should use the get_rel_str(), as get_str() will use > the absolute offset and not the relative one. > > -- Steve It appears both cases call into process_str() and set the TEP_PRINT_STRING field type. The TEP_FIELD_IS_RELATIVE bit to advance the offset to a relative position is within dynamic_offset which is used for TEP_PRINT_STRING field types. I'm not sure if this was intentional or if __get_rel_str is an artifact left behind considering __get_str appears to be doing the same thing? Thanks, -Beau
On Mon, 29 Nov 2021 13:26:35 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: > On Mon, Nov 29, 2021 at 03:27:04PM -0500, Steven Rostedt wrote: > > On Mon, 29 Nov 2021 12:15:07 -0800 > > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > > > @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg, > > > > free_token(token); > > > > return process_int_array(event, arg, tok); > > > > } > > > > - if (strcmp(token, "__get_str") == 0) { > > > > + if (strcmp(token, "__get_str") == 0 || > > > > + strcmp(token, "__get_rel_str") == 0) { > > > > > > Should user_events use __get_rel_str vs __get_str for the print_fmt? > > > Both __dyn_loc and __rel_loc use __get_str currently. > > > > I'm guessing that it should use the get_rel_str(), as get_str() will use > > the absolute offset and not the relative one. Yes, you can see an example in this patch; https://lore.kernel.org/all/163757343050.510314.2876529802471645178.stgit@devnote2/T/#u Thus, the user-event must tell the kernel which is used correctly. > > > > -- Steve > > It appears both cases call into process_str() and set the > TEP_PRINT_STRING field type. > > The TEP_FIELD_IS_RELATIVE bit to advance the offset to a relative position > is within dynamic_offset which is used for TEP_PRINT_STRING field types. Correct. The field type attribute is set in event_read_fields() before this process. > > I'm not sure if this was intentional or if __get_rel_str is an artifact > left behind considering __get_str appears to be doing the same thing? Note that this patch is for libtraceevent, not in-kernel code. In the kernel, for example, your user-event, you must make a flag for each field to identify that is __rel_loc, __data_loc, or just a value. The event filter (and histogram) will parse the "events/*/*/format" file and identify the field is __rel_loc or __data_loc. https://lore.kernel.org/all/163757343050.510314.2876529802471645178.stgit@devnote2/T/#m98591a73fb48e4ac79ed6d2e8eb14a13c69703c8 And __get_str()/__get_rel_str() are used in the event which statically defined (Note that trace_event field itself has no information about the __rel_loc/__data_loc, so print_fmt function for each field must handle that correctly.) Thank you,
diff --git a/src/event-parse.c b/src/event-parse.c index 70637586bc9a..a88c73a8d0fb 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -1529,6 +1529,14 @@ static int field_is_dynamic(struct tep_format_field *field) return 0; } +static int field_is_relative_dynamic(struct tep_format_field *field) +{ + if (strncmp(field->type, "__rel_loc", 9) == 0) + return 1; + + return 0; +} + static int field_is_long(struct tep_format_field *field) { /* includes long long */ @@ -1784,6 +1792,8 @@ static int event_read_fields(struct tep_event *event, struct tep_format_field ** field->flags |= TEP_FIELD_IS_STRING; if (field_is_dynamic(field)) field->flags |= TEP_FIELD_IS_DYNAMIC; + if (field_is_relative_dynamic(field)) + field->flags |= TEP_FIELD_IS_DYNAMIC | TEP_FIELD_IS_RELATIVE; if (field_is_long(field)) field->flags |= TEP_FIELD_IS_LONG; @@ -3113,7 +3123,7 @@ process_str(struct tep_event *event __maybe_unused, struct tep_print_arg *arg, arg->type = TEP_PRINT_STRING; arg->string.string = token; - arg->string.offset = -1; + arg->string.field = NULL; if (read_expected(TEP_EVENT_DELIM, ")") < 0) goto out_err; @@ -3142,7 +3152,7 @@ process_bitmask(struct tep_event *event __maybe_unused, struct tep_print_arg *ar arg->type = TEP_PRINT_BITMASK; arg->bitmask.bitmask = token; - arg->bitmask.offset = -1; + arg->bitmask.field = NULL; if (read_expected(TEP_EVENT_DELIM, ")") < 0) goto out_err; @@ -3308,19 +3318,23 @@ process_function(struct tep_event *event, struct tep_print_arg *arg, free_token(token); return process_int_array(event, arg, tok); } - if (strcmp(token, "__get_str") == 0) { + if (strcmp(token, "__get_str") == 0 || + strcmp(token, "__get_rel_str") == 0) { free_token(token); return process_str(event, arg, tok); } - if (strcmp(token, "__get_bitmask") == 0) { + if (strcmp(token, "__get_bitmask") == 0 || + strcmp(token, "__get_rel_bitmask") == 0) { free_token(token); return process_bitmask(event, arg, tok); } - if (strcmp(token, "__get_dynamic_array") == 0) { + if (strcmp(token, "__get_dynamic_array") == 0 || + strcmp(token, "__get_rel_dynamic_array") == 0) { free_token(token); return process_dynamic_array(event, arg, tok); } - if (strcmp(token, "__get_dynamic_array_len") == 0) { + if (strcmp(token, "__get_dynamic_array_len") == 0 || + strcmp(token, "__get_rel_dynamic_array_len") == 0) { free_token(token); return process_dynamic_array_len(event, arg, tok); } @@ -3886,6 +3900,8 @@ static inline void dynamic_offset_field(struct tep_handle *tep, unsigned int *len) { dynamic_offset(tep, field->size, data + field->offset, offset, len); + if (field->flags & TEP_FIELD_IS_RELATIVE) + *offset += field->offset + field->size; } static unsigned long long @@ -4398,13 +4414,11 @@ static void print_str_arg(struct trace_seq *s, void *data, int size, case TEP_PRINT_TYPE: break; case TEP_PRINT_STRING: { - if (arg->string.offset == -1) { - struct tep_format_field *f; - - f = tep_find_any_field(event, arg->string.string); - arg->string.offset = f->offset; - } - dynamic_offset(tep, 4, data + arg->string.offset, &offset, &len); + if (!arg->string.field) + arg->string.field = tep_find_any_field(event, arg->string.string); + if (!arg->string.field) + break; + dynamic_offset_field(tep, arg->string.field, data, &offset, &len); /* Do not attempt to save zero length dynamic strings */ if (!len) break; @@ -4415,13 +4429,11 @@ static void print_str_arg(struct trace_seq *s, void *data, int size, print_str_to_seq(s, format, len_arg, arg->string.string); break; case TEP_PRINT_BITMASK: { - if (arg->bitmask.offset == -1) { - struct tep_format_field *f; - - f = tep_find_any_field(event, arg->bitmask.bitmask); - arg->bitmask.offset = f->offset; - } - dynamic_offset(tep, 4, data + arg->bitmask.offset, &offset, &len); + if (!arg->bitmask.field) + arg->bitmask.field = tep_find_any_field(event, arg->bitmask.bitmask); + if (!arg->bitmask.field) + break; + dynamic_offset_field(tep, arg->bitmask.field, data, &offset, &len); print_bitmask_to_seq(tep, s, format, len_arg, data + offset, len); break; @@ -7343,6 +7355,8 @@ void *tep_get_field_raw(struct trace_seq *s, struct tep_event *event, data + offset, field->size); *len = offset >> 16; offset &= 0xffff; + if (field->flags & TEP_FIELD_IS_RELATIVE) + offset += field->offset + field->size; } else *len = field->size; diff --git a/src/event-parse.h b/src/event-parse.h index 083389358127..db8bc6db5431 100644 --- a/src/event-parse.h +++ b/src/event-parse.h @@ -125,6 +125,7 @@ enum tep_format_flags { TEP_FIELD_IS_LONG = 32, TEP_FIELD_IS_FLAG = 64, TEP_FIELD_IS_SYMBOLIC = 128, + TEP_FIELD_IS_RELATIVE = 256, }; struct tep_format_field { @@ -153,12 +154,12 @@ struct tep_print_arg_atom { struct tep_print_arg_string { char *string; - int offset; + struct tep_format_field *field; }; struct tep_print_arg_bitmask { char *bitmask; - int offset; + struct tep_format_field *field; }; struct tep_print_arg_field { diff --git a/src/parse-filter.c b/src/parse-filter.c index 368826bb5a57..5df177070d53 100644 --- a/src/parse-filter.c +++ b/src/parse-filter.c @@ -1712,8 +1712,11 @@ static const char *get_field_str(struct tep_filter_arg *arg, struct tep_record * if (arg->str.field->flags & TEP_FIELD_IS_DYNAMIC) { addr = *(unsigned int *)val; - val = record->data + (addr & 0xffff); size = addr >> 16; + addr &= 0xffff; + if (arg->str.field->flags & TEP_FIELD_IS_RELATIVE) + addr += arg->str.field->offset + arg->str.field->size; + val = record->data + addr; } /*
Add '__rel_loc' new dynamic data location attribute which encodes the data location from the next to the field itself. This is similar to the '__data_loc' but the location offset is not from the event entry but from the next of the field. This patch adds '__rel_loc' decoding support in the libtraceevent. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- Changes in v2: - Fix to adjust '*offset' instead of 'offset' in dynamic_offset_field() --- src/event-parse.c | 54 +++++++++++++++++++++++++++++++++------------------- src/event-parse.h | 5 +++-- src/parse-filter.c | 5 ++++- 3 files changed, 41 insertions(+), 23 deletions(-)