Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] trace-cmd: Add support for non-qemu VMs
@ 2021-04-26 10:35 Tzvetomir Stoyanov (VMware)
  2021-04-26 18:44 ` Joel Fernandes
  2021-05-04 21:23 ` Steven Rostedt
  0 siblings, 2 replies; 6+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2021-04-26 10:35 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, joelaf

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.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] trace-cmd: Add support for non-qemu VMs
  2021-04-26 10:35 [PATCH] trace-cmd: Add support for non-qemu VMs Tzvetomir Stoyanov (VMware)
@ 2021-04-26 18:44 ` Joel Fernandes
  2021-04-27  1:19   ` Joel Fernandes
  2021-04-27  8:27   ` Yordan Karadzhov
  2021-05-04 21:23 ` Steven Rostedt
  1 sibling, 2 replies; 6+ messages in thread
From: Joel Fernandes @ 2021-04-26 18:44 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: Steven Rostedt, Linux Trace Devel

Hi Tzvetomir, Thanks for the patch. I replied below:

On Mon, Apr 26, 2021 at 6:35 AM Tzvetomir Stoyanov (VMware)
<tz.stoyanov@gmail.com> 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. I tried it twice and double
checked that trace-cmd picked up your patch (I did not update
KernelShark after applying your patch, just updated trace-cmd).

In the below drive link , I saved the .dat files and also share the
KernelShark screenshot which still asks me for an offset in a pop up
window. If I enter 0, I get an out of sync trace.

I uploaded the trace files here:
https://drive.google.com/drive/folders/1t4yEElcWzjfTtt2F2Nvst1BD5Xr4mlVq?usp=sharing

Let me know if you want me to try something.

-Joel



>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] trace-cmd: Add support for non-qemu VMs
  2021-04-26 18:44 ` Joel Fernandes
@ 2021-04-27  1:19   ` Joel Fernandes
  2021-04-27  8:27   ` Yordan Karadzhov
  1 sibling, 0 replies; 6+ messages in thread
From: Joel Fernandes @ 2021-04-27  1:19 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: Steven Rostedt, Linux Trace Devel

On Mon, Apr 26, 2021 at 2:44 PM Joel Fernandes <joelaf@google.com> wrote:
>
> Hi Tzvetomir, Thanks for the patch. I replied below:
>
> On Mon, Apr 26, 2021 at 6:35 AM Tzvetomir Stoyanov (VMware)
> <tz.stoyanov@gmail.com> 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@google.com>

thanks,

- Joel


> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  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
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] trace-cmd: Add support for non-qemu VMs
  2021-04-26 18:44 ` Joel Fernandes
  2021-04-27  1:19   ` Joel Fernandes
@ 2021-04-27  8:27   ` Yordan Karadzhov
  2021-04-27 15:34     ` Joel Fernandes
  1 sibling, 1 reply; 6+ messages in thread
From: Yordan Karadzhov @ 2021-04-27  8:27 UTC (permalink / raw)
  To: Joel Fernandes, Tzvetomir Stoyanov (VMware)
  Cc: Steven Rostedt, Linux Trace Devel

Hi all,

On 26.04.21 г. 21:44, Joel Fernandes wrote:
> In the below drive link , I saved the .dat files and also share the
> KernelShark screenshot which still asks me for an offset in a pop up
> window.

This pop up dialog that asks for the offset is a legacy from the time 
when we have been been doing the calculation of the offset by hand. It 
still can be useful in some specific cases, but the user can do the same 
from the "Tools" menu. It may be better if we disable the automatic 
pop-up when a data file is appended. What do you think?

Thanks!
Yordan


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] trace-cmd: Add support for non-qemu VMs
  2021-04-27  8:27   ` Yordan Karadzhov
@ 2021-04-27 15:34     ` Joel Fernandes
  0 siblings, 0 replies; 6+ messages in thread
From: Joel Fernandes @ 2021-04-27 15:34 UTC (permalink / raw)
  To: Yordan Karadzhov
  Cc: Tzvetomir Stoyanov (VMware), Steven Rostedt, Linux Trace Devel

On Tue, Apr 27, 2021 at 4:27 AM Yordan Karadzhov <y.karadz@gmail.com> wrote:
>
> Hi all,
>
> On 26.04.21 г. 21:44, Joel Fernandes wrote:
> > In the below drive link , I saved the .dat files and also share the
> > KernelShark screenshot which still asks me for an offset in a pop up
> > window.
>
> This pop up dialog that asks for the offset is a legacy from the time
> when we have been been doing the calculation of the offset by hand. It
> still can be useful in some specific cases, but the user can do the same
> from the "Tools" menu. It may be better if we disable the automatic
> pop-up when a data file is appended. What do you think?

This makes sense to me. The pop up is confusing. Perhaps you can make
it a menu option if someone wants to inject an offset after the fact.

-Joel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] trace-cmd: Add support for non-qemu VMs
  2021-04-26 10:35 [PATCH] trace-cmd: Add support for non-qemu VMs Tzvetomir Stoyanov (VMware)
  2021-04-26 18:44 ` Joel Fernandes
@ 2021-05-04 21:23 ` Steven Rostedt
  1 sibling, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2021-05-04 21:23 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: linux-trace-devel, joelaf

On Mon, 26 Apr 2021 13:35:28 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> 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.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---

We need to fix this for qemu as well. I just upgraded my test box to
Fedora-33 and it renamed qemu from qemu-system-x86 to qemu-kvm, and I hit
the same issue that Joel did with his code. And this patch just removes the
use of KVM synchronization for my box.

I'll try to find other ways to get this information than the hack we
currently have. Relying on the application name is not robust at all. But
unfortunately, there's currently no way that I can find that exports the
CID of tasks. Perhaps this is going to require a kernel change. To have a
netstat functionality for vsockets.

-- Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-26 10:35 [PATCH] trace-cmd: Add support for non-qemu VMs Tzvetomir Stoyanov (VMware)
2021-04-26 18:44 ` Joel Fernandes
2021-04-27  1:19   ` Joel Fernandes
2021-04-27  8:27   ` Yordan Karadzhov
2021-04-27 15:34     ` Joel Fernandes
2021-05-04 21:23 ` Steven Rostedt

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git