[RFC] Open multiple trace files and, if possible, pair them

[RFC] Open multiple trace files and, if possible, pair them

[Stefano De Venuto]

The goal of this patch is to teach to trace-cmd how to open multiple
files and, if they are generated by an host and some guests, pair and
synchronize them.

We do that by checking, for every input file, whether or not
it has a corresponding peer, and if that's the case pairs them.

We know that the code must be changed following the new API, but we
would like to have an opinion on the work done so far.

Signed-off-by: Stefano De Venuto <stefano.devenuto99@gmail.com>
---
 tracecmd/trace-read.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index ce07b6b..734c2e7 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -1191,6 +1191,39 @@ enum output_type {
 	OUTPUT_VERSION_ONLY,
 };
 
+static void pair_host_guest(struct list_head *handle_list,
+				struct tracecmd_input *peer_handle)
+{
+	unsigned long long current_trace_id;
+	unsigned long long peer_trace_id;
+	struct handle_list *handles;
+
+	peer_trace_id = tracecmd_get_traceid(peer_handle);
+
+	list_for_each_entry(handles, handle_list, list) {
+
+		current_trace_id = tracecmd_get_traceid(handles->handle);
+
+		/* If the current handle represents a Host */
+		if (tracecmd_get_guest_cpumap(handles->handle, peer_trace_id,
+						NULL, NULL, NULL) != -1) {
+
+			current_trace_id = tracecmd_get_traceid(handles->handle);
+			if (tracecmd_get_tsync_peer(peer_handle) == current_trace_id)
+				tracecmd_pair_peer(peer_handle, handles->handle);
+
+		/* If the current handle represents a Guest */
+		} else if (tracecmd_get_tsync_peer(handles->handle) == peer_trace_id) {
+
+			current_trace_id = tracecmd_get_traceid(handles->handle);
+			if (tracecmd_get_guest_cpumap(peer_handle, current_trace_id,
+							NULL, NULL, NULL) != -1)
+				tracecmd_pair_peer(handles->handle, peer_handle);
+
+		}
+	}
+}
+
 static void read_data_info(struct list_head *handle_list, enum output_type otype,
 			   int global)
 {
@@ -1837,6 +1870,8 @@ void trace_report (int argc, char **argv)
 				return;
 		}
 
+		pair_host_guest(&handle_list, handle);
+
 		if (show_funcs) {
 			tep_print_funcs(pevent);
 			return;
-- 
2.30.0

Re: [RFC] Open multiple trace files and, if possible, pair them

[raistlin.df]

Hi Stefano!

Great to see your patch here on the list! :-)

On Fri, 2021-03-05 at 07:28 +0100, Stefano De Venuto wrote:
> The goal of this patch is to teach to trace-cmd how to open multiple
> files and, if they are generated by an host and some guests, pair and
> synchronize them.
> 
> We do that by checking, for every input file, whether or not
> it has a corresponding peer, and if that's the case pairs them.
> 
Exactly. So, for the trace-devel community: our (Stefano's and mine
:-D) end goal is to do some analysis & manipulation on a combined
host+guest(s) trace file. Of course, in order to do that, we need a
nice and easy way to get such a trace file!

Implementing the support for that in trace-cmd seemed the most
reasonable first step, so we reached out and decided to work on
https://bugzilla.kernel.org/show_bug.cgi?id=211657 , which is what this
patch does.

Now ...

> We know that the code must be changed following the new API, but we
> would like to have an opinion on the work done so far.
> 
... the API is changing, so we'll have to adapt it.

Still, the patch includes, e.g., the logic for opening multiple trace
files and figuring out whether they are guest or host, etc.

And maybe these bits are still valid and you're able/willing to give us
some feedback on whether they make sense? :-)

> Signed-off-by: Stefano De Venuto <stefano.devenuto99@gmail.com>
> ---
>  tracecmd/trace-read.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
For Stefano: about the patch itself, I think the only thing I'm going
to say is that some of the lines look a bit long.

I do not see a CODING_STYLE file in the repo, but just looking around,
I got the impression that we should stay within 76-80 characters all
the times that it is possible.

And that's it, and I'm happy to let the experts speak, as I'm only
trying to learn trace-cmd internals myself. ;-D

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Re: [RFC] Open multiple trace files and, if possible, pair them

