Re: perf-tools-next: Request for testing hybrid changes

[Ravi Bangoria <ravi.bangoria@amd.com>]

On 04-Jul-23 6:14 PM, James Clark wrote:
> 
> 
> On 04/07/2023 13:33, Ravi Bangoria wrote:
>>>> For heterogeneous ARM PMUs I believe this is exactly the path taken to
>>>> cause the perf_event_open to fail. The resolution is to add the bit to
>>>> the PMU's capabilities. I believe doing it here will suffice:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/drivers/perf/arm_pmu.c?h=perf/core#n877
>>>> So:
>>>> ```
>>>> .capabilities = PERF_PMU_CAP_HETEROGENEOUS_CPUS | PERF_PMU_CAP_EXTENDED_REGS,
>>>> ```
>>>> becomes:
>>>> ```
>>>> .capabilities = PERF_PMU_CAP_HETEROGENEOUS_CPUS |
>>>> PERF_PMU_CAP_EXTENDED_REGS | PERF_PMU_CAP_EXTENDED_HW_TYPE,
>>>> ```
>>>> Now strictly speaking you only need this in the case of heterogeneous
>>>> PMUs, but that's the case for other bits here and I don't see special
>>>> case logic...
>>>>
>>>
>>> Hi Ian,
>>>
>>> I tested the above and it seems to work fine on Arm so I will send it
>>> shortly. Some of the legacy perf stat use cases are working better with
>>> it so it makes sense to me. I also don't see a reason to not always do
>>> it even for homogeneous PMUs. To be honest I don't see a reason not to
>>> remove it as a capability and have it as the default behaviour
>>> everywhere? It all happens in the core code so I'm not sure adding it
>>> per PMU device is necessary, but maybe there is a reason?
>>
>> EventSelect on AMD is 12 bits where upper nibble is passed via config[35:32].
>> i.e. it conflicts with the semantics of PERF_PMU_CAP_EXTENDED_HW_TYPE. So,
>> please don't make it default :)
>>
>> Thanks,
>> Ravi
> 
> Ah, thanks for the answer. It's a bit late now but I wonder why it
> wasn't added as a completely new argument to perf_event_open. That would
> have avoided that. Is there a difference between adding a completely new
> argument and adding a new argument by stuffing something into an
> existing field.

My bad. let me take my words back. I forgot that perf uses generalized event
codes for PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE and does not use actual
raw config.

Anyway, I think there is still a bit of ambiguity with this code. For ex,

  type = PERF_TYPE_HARDWARE;
  config = 0xU00000002;

counts instructions for 0xU = 0x0 / 0x1 / 0x3 / 0x4 / invalid pmu `type`,
otherwise perf_event_open() fails. See below simple test:

$ cat test.c
#include <linux/perf_event.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/syscall.h>
#include <unistd.h>

static long
perf_event_open(struct perf_event_attr *hw_event, pid_t pid,
               int cpu, int group_fd, unsigned long flags)
{
        int ret;

        ret = syscall(SYS_perf_event_open, hw_event, pid, cpu,
                      group_fd, flags);
        return ret;
}

void loop(unsigned long config_upper_32)
{
        struct perf_event_attr pe;
        int fd;

        memset(&pe, 0, sizeof(pe));
        pe.type = PERF_TYPE_HARDWARE;
        pe.size = sizeof(pe);
        pe.config = config_upper_32 | 0x2;
        pe.disabled = 1;
        pe.exclude_kernel = 1;
        pe.exclude_hv = 1;

        fd = perf_event_open(&pe, 0, -1, -1, 0);
        if (fd == -1) {
                printf("type: %d, config: 0x%llx: Error\n", pe.type, pe.config);
                return;
        }

        printf("type: %d, config: 0x%llx: Ok\n", pe.type, pe.config);
        close(fd);
}

int main(void)
{
        unsigned long i;

        for (i = 0; i < 0x20; i++)
                loop(i << 32);
        return 0;
}

$ gcc -Wall -g test.c

$ ./a.out
type: 0, config: 0x2: Ok
type: 0, config: 0x100000002: Ok
type: 0, config: 0x200000002: Error
type: 0, config: 0x300000002: Ok
type: 0, config: 0x400000002: Ok
type: 0, config: 0x500000002: Error
type: 0, config: 0x600000002: Error
type: 0, config: 0x700000002: Error
type: 0, config: 0x800000002: Error
type: 0, config: 0x900000002: Error
type: 0, config: 0xa00000002: Error
type: 0, config: 0xb00000002: Error
type: 0, config: 0xc00000002: Error
type: 0, config: 0xd00000002: Error
type: 0, config: 0xe00000002: Error
type: 0, config: 0xf00000002: Error
type: 0, config: 0x1000000002: Error
type: 0, config: 0x1100000002: Error
type: 0, config: 0x1200000002: Ok
type: 0, config: 0x1300000002: Ok
type: 0, config: 0x1400000002: Ok
type: 0, config: 0x1500000002: Ok
type: 0, config: 0x1600000002: Ok
type: 0, config: 0x1700000002: Ok
type: 0, config: 0x1800000002: Ok
type: 0, config: 0x1900000002: Ok
type: 0, config: 0x1a00000002: Ok
type: 0, config: 0x1b00000002: Ok
type: 0, config: 0x1c00000002: Ok
type: 0, config: 0x1d00000002: Ok
type: 0, config: 0x1e00000002: Ok
type: 0, config: 0x1f00000002: Ok
$

Max pmu `type` value on this machine is 17 (i.e. 0x11).

Thanks,
Ravi