Re: [PATCH v4 09/10] tools: lib: perf: Implement riscv mmap support

[Jessica Clarke]

On 31 Jul 2023, at 17:06, Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
> 
> On Mon, Jul 31, 2023 at 5:10 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>> 
>> On Mon, Jul 31, 2023 at 3:27 AM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
>>> 
>>> On Mon, Jul 31, 2023 at 12:15 PM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
>>>> 
>>>> Hi Ian,
>>>> 
>>>> On Fri, Jul 28, 2023 at 7:53 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>>>>> 
>>>>> On Thu, Jul 27, 2023 at 7:28 AM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
>>>>>> 
>>>>>> riscv now supports mmaping hardware counters so add what's needed to
>>>>>> take advantage of that in libperf.
>>>>>> 
>>>>>> Signed-off-by: Alexandre Ghiti <alexghiti@xxxxxxxxxxxx>
>>>>>> Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
>>>>>> Reviewed-by: Atish Patra <atishp@xxxxxxxxxxxx>
>>>>>> ---
>>>>>> tools/lib/perf/mmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 65 insertions(+)
>>>>>> 
>>>>>> diff --git a/tools/lib/perf/mmap.c b/tools/lib/perf/mmap.c
>>>>>> index 0d1634cedf44..378a163f0554 100644
>>>>>> --- a/tools/lib/perf/mmap.c
>>>>>> +++ b/tools/lib/perf/mmap.c
>>>>>> @@ -392,6 +392,71 @@ static u64 read_perf_counter(unsigned int counter)
>>>>>> 
>>>>>> static u64 read_timestamp(void) { return read_sysreg(cntvct_el0); }
>>>>>> 
>>>>>> +#elif __riscv_xlen == 64
>>>>> 
>>>>> This is something of an odd guard, perhaps:
>>>>> #elif defined(__riscv) && __riscv_xlen == 64
>>>>> 
>>>>> That way it is more intention revealing that this is riscv code. Could
>>>>> you add a comment relating to the __riscv_xlen ?
>>>> 
>>>> I guess Andrew answered that already.
>>>> 
>> 
>> Not sure. I still think it looks weird:
>> ...
>> #if defined(__i386__) || defined(__x86_64__)
>> ...
>> #elif defined(__aarch64__)
>> ...
>> #elif __riscv_xlen == 64
>> ...
>> #else
>> static u64 read_perf_counter(unsigned int counter __maybe_unused) { return 0; }
>> static u64 read_timestamp(void) { return 0; }
>> #endif
>> 
>> The first two are clearly #ifdef-ing architecture specific assembly
>> code, under what conditions I get RISC-V code  ¯\(ツ)/¯ At least worth
>> a comment like "csrr is only available when you have xlens of 64
>> because ..."
> 
> __riscv_xlen indicates riscv64, which is the only target of this
> patchset. But if you prefer, I don't mind adding back the
> defined(__riscv) if I re-spin a new version.

I mean, -Wundef is a thing that will scream at you if you evaluate a
macro that’s undefined and get an implicit 0, and parts of the linux
build seem to enable it. So I would strongly recommend against
(ab)using that feature of the preprocessor, especially when it aligns
with greater clarity.

Jess