[Steven Rostedt]

On Fri, 05 Mar 2021 10:18:07 +0100
raistlin.df@gmail.com wrote:

> For Stefano: about the patch itself, I think the only thing I'm going
> to say is that some of the lines look a bit long.
> 
> I do not see a CODING_STYLE file in the repo, but just looking around,
> I got the impression that we should stay within 76-80 characters all
> the times that it is possible.

Honestly, I'm not a fan of the 80 character limit. I don't want lines that
are 200 characters long either. My windows are normally between 90 and 100
characters wide, so keeping it under 100 is fine with me. Especially if it
keeps the code looking better.

For example:


+			current_trace_id = tracecmd_get_traceid(handles->handle);
+			if (tracecmd_get_guest_cpumap(peer_handle, current_trace_id,
+							NULL, NULL, NULL) != -1)
+				tracecmd_pair_peer(handles->handle, peer_handle);


Should be either:

			current_trace_id = tracecmd_get_traceid(handles->handle);
			if (tracecmd_get_guest_cpumap(peer_handle, current_trace_id, NULL, NULL, NULL) != -1)
				tracecmd_pair_peer(handles->handle, peer_handle);


or:

			current_trace_id = tracecmd_get_traceid(handles->handle);
			if (tracecmd_get_guest_cpumap(peer_handle, current_trace_id,
						      NULL, NULL, NULL) != -1) {
				tracecmd_pair_peer(handles->handle, peer_handle);
			}

Two changes above:

If you break a if conditional up, that categorizes it as a "complex"
statement and brackets should be used. (This is also for Linux kernel
coding style).

The second is, if you break up a function call parameters, the parameters
starting on the next line should line up with the starting parenthesis.
Note, this is not always the case. If the broken line is still long, it is
OK to have the line start before the parenthesis to keep it symmetrical:

			current_trace_id = tracecmd_get_traceid(handles->handle);
			if (tracecmd_get_guest_cpumap(peer_handle, current_trace_id,
					some_string, another_string, bigger_string_as_well) != -1) {
				tracecmd_pair_peer(handles->handle, peer_handle);
			}

But that too should be avoided if possible.

Thanks!

-- Steve

Re: [RFC] Open multiple trace files and, if possible, pair them

[Tzvetomir Stoyanov]

Hi Stefano,

On Fri, Mar 5, 2021 at 8:28 AM Stefano De Venuto
<stefano.devenuto99@gmail.com> wrote:
>
> The goal of this patch is to teach to trace-cmd how to open multiple
> files and, if they are generated by an host and some guests, pair and
> synchronize them.
>
> We do that by checking, for every input file, whether or not
> it has a corresponding peer, and if that's the case pairs them.
>
> We know that the code must be changed following the new API, but we
> would like to have an opinion on the work done so far.

Thanks for sending this patch!
This was exactly the missing functionality
in order to display host-guest trace files using the not-so-old trace-cmd
logic. I have only a few minor coding style comments and may be some
optimizations, but the patch looks good to me in general. If we haven't
decided to change the default behaviour of applying these TS corrections,
your fixes would be merged upstream.

