Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events

[James Clark]

On 29/05/2024 20:18, Namhyung Kim wrote:
> Hello,
> 
> On Wed, May 29, 2024 at 10:48 AM Ben Gainey <Ben.Gainey@xxxxxxx> wrote:
>>
>> On Mon, 2024-05-27 at 18:47 +0100, Ben Gainey wrote:
>>> On Thu, 2024-05-23 at 18:41 -0700, Namhyung Kim wrote:
>>>> On Tue, May 21, 2024 at 6:30 AM Ben Gainey <ben.gainey@xxxxxxx>
>>>> wrote:
>>>>>
>>>>> The tool will now default to this new mode if the user specifies
>>>>> a
>>>>> sampling group when not in system-wide mode, and when --no-
>>>>> inherit
>>>>> is not specified.
>>>>>
>>>>> This change updates evsel to allow the combination of inherit
>>>>> and PERF_SAMPLE_READ.
>>>>>
>>>>> A fallback is implemented for kernel versions where this feature
>>>>> is
>>>>> not
>>>>> supported.
>>>>
>>>> But I'm afraid the test would fail on old kernels.  Maybe we need
>>>> to
>>>> put it in the selftests.
>>>>
>>>
>>> Sorry, not clear what you mean?
>>>
>>> Is the issue that the fallback on older kernels fails, or that the
>>> "perf test" tests fail?
>>>
>>> Thanks
>>> Ben
>>
>> Just to follow up, I've rechecked the fallback on an unmodified 6.9.1
>> kernel with the following:
>>
>>     perf record -vv -e '{cycles,instructions}:S' ls
>>
>> With an unpatched version of perf running on an unpatched kernel, the
>> cycles event is opened as:
>>
>>     ------------------------------------------------------------
>>     perf_event_attr:
>>       type                             0 (PERF_TYPE_HARDWARE)
>>       size                             136
>>       config                           0 (PERF_COUNT_HW_CPU_CYCLES)
>>       { sample_period, sample_freq }   4000
>>       sample_type                      IP|TID|TIME|READ|ID|PERIOD
>>       read_format                      ID|GROUP|LOST
>>       disabled                         1
>>       exclude_kernel                   1
>>       exclude_hv                       1
>>       mmap                             1
>>       comm                             1
>>       freq                             1
>>       enable_on_exec                   1
>>       task                             1
>>       sample_id_all                    1
>>       exclude_guest                    1
>>       mmap2                            1
>>       comm_exec                        1
>>       ksymbol                          1
>>       bpf_event                        1
>>     ------------------------------------------------------------
>>
>> whereas with these patches applied to perf, on an unpatched kernel, the
>> output is as follows
>>
>>     ------------------------------------------------------------
>>     perf_event_attr:
>>       type                             0 (PERF_TYPE_HARDWARE)
>>       size                             136
>>       config                           0 (PERF_COUNT_HW_CPU_CYCLES)
>>       { sample_period, sample_freq }   4000
>>       sample_type                      IP|TID|TIME|READ|ID|PERIOD
>>       read_format                      ID|GROUP|LOST
>>       disabled                         1
>>       inherit                          1
>>       exclude_kernel                   1
>>       exclude_hv                       1
>>       mmap                             1
>>       comm                             1
>>       freq                             1
>>       enable_on_exec                   1
>>       task                             1
>>       sample_id_all                    1
>>       exclude_guest                    1
>>       mmap2                            1
>>       comm_exec                        1
>>       ksymbol                          1
>>       bpf_event                        1
>>     ------------------------------------------------------------
>>     sys_perf_event_open: pid 3442954  cpu 0  group_fd -1  flags 0x8
>>     sys_perf_event_open failed, error -22
>>     Using PERF_SAMPLE_READ / :S modifier is not compatible with
>> inherit, falling back to no-inherit.
>>     ------------------------------------------------------------
>>     perf_event_attr:
>>       type                             0 (PERF_TYPE_HARDWARE)
>>       size                             136
>>       config                           0 (PERF_COUNT_HW_CPU_CYCLES)
>>       { sample_period, sample_freq }   4000
>>       sample_type                      IP|TID|TIME|READ|ID|PERIOD
>>       read_format                      ID|GROUP|LOST
>>       disabled                         1
>>       exclude_kernel                   1
>>       exclude_hv                       1
>>       mmap                             1
>>       comm                             1
>>       freq                             1
>>       enable_on_exec                   1
>>       task                             1
>>       sample_id_all                    1
>>       exclude_guest                    1
>>       mmap2                            1
>>       comm_exec                        1
>>       ksymbol                          1
>>       bpf_event                        1
>>     ------------------------------------------------------------
>>
>> The command falls back to the same configuration as was previously
>> used. The same is true for the instructions event.
>>
>> `perf test` fails on an unpatched kernel in "15: Setup struct
>> perf_event_attr" for the test "test-record-group-sampling1" but that is
>> surely expected for unpatched kernels?
>>
>> Is there some very-old kernel version where this would be expected to
>> succeed by accident?
> 
> I don't think so but I don't want the test to fail depending on the
> kernel version.  Maybe we can check the allowed combination
> first and skip the test if perf_event_open() fails.  And then it can
> verify if the kernel rejects the unsupported combinations.  Not
> sure if it's easy to do that in the current attr test framework.
> 
> Thanks,
> Namhyung

I added the kernel version feature to the attr tests. Seems like you can
add two tests, one for before and one for after.

Search "kernel_until" in tests/attr