Em Wed, Feb 24, 2016 at 02:37:45PM +0530, Ravi Bangoria escreveu:
> commit d49dadea7862 ("perf tools: Make 'trace' or 'trace_fields' sort key
> default for tracepoint events") makes 'trace' sort key as a default
> while displaying report for tracepoint.
>
> Because tracepoint(kvm_hv:kvm_guest_exit) is used as a default event,
> perf kvm report will display output as a list of tracepoint hits and
> not with a normal report columns.
>
> This patch will replace 'trace' field with 'overhead,comm,dso,sym' while
> displaying perf kvm report of powerpc.
>
> Before applying patch:
>
> $ ./perf kvm --guestkallsyms=guest.kallsyms --guestmodules=guest.modules report --stdio
> # To display the perf.data header info, please use --header/--header-only options.
> #
> #
> # Total Lost Samples: 0
> #
> # Samples: 181K of event 'kvm_hv:kvm_guest_exit'
> # Event count (approx.): 181061
> #
> # Overhead Trace output
> # ........ .................................................................................
> #
> 0.02% VCPU 8: trap=HV_DECREMENTER pc=0xc000000000091924 msr=0x8000000000009032, ceded=0
> 0.00% VCPU 0: trap=HV_DECREMENTER pc=0xc000000000091924 msr=0x8000000000009032, ceded=0
> 0.00% VCPU 8: trap=HV_DECREMENTER pc=0x10005c7c msr=0x800000000280f032, ceded=0
> 0.00% VCPU 8: trap=HV_DECREMENTER pc=0x1001ef14 msr=0x800000000280f032, ceded=0
> 0.00% VCPU 8: trap=HV_DECREMENTER pc=0x3fff83398830 msr=0x800000000280f032, ceded=0
> 0.00% VCPU 8: trap=HV_DECREMENTER pc=0x3fff833a6fe4 msr=0x800000000280f032, ceded=0
> 0.00% VCPU 8: trap=HV_DECREMENTER pc=0x3fff833a7a64 msr=0x800000000280f032, ceded=0
>
> After applying patch:
>
> $ ./perf kvm --guestkallsyms=guest.kallsyms --guestmodules=guest.modules report --stdio
> # To display the perf.data header info, please use --header/--header-only options.
> #
> #
> # Total Lost Samples: 0
> #
> # Samples: 181K of event 'kvm_hv:kvm_guest_exit'
> # Event count (approx.): 181061
> #
> # Overhead Command Shared Object Symbol
> # ........ ....... ....................... ..............................
> #
> 0.02% :57276 [guest.kernel.kallsyms] [g] .plpar_hcall_norets
> 0.00% :57274 [guest.kernel.kallsyms] [g] .plpar_hcall_norets
> 0.00% :57276 [guest.kernel.kallsyms] [g] .__copy_tofrom_user_power7
> 0.00% :57276 [guest.kernel.kallsyms] [g] ._atomic_dec_and_lock
> 0.00% :57276 [guest.kernel.kallsyms] [g] ._raw_spin_lock
> 0.00% :57276 [guest.kernel.kallsyms] [g] ._switch
> 0.00% :57276 [guest.kernel.kallsyms] [g] .bio_add_page
> 0.00% :57276 [guest.kernel.kallsyms] [g] .kmem_cache_alloc
>
> Signed-off-by: Ravi Bangoria <[email protected]>
> ---
> tools/perf/builtin-report.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 31ec4ba..5d96882 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -930,6 +930,11 @@ repeat:
> else
> use_browser = 0;
>
> + if (!field_order &&
> + is_perf_data_reorded_on_ppc(session->evlist) &&
> + perf_guest_only())
> + field_order = "overhead,comm,dso,sym";
> +
Can you please do it as:
__weak void arch__override_field_order(struct perf_evlist *evlist, const char **field_order)
{
}
This way we don't see any arch specific stuff in the tool, also I
haven't seen any doc update, are you sure nothing needs to be added to
tools/perf/Documentaton/ for any of these patches?
I think this needs to be documented further, probably in
tools/perf/design.txt too?
> if (setup_sorting(session->evlist) < 0) {
> if (sort_order)
> parse_options_usage(report_usage, options, "s", 1);
> --
> 2.1.4
Thanks Arnaldo,
Please find my comments.
On Wednesday 02 March 2016 07:55 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 24, 2016 at 02:37:45PM +0530, Ravi Bangoria escreveu:
>> use_browser = 0;
>>
>> + if (!field_order &&
>> + is_perf_data_reorded_on_ppc(session->evlist) &&
>> + perf_guest_only())
>> + field_order = "overhead,comm,dso,sym";
>> +
> Can you please do it as:
>
> __weak void arch__override_field_order(struct perf_evlist *evlist, const char **field_order)
> {
> }
So you mean like this - Just implement only weak function and move code
into it?
ie. No strong implementation at this point of time.
Like,
__weak void arch__override_field_order(struct perf_evlist *evlist, const
char **f_order)
{
if (!field_order &&
is_perf_data_reorded_on_ppc(session->evlist) &&
perf_guest_only())
*field_order = "overhead,comm,dso,sym";
}
Then I can do that.
But if you are proposing to implement a strong function and move this code
into in, then we won't be able to enable cross arch reporting.
>
> This way we don't see any arch specific stuff in the tool, also I
> haven't seen any doc update, are you sure nothing needs to be added to
> tools/perf/Documentaton/ for any of these patches?
>
> I think this needs to be documented further, probably in
> tools/perf/design.txt too?
Yes, I'll do this in next version.
Regards,
Ravi
Em Wed, Mar 02, 2016 at 09:16:48PM +0530, Ravi Bangoria escreveu:
> Thanks Arnaldo,
>
> Please find my comments.
>
> On Wednesday 02 March 2016 07:55 PM, Arnaldo Carvalho de Melo wrote:
> >Em Wed, Feb 24, 2016 at 02:37:45PM +0530, Ravi Bangoria escreveu:
> >> use_browser = 0;
> >>+ if (!field_order &&
> >>+ is_perf_data_reorded_on_ppc(session->evlist) &&
> >>+ perf_guest_only())
> >>+ field_order = "overhead,comm,dso,sym";
> >>+
> >Can you please do it as:
> >
> >__weak void arch__override_field_order(struct perf_evlist *evlist, const char **field_order)
> >{
> >}
>
> So you mean like this - Just implement only weak function and move code into
> it?
> ie. No strong implementation at this point of time.
>
> Like,
>
> __weak void arch__override_field_order(struct perf_evlist *evlist, const
> char **f_order)
> {
> if (!field_order &&
> is_perf_data_reorded_on_ppc(session->evlist) &&
Oh, I see, ugh, when running on x86_64 we wouldn't use this, so we need
to have per arch default field orders, now I have to recall why is it
that we need this per-arch field order :-\
- Arnaldo
> perf_guest_only())
> *field_order = "overhead,comm,dso,sym";
> }
>
> Then I can do that.
>
> But if you are proposing to implement a strong function and move this code
> into in, then we won't be able to enable cross arch reporting.
>
> >
> >This way we don't see any arch specific stuff in the tool, also I
> >haven't seen any doc update, are you sure nothing needs to be added to
> >tools/perf/Documentaton/ for any of these patches?
> >
> >I think this needs to be documented further, probably in
> >tools/perf/design.txt too?
>
> Yes, I'll do this in next version.
>
> Regards,
> Ravi
Thanks acme,
On Wednesday 02 March 2016 09:52 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 02, 2016 at 09:16:48PM +0530, Ravi Bangoria escreveu:
>> Thanks Arnaldo,
>>
>> Please find my comments.
>>
>> On Wednesday 02 March 2016 07:55 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Wed, Feb 24, 2016 at 02:37:45PM +0530, Ravi Bangoria escreveu:
>>>> use_browser = 0;
>>>> + if (!field_order &&
>>>> + is_perf_data_reorded_on_ppc(session->evlist) &&
>>>> + perf_guest_only())
>>>> + field_order = "overhead,comm,dso,sym";
>>>> +
>>> Can you please do it as:
>>>
>>> __weak void arch__override_field_order(struct perf_evlist *evlist, const char **field_order)
>>> {
>>> }
>> So you mean like this - Just implement only weak function and move code into
>> it?
>> ie. No strong implementation at this point of time.
>>
>> Like,
>>
>> __weak void arch__override_field_order(struct perf_evlist *evlist, const
>> char **f_order)
>> {
>> if (!field_order &&
>> is_perf_data_reorded_on_ppc(session->evlist) &&
> Oh, I see, ugh, when running on x86_64 we wouldn't use this, so we need
> to have per arch default field orders, now I have to recall why is it
> that we need this per-arch field order :-\
Sorry, I'm little bit confused. We need arch specific functionality present
on all arch to make cross arch reporting possible.
for example, record perf.data on ppc and report on x86, we need
ppc specific function present in perf binary compiled on x86.
Please let me know if I understood it wrong.
Regads,
Ravi
Hi Arnaldo,
Gentle reminder :) Any updates?
Regards,
Ravi
On Thursday 03 March 2016 06:49 AM, Ravi Bangoria wrote:
> Thanks acme,
>
> On Wednesday 02 March 2016 09:52 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Mar 02, 2016 at 09:16:48PM +0530, Ravi Bangoria escreveu:
>>> Thanks Arnaldo,
>>>
>>> Please find my comments.
>>>
>>> On Wednesday 02 March 2016 07:55 PM, Arnaldo Carvalho de Melo wrote:
>>>> Em Wed, Feb 24, 2016 at 02:37:45PM +0530, Ravi Bangoria escreveu:
>>>>> use_browser = 0;
>>>>> + if (!field_order &&
>>>>> + is_perf_data_reorded_on_ppc(session->evlist) &&
>>>>> + perf_guest_only())
>>>>> + field_order = "overhead,comm,dso,sym";
>>>>> +
>>>> Can you please do it as:
>>>>
>>>> __weak void arch__override_field_order(struct perf_evlist *evlist,
>>>> const char **field_order)
>>>> {
>>>> }
>>> So you mean like this - Just implement only weak function and move
>>> code into
>>> it?
>>> ie. No strong implementation at this point of time.
>>>
>>> Like,
>>>
>>> __weak void arch__override_field_order(struct perf_evlist *evlist,
>>> const
>>> char **f_order)
>>> {
>>> if (!field_order &&
>>> is_perf_data_reorded_on_ppc(session->evlist) &&
>> Oh, I see, ugh, when running on x86_64 we wouldn't use this, so we need
>> to have per arch default field orders, now I have to recall why is it
>> that we need this per-arch field order :-\
>
> Sorry, I'm little bit confused. We need arch specific functionality
> present
> on all arch to make cross arch reporting possible.
>
> for example, record perf.data on ppc and report on x86, we need
> ppc specific function present in perf binary compiled on x86.
>
> Please let me know if I understood it wrong.
>
> Regads,
> Ravi
>