On Mon, Apr 26, 2021 at 2:44 PM Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
>
> Hi Tzvetomir, Thanks for the patch. I replied below:
>
> On Mon, Apr 26, 2021 at 6:35 AM Tzvetomir Stoyanov (VMware)
> <tz.stoyanov@xxxxxxxxx> wrote:
> >
> > Current host-guest tracing implementation assumes that qemu runs the VMs
> > and uses qemu specific logic to collect various information:
> > - PID of the process, running the guest VM. In case of KVM, this PID
> > is used to get the KVM guest TSC clock parameters, needed for better
> > host and guest trace timestamps synchronization.
> > - PIDs of each thread, running a guest virtual CPU. This is used for
> > better trace visualisation. It helps to map the host task to a vCPU
> > and to visualise them together.
> > In case qemu is not used to run the VMs, host-guest tracing fails. As
> > that information is not mandatory, we can easily support non-qemu VMs.
> > Changes, proposed by the patch:
> > - if PID of the process, running the guest VM, is not available - fail
> > back to a PTP-like algorithm for trace timestamps synchronization.
> > - if PIDs of the threads, running guest virtual CPUs, are not available
> > write -1 in the trace file metadata.
>
> This does not seem to be working still.
Turns out I did not rebuild properly. The PTP sync is working fine on
crosvm and I can indeed see that the vCPU events are time-synced with
host ones.
Tested-By: Joel Fernandes <joelaf@xxxxxxxxxx>
thanks,
- Joel
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> > ---
> > tracecmd/include/trace-local.h | 4 +--
> > tracecmd/trace-record.c | 36 ++++++++++++-------------
> > tracecmd/trace-vm.c | 48 ++++++++++++++++++++++++++++++++--
> > 3 files changed, 66 insertions(+), 22 deletions(-)
> >
> > diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
> > index 1218de12..04d8f32c 100644
> > --- a/tracecmd/include/trace-local.h
> > +++ b/tracecmd/include/trace-local.h
> > @@ -301,8 +301,8 @@ struct trace_guest {
> > int cpu_max;
> > int *cpu_pid;
> > };
> > -struct trace_guest *get_guest_by_cid(unsigned int guest_cid);
> > -struct trace_guest *get_guest_by_name(char *name);
> > +struct trace_guest *trace_get_guest(unsigned int cid, const char *name);
> > +bool trace_have_guests_pid(void);
> > void read_qemu_guests(void);
> > int get_guest_pid(unsigned int guest_cid);
> > int get_guest_vcpu_pid(unsigned int guest_cid, unsigned int guest_vcpu);
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index fd03a605..4636475f 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -3251,10 +3251,7 @@ static char *parse_guest_name(char *gname, int *cid, int *port)
> > *cid = atoi(gname);
> >
> > read_qemu_guests();
> > - if (*cid > 0)
> > - guest = get_guest_by_cid(*cid);
> > - else
> > - guest = get_guest_by_name(gname);
> > + guest = trace_get_guest(*cid, gname);
> > if (guest) {
> > *cid = guest->cid;
> > return guest->name;
> > @@ -3723,14 +3720,14 @@ static int host_tsync(struct common_record_context *ctx,
> >
> > if (!proto)
> > return -1;
> > - guest = get_guest_by_cid(instance->cid);
> > + guest = trace_get_guest(instance->cid, NULL);
> > if (guest == NULL)
> > return -1;
> >
> > instance->tsync = tracecmd_tsync_with_guest(top_instance.trace_id,
> > instance->tsync_loop_interval,
> > instance->cid, tsync_port,
> > - guest->pid, guest->cpu_max,
> > + guest->pid, instance->cpu_count,
> > proto, ctx->clock);
> > if (!instance->tsync)
> > return -1;
> > @@ -3792,6 +3789,7 @@ static void connect_to_agent(struct common_record_context *ctx,
> > printf("Negotiated %s time sync protocol with guest %s\n",
> > tsync_protos_reply,
> > instance->name);
> > + instance->cpu_count = nr_cpus;
> > host_tsync(ctx, instance, tsync_port, tsync_protos_reply);
> > } else
> > warning("Failed to negotiate timestamps synchronization with the guest");
> > @@ -3975,21 +3973,19 @@ static void append_buffer(struct tracecmd_output *handle,
> > static void
> > add_guest_info(struct tracecmd_output *handle, struct buffer_instance *instance)
> > {
> > - struct trace_guest *guest = get_guest_by_cid(instance->cid);
> > + struct trace_guest *guest = trace_get_guest(instance->cid, NULL);
> > char *buf, *p;
> > int size;
> > + int pid;
> > int i;
> >
> > if (!guest)
> > return;
> > - for (i = 0; i < guest->cpu_max; i++)
> > - if (!guest->cpu_pid[i])
> > - break;
> >
> > size = strlen(guest->name) + 1;
> > - size += sizeof(long long); /* trace_id */
> > - size += sizeof(int); /* cpu count */
> > - size += i * 2 * sizeof(int); /* cpu,pid pair */
> > + size += sizeof(long long); /* trace_id */
> > + size += sizeof(int); /* cpu count */
> > + size += instance->cpu_count * 2 * sizeof(int); /* cpu,pid pair */
> >
> > buf = calloc(1, size);
> > if (!buf)
> > @@ -4001,14 +3997,16 @@ add_guest_info(struct tracecmd_output *handle, struct buffer_instance *instance)
> > memcpy(p, &instance->trace_id, sizeof(long long));
> > p += sizeof(long long);
> >
> > - memcpy(p, &i, sizeof(int));
> > + memcpy(p, &instance->cpu_count, sizeof(int));
> > p += sizeof(int);
> > - for (i = 0; i < guest->cpu_max; i++) {
> > - if (!guest->cpu_pid[i])
> > - break;
> > + for (i = 0; i < instance->cpu_count; i++) {
> > + if (i < guest->cpu_max)
> > + pid = guest->cpu_pid[i];
> > + else
> > + pid = -1;
> > memcpy(p, &i, sizeof(int));
> > p += sizeof(int);
> > - memcpy(p, &guest->cpu_pid[i], sizeof(int));
> > + memcpy(p, &pid, sizeof(int));
> > p += sizeof(int);
> > }
> >
> > @@ -6553,12 +6551,14 @@ static void set_tsync_params(struct common_record_context *ctx)
> > /*
> > * If no clock is configured &&
> > * KVM time sync protocol is available &&
> > + * there is information of each guest PID process &&
> > * tsc-x86 clock is supported &&
> > * 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 (tsync_proto_is_supported("kvm") &&
> > + trace_have_guests_pid() &&
> > clock_is_supported(NULL, TSC_CLOCK) &&
> > !get_tsc_nsec(&shift, &mult) && mult) {
> > clock = strdup(TSC_CLOCK);
> > diff --git a/tracecmd/trace-vm.c b/tracecmd/trace-vm.c
> > index c8924ece..c0333b67 100644
> > --- a/tracecmd/trace-vm.c
> > +++ b/tracecmd/trace-vm.c
> > @@ -35,7 +35,7 @@ static int set_vcpu_pid_mapping(struct trace_guest *guest, int cpu, int pid)
> > return 0;
> > }
> >
> > -struct trace_guest *get_guest_by_cid(unsigned int guest_cid)
> > +static struct trace_guest *get_guest_by_cid(unsigned int guest_cid)
> > {
> > int i;
> >
> > @@ -48,7 +48,7 @@ struct trace_guest *get_guest_by_cid(unsigned int guest_cid)
> > return NULL;
> > }
> >
> > -struct trace_guest *get_guest_by_name(char *name)
> > +static struct trace_guest *get_guest_by_name(const char *name)
> > {
> > int i;
> >
> > @@ -61,6 +61,50 @@ struct trace_guest *get_guest_by_name(char *name)
> > return NULL;
> > }
> >
> > +bool trace_have_guests_pid(void)
> > +{
> > + for (int i = 0; i < guests_len; i++) {
> > + if (guests[i].pid < 0)
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static struct trace_guest *add_guest(unsigned int cid, const char *name)
> > +{
> > + guests = realloc(guests, (guests_len + 1) * sizeof(*guests));
> > + if (!guests)
> > + die("allocating new guest");
> > + memset(&guests[guests_len], 0, sizeof(struct trace_guest));
> > + guests[guests_len].name = strdup(name);
> > + if (!guests[guests_len].name)
> > + die("allocating guest name");
> > + guests[guests_len].cid = cid;
> > + guests[guests_len].pid = -1;
> > + guests_len++;
> > +
> > + return &guests[guests_len - 1];
> > +}
> > +
> > +struct trace_guest *trace_get_guest(unsigned int cid, const char *name)
> > +{
> > + struct trace_guest *guest = NULL;
> > +
> > + if (name) {
> > + guest = get_guest_by_name(name);
> > + if (guest)
> > + return guest;
> > + }
> > +
> > + if (cid > 0) {
> > + guest = get_guest_by_cid(cid);
> > + if (!guest && name)
> > + guest = add_guest(cid, name);
> > + }
> > + return guest;
> > +}
> > +
> > static char *get_qemu_guest_name(char *arg)
> > {
> > char *tok, *end = arg;
> > --
> > 2.30.2
> >
|