Re: [PATCH v2 6/8] perf mem: Fix wrong verbose output for recording events

From: "Jin, Yao" <yao.jin@linux.intel.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
	mingo@redhat.com, alexander.shishkin@linux.intel.com,
	Linux-kernel@vger.kernel.org, ak@linux.intel.com,
	kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH v2 6/8] perf mem: Fix wrong verbose output for recording events
Date: Tue, 1 Jun 2021 09:10:39 +0800	[thread overview]
Message-ID: <3627ceef-fc72-1951-c1b6-4c59525a1afa@linux.intel.com> (raw)
In-Reply-To: <YLVmVBDKGkSWgHwR@krava>

Hi Jiri,

On 6/1/2021 6:42 AM, Jiri Olsa wrote:
> On Thu, May 27, 2021 at 08:16:08AM +0800, Jin Yao wrote:
>> Current code:
>>
>> for (j = 0; j < argc; j++, i++)
>>          rec_argv[i] = argv[j];
>>
>> if (verbose > 0) {
>>          pr_debug("calling: record ");
>>
>>          while (rec_argv[j]) {
>>                  pr_debug("%s ", rec_argv[j]);
>>                  j++;
>>          }
>>          pr_debug("\n");
>> }
>>
>> The entries of argv[] are copied to the end of rec_argv[], not
>> copied to the beginning of rec_argv[]. So the index j at
>> rec_argv[] doesn't point to the first event.
>>
>> Now we record the start index and end index for events in rec_argv[],
>> and print them if verbose is enabled.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>> v2:
>>   - New in v2.
>>
>>   tools/perf/builtin-mem.c | 8 +++++---
> 
> hi,
> do we need the same in c2c as well?
> 
> jirka
> 

perf c2c is a bit different. It sets 'j = 0;' before 'while' loop so it prints all of rec_argv[].

In test,

# perf c2c record -vvv -a -- sleep 1

calling: record -W -d --phys-data --sample-cpu -e 
{cpu_core/mem-loads-aux/,cpu_core/mem-loads,ldlat=30/}:P -e cpu_atom/mem-loads,ldlat=30/P -e 
cpu_core/mem-stores/P -e cpu_atom/mem-stores/P -a sleep 1

The verbose output looks OK.

Thanks
Jin Yao

>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index 6b633df458c2..0fd2a74dbaca 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -65,6 +65,7 @@ static const char * const *record_mem_usage = __usage;
>>   static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   {
>>   	int rec_argc, i = 0, j, tmp_nr = 0;
>> +	int start, end;
>>   	const char **rec_argv;
>>   	char **rec_tmp;
>>   	int ret;
>> @@ -144,9 +145,11 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   	if (mem->data_page_size)
>>   		rec_argv[i++] = "--data-page-size";
>>   
>> +	start = i;
>>   	ret = perf_mem_events__record_args(rec_argv, &i, rec_tmp, &tmp_nr);
>>   	if (ret)
>>   		goto out;
>> +	end = i;
>>   
>>   	if (all_user)
>>   		rec_argv[i++] = "--all-user";
>> @@ -160,10 +163,9 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   	if (verbose > 0) {
>>   		pr_debug("calling: record ");
>>   
>> -		while (rec_argv[j]) {
>> +		for (j = start; j < end; j++)
>>   			pr_debug("%s ", rec_argv[j]);
>> -			j++;
>> -		}
>> +
>>   		pr_debug("\n");
>>   	}
>>   
>> -- 
>> 2.17.1
>>
> 