On Wed, Mar 17, 2021 at 11:49 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Wed, 17 Mar 2021 11:57:31 +0200
> Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> 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 ;-)
According to the code, trace-cmd supports clock per instance. And it
works, I tried this:
#trace-cmd record -C x86-tsc -e irq -B foo -C local -e sched sleep 1
and got different clocks:
<...>-290676 [006]1956659520918: softirq_exit: ....
foo: zoom-10894 [004]112198623756671: sched_switch: ....
although I cannot imagine a use case for this. It will be weird to
visualise this by KerneShark.
I think that we should introduce options per instance now, as "-C
tsc2nsec" cannot be implemented without it.
I see two possible ways to implement it:
1. Break the current trace.dat file format by adding metadata for
each instance.
2. Keep the current format and use nested options - that's it, just
another option "OPTION_INSTANCE". It will hold instance name and all
options, specific for that instance, using the current options format.
But there is one question, valid for both ways: What should be the
scope for the global options ? Apply them only on the top instance, or
they should be common for all instances ?
>
> 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
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|