Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] perf script python: Fix buffer size to report iregs in
 perf script
To:     Kajol Jain <kjain@linux.ibm.com>, acme@kernel.org
Cc:     maddy@linux.vnet.ibm.com, atrajeev@linux.vnet.ibm.com,
        pc@us.ibm.com, linux-kernel@vger.kernel.org, jolsa@redhat.com,
        ravi.bangoria@linux.ibm.com, linux-perf-users@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org
References: <20210628062341.155839-1-kjain@linux.ibm.com>
From:   Nageswara Sastry <rnsastry@linux.ibm.com>
Message-ID: <46ddcac9-e115-6b96-11ec-6208bcd1c98a@linux.ibm.com>
Date:   Mon, 28 Jun 2021 12:45:36 +0530
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210628062341.155839-1-kjain@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk

Tested by creating perf-script.py using perf script
and priting the iregs. Seen more values with this patch.


Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>

On 28/06/21 11:53 am, 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
> @@ -687,7 +687,7 @@ static void set_sample_datasrc_in_dict(PyObject *dict,
>   			_PyUnicode_FromString(decode));
>   }
>   
> -static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
> +static void regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
>   {
>   	unsigned int i = 0, r;
>   	int printed = 0;
> @@ -695,7 +695,7 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
>   	bf[0] = 0;
>   
>   	if (!regs || !regs->regs)
> -		return 0;
> +		return;
>   
>   	for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
>   		u64 val = regs->regs[i++];
> @@ -704,8 +704,6 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
>   				     "%5s:0x%" PRIx64 " ",
>   				     perf_reg_name(r), val);
>   	}
> -
> -	return printed;
>   }
>   
>   static void set_regs_in_dict(PyObject *dict,
> @@ -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];
>   
>   	regs_map(&sample->intr_regs, attr->sample_regs_intr, bf, sizeof(bf));
>   
> 

-- 
Thanks and Regards
R.Nageswara Sastry