Messages in this thread | | | Subject | Re: [PATCH] perf script python: Fix buffer size to report iregs in perf script | From | kajoljain <> | Date | Tue, 6 Jul 2021 17:26:12 +0530 |
| |
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 >>
| |