| Message ID | 20191203103522.482684-15-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Timestamp synchronization of host - guest tracing session | expand |
On Tue, 3 Dec 2019 12:35:18 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > When tracing host and guest machines, both should use the same > tracing clock for event timestamps. If a clock is specified > as host tracing argument, with option "-C clock_name", the same > is injected as guest tracing argument. If the user wants to use > different tracing clocks, it can specify it using "-C clock_name" > as guest tracing argument. In that case, the one specified by > the user has higher priority. > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > tracecmd/trace-record.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index 49730d6..e7fb1bd 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -5405,6 +5405,8 @@ static void parse_record_options(int argc, > char *sav; > int name_counter = 0; > int neg_event = 0; > + struct buffer_instance *instance; > + bool guest_config = false; Should have a different name, like "clock_set", as "guest_config" doesn't let us know what this is about. > > init_common_record_context(ctx, curr_cmd); > > @@ -5562,6 +5564,7 @@ static void parse_record_options(int argc, > break; > case 'C': > ctx->instance->ftrace->clock = optarg; > + guest_config = true; > break; > case 'v': > neg_event = 1; > @@ -5779,14 +5782,26 @@ static void parse_record_options(int argc, > > /* If --date is specified, prepend it to all guest VM flags */ > if (ctx->date) { > - struct buffer_instance *instance; > - > for_all_instances(instance) { > if (is_guest(instance)) > add_argv(instance, "--date", true); > } > } > > + if (guest_config) { > + /* If -C is specified, prepend clock to all guest VM flags */ > + for_all_instances(instance) { > + if (top_instance.ftrace->clock) { > + if (is_guest(instance)) { We should only append this, if the guest didn't have a clock set already. As the change log seems to say, if the user states a "-C clock" for the guest, that should take precedence over the host clock set. That is, a user may specifically state that they are using a different clock. If we have frequency and offset set, it should still work with different clocks. -- Steve > + add_argv(instance, > + (char *)top_instance.ftrace->clock, > + true); > + add_argv(instance, "-C", true); > + } > + } > + } > + } > + > if (!ctx->filtered && ctx->instance->filter_mod) > add_func(&ctx->instance->filter_funcs, > ctx->instance->filter_mod, "*");
On Mon, Dec 9, 2019 at 9:31 PM Steven Rostedt <rostedt@goodmis.org> wrote: > [ ... ] > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index 49730d6..e7fb1bd 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > @@ -5405,6 +5405,8 @@ static void parse_record_options(int argc, > > char *sav; > > int name_counter = 0; > > int neg_event = 0; > > + struct buffer_instance *instance; > > + bool guest_config = false; > > Should have a different name, like "clock_set", as "guest_config" > doesn't let us know what this is about. I use the same flag in patch 16 from the same series, with the same purpose - to apply host config to the guest. That's why decided to call it "guest_config". > > > > > > init_common_record_context(ctx, curr_cmd); > > > > @@ -5562,6 +5564,7 @@ static void parse_record_options(int argc, > > break; > > case 'C': > > ctx->instance->ftrace->clock = optarg; > > + guest_config = true; > > break; > > case 'v': > > neg_event = 1; > > @@ -5779,14 +5782,26 @@ static void parse_record_options(int argc, > > > > /* If --date is specified, prepend it to all guest VM flags */ > > if (ctx->date) { > > - struct buffer_instance *instance; > > - > > for_all_instances(instance) { > > if (is_guest(instance)) > > add_argv(instance, "--date", true); > > } > > } > > > > + if (guest_config) { > > + /* If -C is specified, prepend clock to all guest VM flags */ > > + for_all_instances(instance) { > > + if (top_instance.ftrace->clock) { > > + if (is_guest(instance)) { > > We should only append this, if the guest didn't have a clock set > already. As the change log seems to say, if the user states a "-C > clock" for the guest, that should take precedence over the host clock > set. That is, a user may specifically state that they are using a > different clock. If we have frequency and offset set, it should still > work with different clocks. The guest config string is not parsed in the host context, that's why the host doesn't know if a guest has an explicit "-C clock" argument. I can parse the guest config here, but this will complicate the implementation. Using the current approach still guarantees that the user specified config has higher priority than injected one - add_argv() API prepends to the beginning of the string, so user arguments are always after the injected one. When guest parses the string, in case of duplicated "-C clock" arguments, the last one wins. > > -- Steve > > > > + add_argv(instance, > > + (char *)top_instance.ftrace->clock, > > + true); > > + add_argv(instance, "-C", true); > > + } > > + } > > + } > > + } > > + > > if (!ctx->filtered && ctx->instance->filter_mod) > > add_func(&ctx->instance->filter_funcs, > > ctx->instance->filter_mod, "*"); > Thanks !
On Tue, 10 Dec 2019 10:49:43 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > > + if (guest_config) { > > > + /* If -C is specified, prepend clock to all guest VM flags */ > > > + for_all_instances(instance) { > > > + if (top_instance.ftrace->clock) { > > > + if (is_guest(instance)) { > > > > We should only append this, if the guest didn't have a clock set > > already. As the change log seems to say, if the user states a "-C > > clock" for the guest, that should take precedence over the host clock > > set. That is, a user may specifically state that they are using a > > different clock. If we have frequency and offset set, it should still > > work with different clocks. > > The guest config string is not parsed in the host context, that's why the host > doesn't know if a guest has an explicit "-C clock" argument. > I can parse the guest config here, but this will complicate the implementation. > Using the current approach still guarantees that the user specified > config has higher > priority than injected one - add_argv() API prepends to the beginning > of the string, so > user arguments are always after the injected one. When guest parses > the string, in case of > duplicated "-C clock" arguments, the last one wins. I'm confused. I'm looking at this: for (;;) { [..] switch (c) { [..] case 'A': [..] ctx->instance->flags |= BUFFER_FL_GUEST; [..] case 'C': ctx->instance->ftrace->clock = optarg; guest_config = true; break; [..] } [..] } [..] if (guest_config) { /* If -C is specified, prepend clock to all guest VM flags */ for_all_instances(instance) { if (top_instance.ftrace->clock) { Why can't we have here: if (top_instance.ftrace->clock && !instance->ftrace->clock) If the guest instance was given a -C, I would think we don't want to add another -C to pass to that guest? -- Steve if (is_guest(instance)) { add_argv(instance, (char *)top_instance.ftrace->clock, true); add_argv(instance, "-C", true); } } } }
On Tue, Dec 10, 2019 at 5:48 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 10 Dec 2019 10:49:43 +0200 > Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > > > > + if (guest_config) { > > > > + /* If -C is specified, prepend clock to all guest VM flags */ > > > > + for_all_instances(instance) { > > > > + if (top_instance.ftrace->clock) { > > > > + if (is_guest(instance)) { > > > > > > We should only append this, if the guest didn't have a clock set > > > already. As the change log seems to say, if the user states a "-C > > > clock" for the guest, that should take precedence over the host clock > > > set. That is, a user may specifically state that they are using a > > > different clock. If we have frequency and offset set, it should still > > > work with different clocks. > > > > The guest config string is not parsed in the host context, that's why the host > > doesn't know if a guest has an explicit "-C clock" argument. > > I can parse the guest config here, but this will complicate the implementation. > > Using the current approach still guarantees that the user specified > > config has higher > > priority than injected one - add_argv() API prepends to the beginning > > of the string, so > > user arguments are always after the injected one. When guest parses > > the string, in case of > > duplicated "-C clock" arguments, the last one wins. > > I'm confused. I'm looking at this: > > > for (;;) { > [..] > switch (c) { > [..] > case 'A': > [..] > ctx->instance->flags |= BUFFER_FL_GUEST; > [..] > case 'C': > ctx->instance->ftrace->clock = optarg; > guest_config = true; > break; > [..] > } > [..] > } > [..] > if (guest_config) { > /* If -C is specified, prepend clock to all guest VM flags */ > for_all_instances(instance) { > if (top_instance.ftrace->clock) { > > Why can't we have here: > > if (top_instance.ftrace->clock && !instance->ftrace->clock) > > If the guest instance was given a -C, I would think we don't want to add > another -C to pass to that guest? > In case of a guest ("-A" option), the logic skips the switch(), so the guest args are not parsed. There is a check, right before the switch() : /* * If the current instance is to record a guest, then save * all the arguments for this instance. */ if (c != 'B' && c != 'A' && is_guest(ctx->instance)) { add_arg(ctx->instance, c, opts, long_options, optarg); continue; } I can put inside that if() a check for "-C" guest argument, but it will look like a hack. The confusion is that guest_config is set to true for any "-C" host argument, including those for instances, but only the one from the top instance is used to inject guest clock arg. > -- Steve > > if (is_guest(instance)) { > add_argv(instance, > (char *)top_instance.ftrace->clock, > true); > add_argv(instance, "-C", true); > } > } > } > }
On Wed, 11 Dec 2019 10:21:44 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > In case of a guest ("-A" option), the logic skips the switch(), so the > guest args are not parsed. > There is a check, right before the switch() : Ah, thanks, I forgot about that. > > /* > * If the current instance is to record a guest, then save > * all the arguments for this instance. > */ > if (c != 'B' && c != 'A' && is_guest(ctx->instance)) { > add_arg(ctx->instance, c, opts, long_options, optarg); > continue; > } > > I can put inside that if() a check for "-C" guest argument, but it > will look like a hack. I disagree. It's no more of a "hack" than appending a "-C" to the arguments. I think it's the right solution. We can simply add: if (c == 'C') ctx->instance->flags |= BUFFER_FL_HAS_CLOCK; Then we could test if that flag is set for the instance below. -- Steve > > The confusion is that guest_config is set to true for any "-C" host argument, > including those for instances, but only the one from the top instance > is used to inject > guest clock arg. > > > > -- Steve > > > > if (is_guest(instance)) { > > add_argv(instance, > > (char *)top_instance.ftrace->clock, > > true); > > add_argv(instance, "-C", true); > > } > > } > > } > > } > > >
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index 49730d6..e7fb1bd 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -5405,6 +5405,8 @@ static void parse_record_options(int argc, char *sav; int name_counter = 0; int neg_event = 0; + struct buffer_instance *instance; + bool guest_config = false; init_common_record_context(ctx, curr_cmd); @@ -5562,6 +5564,7 @@ static void parse_record_options(int argc, break; case 'C': ctx->instance->ftrace->clock = optarg; + guest_config = true; break; case 'v': neg_event = 1; @@ -5779,14 +5782,26 @@ static void parse_record_options(int argc, /* If --date is specified, prepend it to all guest VM flags */ if (ctx->date) { - struct buffer_instance *instance; - for_all_instances(instance) { if (is_guest(instance)) add_argv(instance, "--date", true); } } + if (guest_config) { + /* If -C is specified, prepend clock to all guest VM flags */ + for_all_instances(instance) { + if (top_instance.ftrace->clock) { + if (is_guest(instance)) { + add_argv(instance, + (char *)top_instance.ftrace->clock, + true); + add_argv(instance, "-C", true); + } + } + } + } + if (!ctx->filtered && ctx->instance->filter_mod) add_func(&ctx->instance->filter_funcs, ctx->instance->filter_mod, "*");
When tracing host and guest machines, both should use the same tracing clock for event timestamps. If a clock is specified as host tracing argument, with option "-C clock_name", the same is injected as guest tracing argument. If the user wants to use different tracing clocks, it can specify it using "-C clock_name" as guest tracing argument. In that case, the one specified by the user has higher priority. Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- tracecmd/trace-record.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)