| Message ID | 20210324130418.436206-22-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TSC trace clock to nanosecond conversion | expand |
On Wed, 24 Mar 2021 15:04:16 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > In host-guest tracing session, all peers should use the same tracing > clock. If there is no user configured trace clock, the current logic > assumes "local" clock for the session. This could be wrong, as other > clock than "local" could be already configured on the host, before > running trace-cmd. The default clock for host-guest tracing session > should be rertieved from the host's "trace_clock" file. Actually this is wrong. If clock is not specified, you should check the first instance, because if the user does: # trace-cmd start -B foo -p nop -C myclock # trace-cmd record -B foo -e kvm -e sched -A Guest -e all It should not use the clock from the top instance, but instead the first instance. I'm trying to make sure tha trace-cmd does not affect or even use the top instance if it is not part of the command line. > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > tracecmd/trace-record.c | 40 ++++++++++++++++++++++++++++++---------- > 1 file changed, 30 insertions(+), 10 deletions(-) > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index f90fdbe4..2fc6723a 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -6455,11 +6455,12 @@ static void get_tsc_offset(struct common_record_context *ctx) > > static void set_tsync_params(struct common_record_context *ctx) > { > - const char *clock = ctx->clock; > struct buffer_instance *instance; > int shift, mult; > bool force_tsc = false; > + char *clock = NULL; > > + if (!ctx->clock) { > /* > * If no clock is configured && > * KVM time sync protocol is available && > @@ -6468,18 +6469,35 @@ static void set_tsync_params(struct common_record_context *ctx) > * force using the x86-tsc clock for this host-guest tracing session > * and store TSC to nsec multiplier and shift. > */ > - if (!clock && tsync_proto_is_supported("kvm") && > - clock_is_supported(NULL, TSC_CLOCK) && > - !get_tsc_nsec(&shift, &mult) && mult) { > - clock = TSC_CLOCK; > - ctx->tsc2nsec.mult = mult; > - ctx->tsc2nsec.shift = shift; > - ctx->tsc2nsec.offset = get_clock_now(TSC_CLOCK); > - force_tsc = true; > + if (tsync_proto_is_supported("kvm") && > + clock_is_supported(NULL, TSC_CLOCK) && So we want to test the first instance, not the top one. -- Steve > + !get_tsc_nsec(&shift, &mult) && mult) { > + clock = strdup(TSC_CLOCK); > + if (!clock) > + die("Cannot not allocate clock"); > + ctx->tsc2nsec.mult = mult; > + ctx->tsc2nsec.shift = shift; > + ctx->tsc2nsec.offset = get_clock_now(TSC_CLOCK); > + force_tsc = true; > + } else { > + /* > + * Else, use the current clock of the first host instance > + */ > + for_all_instances(instance) { > + if (is_guest(instance)) > + continue; > + clock = tracefs_get_clock(instance->tracefs); > + break; > + } > + } > + } else { > + clock = strdup(ctx->clock); > + if (!clock) > + die("Cannot not allocate clock"); > } > > if (!clock && !ctx->tsync_loop_interval) > - return; > + goto out; > for_all_instances(instance) { > if (clock && !(instance->flags & BUFFER_FL_HAS_CLOCK)) { > /* use the same clock in all tracing peers */ > @@ -6501,6 +6519,8 @@ static void set_tsync_params(struct common_record_context *ctx) > } > instance->tsync_loop_interval = ctx->tsync_loop_interval; > } > +out: > + free(clock); > } > > /*
On Wed, Mar 24, 2021 at 11:15 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 24 Mar 2021 15:04:16 +0200 > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > > In host-guest tracing session, all peers should use the same tracing > > clock. If there is no user configured trace clock, the current logic > > assumes "local" clock for the session. This could be wrong, as other > > clock than "local" could be already configured on the host, before > > running trace-cmd. The default clock for host-guest tracing session > > should be rertieved from the host's "trace_clock" file. > > Actually this is wrong. If clock is not specified, you should check the > first instance, because if the user does: > > # trace-cmd start -B foo -p nop -C myclock > > # trace-cmd record -B foo -e kvm -e sched -A Guest -e all > > It should not use the clock from the top instance, but instead the first > instance. Yes, the implementation uses the clock from the first non-guest instance, the description of the patch is not correct. > > I'm trying to make sure tha trace-cmd does not affect or even use the top > instance if it is not part of the command line. > > > > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > > --- > > tracecmd/trace-record.c | 40 ++++++++++++++++++++++++++++++---------- > > 1 file changed, 30 insertions(+), 10 deletions(-) > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index f90fdbe4..2fc6723a 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > @@ -6455,11 +6455,12 @@ static void get_tsc_offset(struct common_record_context *ctx) > > > > static void set_tsync_params(struct common_record_context *ctx) > > { > > - const char *clock = ctx->clock; > > struct buffer_instance *instance; > > int shift, mult; > > bool force_tsc = false; > > + char *clock = NULL; > > > > + if (!ctx->clock) { > > /* > > * If no clock is configured && > > * KVM time sync protocol is available && > > @@ -6468,18 +6469,35 @@ static void set_tsync_params(struct common_record_context *ctx) > > * force using the x86-tsc clock for this host-guest tracing session > > * and store TSC to nsec multiplier and shift. > > */ > > - if (!clock && tsync_proto_is_supported("kvm") && > > - clock_is_supported(NULL, TSC_CLOCK) && > > - !get_tsc_nsec(&shift, &mult) && mult) { > > - clock = TSC_CLOCK; > > - ctx->tsc2nsec.mult = mult; > > - ctx->tsc2nsec.shift = shift; > > - ctx->tsc2nsec.offset = get_clock_now(TSC_CLOCK); > > - force_tsc = true; > > + if (tsync_proto_is_supported("kvm") && > > + clock_is_supported(NULL, TSC_CLOCK) && > > So we want to test the first instance, not the top one. I assume that if the top instance supports "x86-tsc" clock, then any instance should support it also ? Is it possible that supported clocks can be different in each instance ? > > -- Steve > > > + !get_tsc_nsec(&shift, &mult) && mult) { > > + clock = strdup(TSC_CLOCK); > > + if (!clock) > > + die("Cannot not allocate clock"); > > + ctx->tsc2nsec.mult = mult; > > + ctx->tsc2nsec.shift = shift; > > + ctx->tsc2nsec.offset = get_clock_now(TSC_CLOCK); > > + force_tsc = true; > > + } else { > > + /* > > + * Else, use the current clock of the first host instance > > + */ > > + for_all_instances(instance) { > > + if (is_guest(instance)) > > + continue; > > + clock = tracefs_get_clock(instance->tracefs); > > + break; This is the loop that gets the clock from the first non-guest instance, in case there is no "-C ..." option in the command line. > > + } > > + } > > + } else { > > + clock = strdup(ctx->clock); > > + if (!clock) > > + die("Cannot not allocate clock"); > > } > > > > if (!clock && !ctx->tsync_loop_interval) > > - return; > > + goto out; > > for_all_instances(instance) { > > if (clock && !(instance->flags & BUFFER_FL_HAS_CLOCK)) { > > /* use the same clock in all tracing peers */ > > @@ -6501,6 +6519,8 @@ static void set_tsync_params(struct common_record_context *ctx) > > } > > instance->tsync_loop_interval = ctx->tsync_loop_interval; > > } > > +out: > > + free(clock); > > } > > > > /* >
On Thu, 25 Mar 2021 07:13:55 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > So we want to test the first instance, not the top one. > > I assume that if the top instance supports "x86-tsc" clock, then any > instance should support it also ? Is it possible that supported clocks > can be different in each instance ? Probably not, but I don't want to assume that is the case. It could be that the top instance supports more clocks than the created ones, if someone were to add some kind of magical clock. -- Steve
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index f90fdbe4..2fc6723a 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -6455,11 +6455,12 @@ static void get_tsc_offset(struct common_record_context *ctx) static void set_tsync_params(struct common_record_context *ctx) { - const char *clock = ctx->clock; struct buffer_instance *instance; int shift, mult; bool force_tsc = false; + char *clock = NULL; + if (!ctx->clock) { /* * If no clock is configured && * KVM time sync protocol is available && @@ -6468,18 +6469,35 @@ static void set_tsync_params(struct common_record_context *ctx) * force using the x86-tsc clock for this host-guest tracing session * and store TSC to nsec multiplier and shift. */ - if (!clock && tsync_proto_is_supported("kvm") && - clock_is_supported(NULL, TSC_CLOCK) && - !get_tsc_nsec(&shift, &mult) && mult) { - clock = TSC_CLOCK; - ctx->tsc2nsec.mult = mult; - ctx->tsc2nsec.shift = shift; - ctx->tsc2nsec.offset = get_clock_now(TSC_CLOCK); - force_tsc = true; + if (tsync_proto_is_supported("kvm") && + clock_is_supported(NULL, TSC_CLOCK) && + !get_tsc_nsec(&shift, &mult) && mult) { + clock = strdup(TSC_CLOCK); + if (!clock) + die("Cannot not allocate clock"); + ctx->tsc2nsec.mult = mult; + ctx->tsc2nsec.shift = shift; + ctx->tsc2nsec.offset = get_clock_now(TSC_CLOCK); + force_tsc = true; + } else { + /* + * Else, use the current clock of the first host instance + */ + for_all_instances(instance) { + if (is_guest(instance)) + continue; + clock = tracefs_get_clock(instance->tracefs); + break; + } + } + } else { + clock = strdup(ctx->clock); + if (!clock) + die("Cannot not allocate clock"); } if (!clock && !ctx->tsync_loop_interval) - return; + goto out; for_all_instances(instance) { if (clock && !(instance->flags & BUFFER_FL_HAS_CLOCK)) { /* use the same clock in all tracing peers */ @@ -6501,6 +6519,8 @@ static void set_tsync_params(struct common_record_context *ctx) } instance->tsync_loop_interval = ctx->tsync_loop_interval; } +out: + free(clock); } /*
In host-guest tracing session, all peers should use the same tracing clock. If there is no user configured trace clock, the current logic assumes "local" clock for the session. This could be wrong, as other clock than "local" could be already configured on the host, before running trace-cmd. The default clock for host-guest tracing session should be rertieved from the host's "trace_clock" file. Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- tracecmd/trace-record.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-)