>
> Signed-off-by: Stefano De Venuto <stefano.devenuto99@gmail.com>
> ---
>  tracecmd/trace-read.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
> index ce07b6b..734c2e7 100644
> --- a/tracecmd/trace-read.c
> +++ b/tracecmd/trace-read.c
> @@ -1191,6 +1191,39 @@ enum output_type {
>         OUTPUT_VERSION_ONLY,
>  };
>
> +static void pair_host_guest(struct list_head *handle_list,
> +                               struct tracecmd_input *peer_handle)
> +{
> +       unsigned long long current_trace_id;
> +       unsigned long long peer_trace_id;
> +       struct handle_list *handles;
> +
> +       peer_trace_id = tracecmd_get_traceid(peer_handle);
> +
> +       list_for_each_entry(handles, handle_list, list) {
> +
> +               current_trace_id = tracecmd_get_traceid(handles->handle);
> +
> +               /* If the current handle represents a Host */
> +               if (tracecmd_get_guest_cpumap(handles->handle, peer_trace_id,
> +                                               NULL, NULL, NULL) != -1) {
> +
> +                       current_trace_id = tracecmd_get_traceid(handles->handle);
> +                       if (tracecmd_get_tsync_peer(peer_handle) == current_trace_id)
> +                               tracecmd_pair_peer(peer_handle, handles->handle);
> +
> +               /* If the current handle represents a Guest */
> +               } else if (tracecmd_get_tsync_peer(handles->handle) == peer_trace_id) {
> +
> +                       current_trace_id = tracecmd_get_traceid(handles->handle);
> +                       if (tracecmd_get_guest_cpumap(peer_handle, current_trace_id,
> +                                                       NULL, NULL, NULL) != -1)
> +                               tracecmd_pair_peer(handles->handle, peer_handle);
> +
> +               }
> +       }
> +}
> +
>  static void read_data_info(struct list_head *handle_list, enum output_type otype,
>                            int global)
>  {
> @@ -1837,6 +1870,8 @@ void trace_report (int argc, char **argv)
>                                 return;
>                 }
>
> +               pair_host_guest(&handle_list, handle);
> +
>                 if (show_funcs) {
>                         tep_print_funcs(pevent);
>                         return;
> --
> 2.30.0
>


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [RFC] Open multiple trace files and, if possible, pair them

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

Hey everyone,

Sorry for the late reply...

On Fri, 2021-03-05 at 15:50 +0200, Tzvetomir Stoyanov wrote:
> Hi Stefano,
> 
> On Fri, Mar 5, 2021 at 8:28 AM Stefano De Venuto
> <stefano.devenuto99@gmail.com> wrote:
> > 
> > The goal of this patch is to teach to trace-cmd how to open
> > multiple
> > files and, if they are generated by an host and some guests, pair
> > and
> > synchronize them.
> > 
> 
> Thanks for sending this patch!
>
Thanks to you for the quick feedback!

> If we haven't
> decided to change the default behaviour of applying these TS
> corrections,
> your fixes would be merged upstream.
> 
Yeah, great timing on our side! :-D

So, what would be the way forward here. I'm guessing re-doing the patch
on top of the new API?

If yes, Stefano, are you up for having a look at how that could be
done?

Thanks again and Regards,
Dario
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] Open multiple trace files and, if possible, pair them

[Steven Rostedt]

On Thu, 11 Mar 2021 18:37:03 +0100
Dario Faggioli <dfaggioli@suse.com> wrote:

> 
> So, what would be the way forward here. I'm guessing re-doing the patch
> on top of the new API?

With the new API it may not be necessary.

As we are making it where the guest files will show the same output
regardless of being paired or not by default. That is, we are going to
store the synchronization information in each trace.dat file (for the host
and all the guests), and the trace.dat files will display the same time
stamp regardless of if it is being displayed with other trace.dat files or
stand alone.

As trace-cmd already merges by time stamp of multiple trace files, I'm not
sure there would be anything more to do once we implement the new API.

-- Steve

Re: [RFC] Open multiple trace files and, if possible, pair them

[Steven Rostedt]

On Thu, 11 Mar 2021 12:50:40 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> As trace-cmd already merges by time stamp of multiple trace files, I'm not
> sure there would be anything more to do once we implement the new API.

That said, we are hoping to have the new API out shortly, and it would be
fantastic if you could start testing it!

Thanks,

-- Steve

Re: [RFC] Open multiple trace files and, if possible, pair them

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On Thu, 2021-03-11 at 12:50 -0500, Steven Rostedt wrote:
> On Thu, 11 Mar 2021 18:37:03 +0100
> Dario Faggioli <dfaggioli@suse.com> wrote:
> > 
> > So, what would be the way forward here. I'm guessing re-doing the
> > patch
> > on top of the new API?
> 
> With the new API it may not be necessary.
> 
Right.

> As we are making it where the guest files will show the same output
> regardless of being paired or not by default. That is, we are going
> to
> store the synchronization information in each trace.dat file (for the
> host
> and all the guests), and the trace.dat files will display the same
> time
> stamp regardless of if it is being displayed with other trace.dat
> files or
> stand alone.
> 
Yep, saw it.

In fact, my mistake was to think that opening multiple trace files
would still need some work. But, as a matter of fact, that also works
already, right?

In which case, we will "just" apply the patches with the new API and
continue our work on top of them.

And sorry for the noise. :-)

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]