| Message ID | 20210305062807.2734-1-stefano.devenuto99@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] Open multiple trace files and, if possible, pair them | expand |
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
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 >
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
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
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
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
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
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;
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(+)