Re: [PATCH] perf tools: Add arm64 version of get_cpuid()

From: John Garry <john.garry@huawei.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: <peterz@infradead.org>, <mingo@redhat.com>,
	<mark.rutland@arm.com>, <alexander.shishkin@linux.intel.com>,
	<jolsa@redhat.com>, <namhyung@kernel.org>,
	<linux-kernel@vger.kernel.org>, <will@kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linuxarm@huawei.com>
Subject: Re: [PATCH] perf tools: Add arm64 version of get_cpuid()
Date: Wed, 12 Feb 2020 15:49:01 +0000	[thread overview]
Message-ID: <ae11aac0-0edf-633c-cafd-6db39faef6b1@huawei.com> (raw)
In-Reply-To: <20200212134024.GC22501@kernel.org>

On 12/02/2020 13:40, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 07, 2020 at 09:13:43AM +0000, John Garry escreveu:
>> On 13/12/2019 13:54, John Garry wrote:
>>
>> Hi Arnaldo,
>>
>> Do we need some reviews on this? Or was it missed/still catching up?
> 
> Got lost in the holidays, devconf.cz, vacations, sorry, picking it up
> now, together with a Tested-by by Shaokun Zhang, some issues with the
> formatting of the patch:
> 
> - Avoid starting lines with '#' as those will vanish when I use 'git am'

ah, so this must be why people use, for example, '/include "..."' in 
commit logs

> 
> - Separate the commit log message from the diff using a '---' at the
>    begining of the line, otherwise 'git am' fails

apologizes, I must have cut that by mistake

> 
> I fixed those up now, will test with my build containers, thanks.
> 

Thanks

> - Arnaldo
>   
>> Cheers,
>> John
>>
>>> Add an arm64 version of get_cpuid(), which is used for various annotation
>>> and headers - for example, I now get the CPUID in "perf report --header",
>>> as shown in this snippet:
>>>
>>> # hostname : ubuntu
>>> # os release : 5.5.0-rc1-dirty
>>> # perf version : 5.5.rc1.gbf8a13dc9851
>>> # arch : aarch64
>>> # nrcpus online : 96
>>> # nrcpus avail : 96
>>> # cpuid : 0x00000000480fd010
>>>
>>> Since much of the code to read the MIDR is already in get_cpuid_str(),
>>> factor out this code.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>
>>> diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
>>> index a32e4b72a98f..d730666ab95d 100644
>>> --- a/tools/perf/arch/arm64/util/header.c
>>> +++ b/tools/perf/arch/arm64/util/header.c
>>> @@ -1,8 +1,10 @@
>>>    #include <stdio.h>
>>>    #include <stdlib.h>
>>>    #include <perf/cpumap.h>
>>> +#include <util/cpumap.h>
>>>    #include <internal/cpumap.h>
>>>    #include <api/fs/fs.h>
>>> +#include <errno.h>
>>>    #include "debug.h"
>>>    #include "header.h"
>>> @@ -12,26 +14,21 @@
>>>    #define MIDR_VARIANT_SHIFT      20
>>>    #define MIDR_VARIANT_MASK       (0xf << MIDR_VARIANT_SHIFT)
>>> -char *get_cpuid_str(struct perf_pmu *pmu)
>>> +static int _get_cpuid(char *buf, size_t sz, struct perf_cpu_map *cpus)
>>>    {
>>> -	char *buf = NULL;
>>> -	char path[PATH_MAX];
>>>    	const char *sysfs = sysfs__mountpoint();
>>> -	int cpu;
>>>    	u64 midr = 0;
>>> -	struct perf_cpu_map *cpus;
>>> -	FILE *file;
>>> +	int cpu;
>>> -	if (!sysfs || !pmu || !pmu->cpus)
>>> -		return NULL;
>>> +	if (!sysfs || sz < MIDR_SIZE)
>>> +		return EINVAL;
>>> -	buf = malloc(MIDR_SIZE);
>>> -	if (!buf)
>>> -		return NULL;
>>> +	cpus = perf_cpu_map__get(cpus);
>>> -	/* read midr from list of cpus mapped to this pmu */
>>> -	cpus = perf_cpu_map__get(pmu->cpus);
>>>    	for (cpu = 0; cpu < perf_cpu_map__nr(cpus); cpu++) {
>>> +		char path[PATH_MAX];
>>> +		FILE *file;
>>> +
>>>    		scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
>>>    				sysfs, cpus->map[cpu]);
>>> @@ -57,12 +54,48 @@ char *get_cpuid_str(struct perf_pmu *pmu)
>>>    		break;
>>>    	}
>>> -	if (!midr) {
>>> +	perf_cpu_map__put(cpus);
>>> +
>>> +	if (!midr)
>>> +		return EINVAL;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int get_cpuid(char *buf, size_t sz)
>>> +{
>>> +	struct perf_cpu_map *cpus = perf_cpu_map__new(NULL);
>>> +	int ret;
>>> +
>>> +	if (!cpus)
>>> +		return EINVAL;
>>> +
>>> +	ret = _get_cpuid(buf, sz, cpus);
>>> +
>>> +	perf_cpu_map__put(cpus);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +char *get_cpuid_str(struct perf_pmu *pmu)
>>> +{
>>> +	char *buf = NULL;
>>> +	int res;
>>> +
>>> +	if (!pmu || !pmu->cpus)
>>> +		return NULL;
>>> +
>>> +	buf = malloc(MIDR_SIZE);
>>> +	if (!buf)
>>> +		return NULL;
>>> +
>>> +	/* read midr from list of cpus mapped to this pmu */
>>> +	res = _get_cpuid(buf, MIDR_SIZE, pmu->cpus);
>>> +	if (res) {
>>>    		pr_err("failed to get cpuid string for PMU %s\n", pmu->name);
>>>    		free(buf);
>>>    		buf = NULL;
>>>    	}
>>> -	perf_cpu_map__put(cpus);
>>>    	return buf;
>>>    }
>>>
>>
>