Received-SPF: pass (google.com: domain of linux-kernel+bounces-194606-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240521133029.83654-1-ben.gainey@arm.com> <20240521133029.83654-5-ben.gainey@arm.com>
 <CAM9d7chssov_fW3ZbpYqGRyhS=xuHLyJ7=rkxTNnJ6fkgEdGJQ@mail.gmail.com>
 <a12573d18f7c8bf73c24c572e43965e3f250acea.camel@arm.com> <e114bf8d2e7b6c0874fffdf8867c954a08772da3.camel@arm.com>
In-Reply-To: <e114bf8d2e7b6c0874fffdf8867c954a08772da3.camel@arm.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Wed, 29 May 2024 12:18:58 -0700
Message-ID: <CAM9d7cjmH=nkEt-z_XpZ1J0oL-EY2d52oz70-KpN+uzM5SEk6A@mail.gmail.com>
Subject: Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when
 opening events
To: Ben Gainey <Ben.Gainey@arm.com>
Cc: "alexander.shishkin@linux.intel.com" <alexander.shishkin@linux.intel.com>, 
	"peterz@infradead.org" <peterz@infradead.org>, "acme@kernel.org" <acme@kernel.org>, 
	"mingo@redhat.com" <mingo@redhat.com>, James Clark <James.Clark@arm.com>, 
	"adrian.hunter@intel.com" <adrian.hunter@intel.com>, "irogers@google.com" <irogers@google.com>, 
	"jolsa@kernel.org" <jolsa@kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"linux-perf-users@vger.kernel.org" <linux-perf-users@vger.kernel.org>, Mark Rutland <Mark.Rutland@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello,

On Wed, May 29, 2024 at 10:48=E2=80=AFAM Ben Gainey <Ben.Gainey@arm.com> wr=
ote:
>
> 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=E2=80=AFAM Ben Gainey <ben.gainey@arm.co=
m>
> > > 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