From: kajoljain <kjain@linux.ibm.com>
To: "acme@kernel.org" <acme@kernel.org>, Jiri Olsa <jolsa@redhat.com>
Cc: maddy@linux.vnet.ibm.com, atrajeev@linux.vnet.ibm.com,
linux-kernel@vger.kernel.org, ravi.bangoria@linux.ibm.com,
linux-perf-users@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
rnsastry@linux.ibm.com, "Paul A. Clarke" <pc@us.ibm.com>
Subject: Re: [PATCH] perf script python: Fix buffer size to report iregs in perf script
Date: Tue, 6 Jul 2021 17:26:12 +0530 [thread overview]
Message-ID: <c6fb2136-21e1-325a-f7f7-9745dbe29661@linux.ibm.com> (raw)
In-Reply-To: <ee98968a-f343-a68e-9a3e-58e97dc130c8@linux.ibm.com>
On 6/29/21 12:39 PM, kajoljain wrote:
>
>
> On 6/28/21 8:19 PM, Paul A. Clarke wrote:
>> On Mon, Jun 28, 2021 at 11:53:41AM +0530, Kajol Jain wrote:
>>> Commit 48a1f565261d ("perf script python: Add more PMU fields
>>> to event handler dict") added functionality to report fields like
>>> weight, iregs, uregs etc via perf report.
>>> That commit predefined buffer size to 512 bytes to print those fields.
>>>
>>> But incase of powerpc, since we added extended regs support
>>> in commits:
>>>
>>> Commit 068aeea3773a ("perf powerpc: Support exposing Performance Monitor
>>> Counter SPRs as part of extended regs")
>>> Commit d735599a069f ("powerpc/perf: Add extended regs support for
>>> power10 platform")
>>>
>>> Now iregs can carry more bytes of data and this predefined buffer size
>>> can result to data loss in perf script output.
>>>
>>> Patch resolve this issue by making buffer size dynamic based on number
>>> of registers needed to print. It also changed return type for function
>>> "regs_map" from int to void, as the return value is not being used by
>>> the caller function "set_regs_in_dict".
>>>
>>> Fixes: 068aeea3773a ("perf powerpc: Support exposing Performance Monitor
>>> Counter SPRs as part of extended regs")
>>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>>> ---
>>> .../util/scripting-engines/trace-event-python.c | 17 ++++++++++++-----
>>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
>>> index 4e4aa4c97ac5..c8c9706b4643 100644
>>> --- a/tools/perf/util/scripting-engines/trace-event-python.c
>>> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
>> [...]
>>> @@ -713,7 +711,16 @@ static void set_regs_in_dict(PyObject *dict,
>>> struct evsel *evsel)
>>> {
>>> struct perf_event_attr *attr = &evsel->core.attr;
>>> - char bf[512];
>>> +
>>> + /*
>>> + * Here value 28 is a constant size which can be used to print
>>> + * one register value and its corresponds to:
>>> + * 16 chars is to specify 64 bit register in hexadecimal.
>>> + * 2 chars is for appending "0x" to the hexadecimal value and
>>> + * 10 chars is for register name.
>>> + */
>>> + int size = __sw_hweight64(attr->sample_regs_intr) * 28;
>>> + char bf[size];
>>
>> I propose using a template rather than a magic number here. Something like:
>> const char reg_name_tmpl[] = "10 chars ";
>> const char reg_value_tmpl[] = "0x0123456789abcdef";
>> const int size = __sw_hweight64(attr->sample_regs_intr) +
>> sizeof reg_name_tmpl + sizeof reg_value_tmpl;
>>
>
> Hi Paul,
> Thanks for reviewing the patch. Yes these are
> some standardization we can do by creating macros for different
> fields.
> The basic idea is, we want to provide significant buffer size
> based on number of registers present in sample_regs_intr to accommodate
> all data.
>
Hi Arnaldo/Jiri,
Is the approach used in this patch looks fine to you?
Thanks,
Kajol Jain
> But before going to optimizing code, Arnaldo/Jiri, is this approach looks good to you?
>
>> Pardon my ignorance, but is there no separation/whitespace between the name
>> and the value?
>
> This is how we will get data via perf script
>
> r0:0xc000000000112008
> r1:0xc000000023b37920
> r2:0xc00000000144c900
> r3:0xc0000000bc566120
> r4:0xc0000000c5600000
> r5:0x2606c6506ca
> r6:0xc000000023b378f8
> r7:0xfffffd9f93a48f0e
> .....
>
> And is there some significance to 10 characters for the
>> register name, or is that a magic number?
>
> Most of the register name are within 10 characters, basically we are giving this
> magic number to make sure we have enough space in buffer to contain all registers
> name with colon.
>
> Thanks,
> Kajol Jain
>
>>
>> PC
>>
next prev parent reply other threads:[~2021-07-06 12:11 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-28 6:23 [PATCH] perf script python: Fix buffer size to report iregs in perf script Kajol Jain
2021-06-28 7:15 ` Nageswara Sastry
2021-06-28 14:49 ` Paul A. Clarke
2021-06-29 7:09 ` kajoljain
2021-07-06 11:56 ` kajoljain [this message]
2021-07-06 19:15 ` Arnaldo Carvalho de Melo
2021-07-07 5:46 ` kajoljain
2021-07-07 14:04 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c6fb2136-21e1-325a-f7f7-9745dbe29661@linux.ibm.com \
--to=kjain@linux.ibm.com \
--cc=acme@kernel.org \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=maddy@linux.vnet.ibm.com \
--cc=pc@us.ibm.com \
--cc=ravi.bangoria@linux.ibm.com \
--cc=rnsastry@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).