| Message ID | 20240112083945.1361293-6-pierre.gondois@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd: split: Handle splitting files with multiple instances | expand |
On Fri, 12 Jan 2024 09:39:45 +0100 Pierre Gondois <pierre.gondois@arm.com> wrote: > trace-cmd can record events in multiple instances: > $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch > > When trying to split a trace.dat file recorded with the above command, > only the events located in the main buffer seems to be split. The > events recorded in the test_instance buffer seem to be discarded: > $ trace-cmd split -i trace.dat -o trace_2.dat 284443 284444 > $ trace-cmd report trace_2.dat > cpus=8 > <...>-525991 [004] 284443.173879: sched_wakeup: [...] > <...>-525991 [004] 284443.173879: sched_wakeup: [...] > <...>-525990 [007] 284443.173885: sched_wakeup: [...] > <...>-525990 [007] 284443.173885: sched_wakeup: [...] > (no sign of sched_switch events) > > Make use of the previous patches to split all the instances of > a trace. This shouldn't be in the change log. As the change log is for history. Think about reading this 5 years from now. Would it make sense about "previous patches"? Thanks for doing this, I'll try to set some time next week to review them. I want to release 3.3 soon. Would you be able to add a way to split out an instance into its own trace.dat file? Or to choose what you want. $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch $ trace-cmd split -B test_instance -o test.dat Would make test_instance the main buffer in test.dat. If you add more than one instance: $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch -B timer_instance -e timer $ trace-cmd split -B test_instance -o test.dat -B timer_instance This would make "test_instance" the main buffer, and keep "timer_instance" as an instance. The "-o file" placement is important. It will make whatever came before it the main buffer. And perhaps even split out more than one! $ trace-cmd split -B test_instance -o test.dat -B timer_instance -o timer.dat Would place the "test_instance" as the main instance in test.dat, and the "timer_instance" as the main instance in timer.dat. Thoughts? -- Steve > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218357 > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > --- >
Hello Steven, On 1/12/24 17:18, Steven Rostedt wrote: > On Fri, 12 Jan 2024 09:39:45 +0100 > Pierre Gondois <pierre.gondois@arm.com> wrote: > >> trace-cmd can record events in multiple instances: >> $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch >> >> When trying to split a trace.dat file recorded with the above command, >> only the events located in the main buffer seems to be split. The >> events recorded in the test_instance buffer seem to be discarded: >> $ trace-cmd split -i trace.dat -o trace_2.dat 284443 284444 >> $ trace-cmd report trace_2.dat >> cpus=8 >> <...>-525991 [004] 284443.173879: sched_wakeup: [...] >> <...>-525991 [004] 284443.173879: sched_wakeup: [...] >> <...>-525990 [007] 284443.173885: sched_wakeup: [...] >> <...>-525990 [007] 284443.173885: sched_wakeup: [...] >> (no sign of sched_switch events) >> > > >> Make use of the previous patches to split all the instances of >> a trace. > > This shouldn't be in the change log. As the change log is for history. > Think about reading this 5 years from now. Would it make sense about > "previous patches"? I thought it was acceptable to do so as the previous patches would be just before in the git log, but ok I will write something better. > > Thanks for doing this, I'll try to set some time next week to review them. > I want to release 3.3 soon. > > Would you be able to add a way to split out an instance into its own > trace.dat file? Or to choose what you want. > > $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch > > $ trace-cmd split -B test_instance -o test.dat > > Would make test_instance the main buffer in test.dat. > > If you add more than one instance: > > $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch -B timer_instance -e timer > > $ trace-cmd split -B test_instance -o test.dat -B timer_instance > > This would make "test_instance" the main buffer, and keep "timer_instance" > as an instance. > > The "-o file" placement is important. It will make whatever came before it > the main buffer. And perhaps even split out more than one! > > $ trace-cmd split -B test_instance -o test.dat -B timer_instance -o timer.dat > > Would place the "test_instance" as the main instance in test.dat, and the > "timer_instance" as the main instance in timer.dat. > > Thoughts? This should be possible, I will try to make an update in this direction, Regards, Pierre > > -- Steve > > > >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218357 >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> --- >>
On Mon, 15 Jan 2024 18:25:46 +0100 Pierre Gondois <pierre.gondois@arm.com> wrote: > > > >> Make use of the previous patches to split all the instances of > >> a trace. > > > > This shouldn't be in the change log. As the change log is for history. > > Think about reading this 5 years from now. Would it make sense about > > "previous patches"? > > I thought it was acceptable to do so as the previous patches would > be just before in the git log, but ok I will write something better. The thing is, what does the previous patches do that's important for this change? If I see this in a change log, I may look at the previous patches, but note that the order is not always the same. I could have imported a patch with a date on it that happens to go between the two and cause the history to be different. But that's usually in merged commits which I avoid here, but still. Sometimes in a change log I'll put: "Now that X was done, we can do Y" Where X is done by previous patches, and if someone is curious, they can go and see X. But just saying "Make use of previous patches" doesn't tell me what those previous patches did. And going back and looking at them, I still don't know exactly how those previous patches are related to this change, except that it did some clean up work. -- Steve
Hello Steven, On 1/12/24 17:18, Steven Rostedt wrote: > On Fri, 12 Jan 2024 09:39:45 +0100 > Pierre Gondois <pierre.gondois@arm.com> wrote: > >> trace-cmd can record events in multiple instances: >> $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch >> >> When trying to split a trace.dat file recorded with the above command, >> only the events located in the main buffer seems to be split. The >> events recorded in the test_instance buffer seem to be discarded: >> $ trace-cmd split -i trace.dat -o trace_2.dat 284443 284444 >> $ trace-cmd report trace_2.dat >> cpus=8 >> <...>-525991 [004] 284443.173879: sched_wakeup: [...] >> <...>-525991 [004] 284443.173879: sched_wakeup: [...] >> <...>-525990 [007] 284443.173885: sched_wakeup: [...] >> <...>-525990 [007] 284443.173885: sched_wakeup: [...] >> (no sign of sched_switch events) >> > > >> Make use of the previous patches to split all the instances of >> a trace. > > This shouldn't be in the change log. As the change log is for history. > Think about reading this 5 years from now. Would it make sense about > "previous patches"? > > Thanks for doing this, I'll try to set some time next week to review them. > I want to release 3.3 soon. > > Would you be able to add a way to split out an instance into its own > trace.dat file? Or to choose what you want. > > $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch > > $ trace-cmd split -B test_instance -o test.dat > > Would make test_instance the main buffer in test.dat. > > If you add more than one instance: > > $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch -B timer_instance -e timer > > $ trace-cmd split -B test_instance -o test.dat -B timer_instance > > This would make "test_instance" the main buffer, and keep "timer_instance" > as an instance. > > The "-o file" placement is important. It will make whatever came before it > the main buffer. And perhaps even split out more than one! > > $ trace-cmd split -B test_instance -o test.dat -B timer_instance -o timer.dat > > Would place the "test_instance" as the main instance in test.dat, and the > "timer_instance" as the main instance in timer.dat. I think it might be difficult to select the main instance and name it in the output file this way. As it has no specific name. I suggest to also add a '-b' parameter to designate the main buffer. So with the following record command: $ trace-cmd record -e sched_wakeup -B switch_instance -e sched_switch -B timer_instance -e timer $ trace-cmd split -B switch_instance -B timer_instance -o test.dat test.dat: - switch_instance as the main instance - timer_instance as a side instance (still named 'timer_instance') $ trace-cmd split -b -B switch_instance -o test.dat test.dat: - main instance as the main instance - switch_instance as a side instance (still named 'switch_instance') $ trace-cmd split -B switch_instance -b -o test.dat test.dat: - switch_instance as the main instance - main instance as a side instance (named 'main' [1]) $ trace-cmd split -B switch_instance -o test.dat -B timer_instance test.dat: - switch_instance as the main instance trace.dat.1 - timer_instance as the main instance. No output file is specified, so the default name is used as the output name. $ trace-cmd split -B switch_instance -o test.dat -b test.dat: - switch_instance as the main instance trace.dat.1: - main instance as the main instance. No output file is specified, so the default name is used as the output name. $ trace-cmd split -B switch_instance -o switch.dat -b -o main.dat -B timer_instance -o timer.dat switch.dat: - switch_instance as the main instance main.dat: - main instance as the main instance timer.dat: - timer_instance as the main instance [1] As the main buffer doesn't have a default name, it might be necessary to hardcode the name, as 'main', or as 'main.X' (X being an increasing number) if there is already a main instance in the trace Does it seems ok to you ? Regards, Pierre > > Thoughts? > > -- Steve > > > >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218357 >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> --- >>
On 1/19/24 17:41, Pierre Gondois wrote: > Hello Steven, > > On 1/12/24 17:18, Steven Rostedt wrote: >> On Fri, 12 Jan 2024 09:39:45 +0100 >> Pierre Gondois <pierre.gondois@arm.com> wrote: >> >>> trace-cmd can record events in multiple instances: >>> $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch >>> >>> When trying to split a trace.dat file recorded with the above command, >>> only the events located in the main buffer seems to be split. The >>> events recorded in the test_instance buffer seem to be discarded: >>> $ trace-cmd split -i trace.dat -o trace_2.dat 284443 284444 >>> $ trace-cmd report trace_2.dat >>> cpus=8 >>> <...>-525991 [004] 284443.173879: sched_wakeup: [...] >>> <...>-525991 [004] 284443.173879: sched_wakeup: [...] >>> <...>-525990 [007] 284443.173885: sched_wakeup: [...] >>> <...>-525990 [007] 284443.173885: sched_wakeup: [...] >>> (no sign of sched_switch events) >>> >> >> >>> Make use of the previous patches to split all the instances of >>> a trace. >> >> This shouldn't be in the change log. As the change log is for history. >> Think about reading this 5 years from now. Would it make sense about >> "previous patches"? >> >> Thanks for doing this, I'll try to set some time next week to review them. >> I want to release 3.3 soon. >> >> Would you be able to add a way to split out an instance into its own >> trace.dat file? Or to choose what you want. >> >> $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch >> >> $ trace-cmd split -B test_instance -o test.dat >> >> Would make test_instance the main buffer in test.dat. >> >> If you add more than one instance: >> >> $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch -B timer_instance -e timer >> >> $ trace-cmd split -B test_instance -o test.dat -B timer_instance >> >> This would make "test_instance" the main buffer, and keep "timer_instance" >> as an instance. >> >> The "-o file" placement is important. It will make whatever came before it >> the main buffer. And perhaps even split out more than one! >> >> $ trace-cmd split -B test_instance -o test.dat -B timer_instance -o timer.dat >> >> Would place the "test_instance" as the main instance in test.dat, and the >> "timer_instance" as the main instance in timer.dat. > > I think it might be difficult to select the main instance and name it in > the output file this way. As it has no specific name. I suggest to also add a '-b' parameter to designate > the main buffer. So with the following record command: > > $ trace-cmd record -e sched_wakeup -B switch_instance -e sched_switch -B timer_instance -e timer > > $ trace-cmd split -B switch_instance -B timer_instance -o test.dat > test.dat: > - switch_instance as the main instance > - timer_instance as a side instance (still named 'timer_instance') > > $ trace-cmd split -b -B switch_instance -o test.dat > test.dat: > - main instance as the main instance > - switch_instance as a side instance (still named 'switch_instance') > > $ trace-cmd split -B switch_instance -b -o test.dat > test.dat: > - switch_instance as the main instance > - main instance as a side instance (named 'main' [1]) > > $ trace-cmd split -B switch_instance -o test.dat -B timer_instance > test.dat: > - switch_instance as the main instance > trace.dat.1 > - timer_instance as the main instance. > No output file is specified, so the default name is used as the output name. > > $ trace-cmd split -B switch_instance -o test.dat -b > test.dat: > - switch_instance as the main instance > trace.dat.1: > - main instance as the main instance. > No output file is specified, so the default name is used as the output name. > > $ trace-cmd split -B switch_instance -o switch.dat -b -o main.dat -B timer_instance -o timer.dat > switch.dat: > - switch_instance as the main instance > main.dat: > - main instance as the main instance > timer.dat: > - timer_instance as the main instance > > [1] > As the main buffer doesn't have a default name, it might be necessary > to hardcode the name, as 'main', or as 'main.X' (X being an increasing number) > if there is already a main instance in the trace It would also mean that: $ trace-cmd split -B switch_instance -b -o switch.dat -B timer_instance -b -o timer.dat switch.dat: - switch_instance as the main instance - main instance as a side instance, named 'main' timer.dat: - timer_instance as the main instance - main instance as a side instance, named 'main' And the usage of the function would be: $ trace-cmd split [-i file] [options] [[-b -B instance] -o file] [start [end]] I.e. the options and start/end parameters would be common to all instances/output files. > > Does it seems ok to you ? > > Regards, > Pierre > >> >> Thoughts? >> >> -- Steve >> >> >> >>> >>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218357 >>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >>> --- >>>
On Fri, 19 Jan 2024 18:06:43 +0100 Pierre Gondois <pierre.gondois@arm.com> wrote: > > $ trace-cmd split -B switch_instance -o switch.dat -b -o main.dat -B timer_instance -o timer.dat > > switch.dat: > > - switch_instance as the main instance > > main.dat: > > - main instance as the main instance > > timer.dat: > > - timer_instance as the main instance > > > > [1] > > As the main buffer doesn't have a default name, it might be necessary > > to hardcode the name, as 'main', or as 'main.X' (X being an increasing number) > > if there is already a main instance in the trace > > > It would also mean that: > $ trace-cmd split -B switch_instance -b -o switch.dat -B timer_instance -b -o timer.dat > switch.dat: > - switch_instance as the main instance > - main instance as a side instance, named 'main' > timer.dat: > - timer_instance as the main instance > - main instance as a side instance, named 'main' > > And the usage of the function would be: > $ trace-cmd split [-i file] [options] [[-b -B instance] -o file] [start [end]] > I.e. the options and start/end parameters would be common to all instances/output files. I usually use the term "top" not "main" for the top instance. But I wouldn't want to have a default name anyway. I would just add "--top" instead of '-b' an the main instance. I say "top" as that's the common terminology I have used in man pages. Although, I have used "main" too, I usually append "(top level)" when I do so. We could call it "--main" too, if it sounds better, but should definitely document that it means the "top level instance". Hmm, maybe even '-M' could work. As for naming, if someone wants to make the main instance an instance, then they must also supply a name. $ trace-cmd split -B switch_instance -M --name 'old_main' -o trace-spit.dat And that would switch the main and switch_instance in trace-split.dat. Doesn't mean we can't have both "-M" and "--main" mean the same thing. I want to keep instance single options capitalized. Thanks, -- Steve
diff --git a/tracecmd/trace-split.c b/tracecmd/trace-split.c index 6ab138a2..37ce4176 100644 --- a/tracecmd/trace-split.c +++ b/tracecmd/trace-split.c @@ -426,6 +426,7 @@ static unsigned long long parse_file(struct tracecmd_input *handle, bool *end_reached) { unsigned long long current; + struct handle_list *handle_entry; struct tracecmd_output *ohandle; struct cpu_data *cpu_data; struct tep_record *record; @@ -437,63 +438,71 @@ static unsigned long long parse_file(struct tracecmd_input *handle, int fd; ohandle = tracecmd_copy(handle, output_file, TRACECMD_FILE_CMD_LINES, 0, NULL); + tracecmd_set_out_clock(ohandle, tracecmd_get_trace_clock(handle)); - cpus = tracecmd_cpus(handle); - cpu_data = malloc(sizeof(*cpu_data) * cpus); - if (!cpu_data) - die("Failed to allocate cpu_data for %d cpus", cpus); - - for (cpu = 0; cpu < cpus; cpu++) { - file = get_temp_file(output_file, NULL, cpu); - touch_file(file); - - fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_LARGEFILE, 0644); - cpu_data[cpu].cpu = cpu; - cpu_data[cpu].fd = fd; - cpu_data[cpu].file = file; - cpu_data[cpu].offset = 0; - if (start) - tracecmd_set_cpu_to_timestamp(handle, cpu, start); - } + list_for_each_entry(handle_entry, &handle_list, list) { + cpus = tracecmd_cpus(handle_entry->handle); + cpu_data = malloc(sizeof(*cpu_data) * cpus); + if (!cpu_data) + die("Failed to allocate cpu_data for %d cpus", cpus); - if (only_cpu >= 0) { - parse_cpu(handle, cpu_data, start, end, count, - 1, only_cpu, type, end_reached); - } else if (percpu) { + for (cpu = 0; cpu < cpus; cpu++) { + file = get_temp_file(output_file, handle_entry->name, cpu); + touch_file(file); + + fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_LARGEFILE, 0644); + cpu_data[cpu].cpu = cpu; + cpu_data[cpu].fd = fd; + cpu_data[cpu].file = file; + cpu_data[cpu].offset = 0; + if (start) + tracecmd_set_cpu_to_timestamp(handle_entry->handle, cpu, start); + } + + if (only_cpu >= 0) { + parse_cpu(handle_entry->handle, cpu_data, start, end, count, + 1, only_cpu, type, end_reached); + } else if (percpu) { + for (cpu = 0; cpu < cpus; cpu++) + parse_cpu(handle_entry->handle, cpu_data, start, + end, count, percpu, cpu, type, end_reached); + } else { + parse_cpu(handle_entry->handle, cpu_data, start, + end, count, percpu, -1, type, end_reached); + } + + cpu_list = malloc(sizeof(*cpu_list) * cpus); + if (!cpu_list) + die("Failed to allocate cpu_list for %d cpus", cpus); for (cpu = 0; cpu < cpus; cpu++) - parse_cpu(handle, cpu_data, start, - end, count, percpu, cpu, type, end_reached); - } else - parse_cpu(handle, cpu_data, start, - end, count, percpu, -1, type, end_reached); - - cpu_list = malloc(sizeof(*cpu_list) * cpus); - if (!cpu_list) - die("Failed to allocate cpu_list for %d cpus", cpus); - for (cpu = 0; cpu < cpus; cpu ++) - cpu_list[cpu] = cpu_data[cpu].file; + cpu_list[cpu] = cpu_data[cpu].file; - tracecmd_set_out_clock(ohandle, tracecmd_get_trace_clock(handle)); - if (tracecmd_append_cpu_data(ohandle, cpus, cpu_list) < 0) - die("Failed to append tracing data\n"); - - for (cpu = 0; cpu < cpus; cpu++) { - /* Set the tracecmd cursor to the next set of records */ - if (cpu_data[cpu].offset) { - record = tracecmd_read_at(handle, cpu_data[cpu].offset, NULL); - if (record && (!current || record->ts > current)) - current = record->ts + 1; - tracecmd_free_record(record); + if (!handle_entry->name) + ret = tracecmd_append_cpu_data(ohandle, cpus, cpu_list); + else + ret = tracecmd_append_buffer_cpu_data(ohandle, handle_entry->name, cpus, + cpu_list); + if (ret < 0) + die("Failed to append tracing data\n"); + + for (cpu = 0; cpu < cpus; cpu++) { + /* Set the tracecmd cursor to the next set of records */ + if (cpu_data[cpu].offset) { + record = tracecmd_read_at(handle, cpu_data[cpu].offset, NULL); + if (record && (!current || record->ts > current)) + current = record->ts + 1; + tracecmd_free_record(record); + } } - } - for (cpu = 0; cpu < cpus; cpu++) { - close(cpu_data[cpu].fd); - delete_temp_file(cpu_data[cpu].file); - put_temp_file(cpu_data[cpu].file); + for (cpu = 0; cpu < cpus; cpu++) { + close(cpu_data[cpu].fd); + delete_temp_file(cpu_data[cpu].file); + put_temp_file(cpu_data[cpu].file); + } + free(cpu_data); + free(cpu_list); } - free(cpu_data); - free(cpu_list); tracecmd_output_close(ohandle);
trace-cmd can record events in multiple instances: $ trace-cmd record -e sched_wakeup -B test_instance -e sched_switch When trying to split a trace.dat file recorded with the above command, only the events located in the main buffer seems to be split. The events recorded in the test_instance buffer seem to be discarded: $ trace-cmd split -i trace.dat -o trace_2.dat 284443 284444 $ trace-cmd report trace_2.dat cpus=8 <...>-525991 [004] 284443.173879: sched_wakeup: [...] <...>-525991 [004] 284443.173879: sched_wakeup: [...] <...>-525990 [007] 284443.173885: sched_wakeup: [...] <...>-525990 [007] 284443.173885: sched_wakeup: [...] (no sign of sched_switch events) Make use of the previous patches to split all the instances of a trace. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218357 Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> --- tracecmd/trace-split.c | 109 ++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 50 deletions(-)