Re: [PATCH 02/12] trace-cmd: Add logic for TSC to nanosecond conversion

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 17 Mar 2021 11:57:31 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > I would break this up into two patches. One that adds the perf mult shift
> > logic, and even allow users to use it! As TSC clocks are still faster than
> > the local clock (as the local clock needs to do the multiplier and shift at
> > every event while recording). It would be a feature to have this:
> >
> >  trace-cmd record --tsc2nsec ...  
> 
> As the tracing clock is per instance, does it mean this flag should
> also be per instance ? I think the below commands should be valid:
>    trace-cmd record --tsc2nsec -B foo -C mono  <-- use tsc-x86 clock +
> tsc2nsec for the top instance and mono clock for instance foo
>    trace-cmd record -C mono -B foo --tsc2nsec  <-- use mono clock for
> the top instance and tsc-x86 clock + tsc2nsec clock for instance foo
> or even more confusing:
>    trace-cmd record -C x86-tsc -B foo --tsc2nsec  <-- use tsc-x86
> clock for the top instance and tsc-x86 clock + tsc2nsec clock for
> instance foo

I just remembered that I previously discussed making the "tsc2nsec" into a
clock type from the perspective of the user (not the trace file). That is,
not to have --tsc2nsec, but instead to have:

 trace-cmd record -C tsc2nsec

And see if there's a tsc clock and the perf interface is available with the
multiplier and shift, and if not, it would return an error just like if you
said "-C foo". I also remember saying that we should have trace-cmd list
show it as well if it is available.

 # trace-cmd list -C
[local] global counter uptime perf mono mono_raw boot x86-tsc tsc2nsec

That is, tack on at the end " tsc2nsec" if we find that it is available.

> This will require changes in the trace.dat file format. The problem is
> that tsc2nsec multiplier, shift and offset are stored as an option in
> the trace.dat file. Options are global, not per instance. When reading
> the file, how to determine events in which instance should be affected
> by this option ? We should either add options per instance, or somehow
> encode the instance scope of each option.

Your right. And I just tried this:

 # trace-cmd record -C x86-tsc -e sched -B foo -C local -e sched sleep 1

And it appears to ignore the -C local. Which I think is the right thing to
do ;-)

Yeah, we should only support one clock for all instances. And keep this as
a global option. We may in the future have instance options, but that will
come as a separate "option" :-)


> > > +static void set_vsync_clock(void)
> > > +{
> > > +     const char *clock = top_instance.clock;
> > > +     struct buffer_instance *instance;
> > > +     bool tsc2nsec = false;
> > > +     int shift, mult, offset;
> > > +
> > > +     /*
> > > +      * If no clock is specified for the top trace instance AND
> > > +      * KVM time sync protocol is available AND
> > > +      * TSC to nsec multiplier and shift are available:
> > > +      * 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") &&
> > > +         !get_tsc_nsec(&shift, &mult, &offset) && mult) {
> > > +             top_instance.clock = strdup(TSC_CLOCK);
> > > +             if (!top_instance.clock)
> > > +                     die("Could not allocate top instance clock");
> > > +             clock = top_instance.clock;  
> >
> > Why is this using the top_instance? What if the user had done:
> >
> >  # trace-cmd record -B host -e kvm -e sched -A guest -e sched  
> 
> You are right, that command is valid. The problem is that guest
> timestamps can be synchronized with one clock only. If there are two
> instances on the host, each with a different clock, the
> synchronization will not work. That's why I look at the top instance
> clock only, which is wrong. I'll change that logic to take the first
> configured host trace clock, i.e. in case of multiple host instances
> with multiple trace clocks, the first wins.
> 


The code should just use the first instance available. Not about being
global, but because I want trace-cmd to not interfere with the top instance
if it is not specified on the command line. That way we can have this:

 # trace-cmd record -e all

in one window, and in another window:

 # trace-cmd record -B guest -e kvm -A guest -e all

and that recorder not do anything to bother the first one. That is, we
should only touch the top instance if it is explicitly specified.


-- Steve