lkml.org 
[lkml]   [2021]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] perf script python: Fix buffer size to report iregs in perf script
Em Tue, Jul 06, 2021 at 05:26:12PM +0530, kajoljain escreveu:
>
>
> 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?

Yeah, and the comment you provide right above it explains it, so I think
that is enough, ok?

- Arnaldo

> 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
> >>

--

- Arnaldo

\
 
 \ /
  Last update: 2021-07-06 21:16    [W:0.077 / U:0.924 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site