Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 06 Sep 2022 17:56:37 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To:     Marco Elver <elver@google.com>, Namhyung Kim <namhyung@kernel.org>
CC:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Ian Rogers <irogers@google.com>,
        linux-perf-users <linux-perf-users@vger.kernel.org>
Subject: Re: [PATCH] perf test: Skip sigtrap test on old kernels
User-Agent: K-9 Mail for Android
In-Reply-To: <CANpmjNMHX2S-29Tyw+zKyaWT7saAiEegxbJapQFs7duJTTncdw@mail.gmail.com>
References: <20220903000210.1112014-1-namhyung@kernel.org> <CANpmjNMPh5QjdxXtrCc5FApjgzV=81CNNiwbeg_rE3NxN_WCZw@mail.gmail.com> <YxdA1CVzy9hzE3i1@kernel.org> <CAM9d7ci2ZujdY75DUtZA+f=fru7yh8VJrj8-r2RgZetu57u61A@mail.gmail.com> <CANpmjNMHX2S-29Tyw+zKyaWT7saAiEegxbJapQFs7duJTTncdw@mail.gmail.com>
Message-ID: <F172EC4B-1CFC-46A5-9DC3-146EDF8F5BDF@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On September 6, 2022 5:50:05 PM GMT-03:00, Marco Elver <elver@google=2Ecom=
> wrote:
>On Tue, 6 Sept 2022 at 20:31, Namhyung Kim <namhyung@kernel=2Eorg> wrote:
>>
>> On Tue, Sep 6, 2022 at 5:45 AM Arnaldo Carvalho de Melo <acme@kernel=2E=
org> wrote:
>> >
>> > Em Sat, Sep 03, 2022 at 08:52:01AM +0200, Marco Elver escreveu:
>> > > On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <namhyung@kernel=2Eorg> =
wrote:
>> > > >
>> > > > If it runs on an old kernel, perf_event_open would fail because o=
f the
>> > > > new fields sigtrap and sig_data=2E  Just skip the test if it fail=
ed=2E
>> > > >
>> > > > Cc: Marco Elver <elver@google=2Ecom>
>> > > > Signed-off-by: Namhyung Kim <namhyung@kernel=2Eorg>
>> > > > ---
>> > > >  tools/perf/tests/sigtrap=2Ec | 1 +
>> > > >  1 file changed, 1 insertion(+)
>> > > >
>> > > > diff --git a/tools/perf/tests/sigtrap=2Ec b/tools/perf/tests/sigt=
rap=2Ec
>> > > > index e32ece90e164=2E=2E7057566e6ae4 100644
>> > > > --- a/tools/perf/tests/sigtrap=2Ec
>> > > > +++ b/tools/perf/tests/sigtrap=2Ec
>> > > > @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *t=
est __maybe_unused, int subtest __m
>> > > >         fd =3D sys_perf_event_open(&attr, 0, -1, -1, perf_event_o=
pen_cloexec_flag());
>> > > >         if (fd < 0) {
>> > > >                 pr_debug("FAILED sys_perf_event_open(): %s\n", st=
r_error_r(errno, sbuf, sizeof(sbuf)));
>> > > > +               ret =3D TEST_SKIP;
>> > >
>> > > Wouldn't we be interested if perf_event_open() fails because it cou=
ld
>> > > actually be a bug? By skipping we'll be more likely to miss the fac=
t
>> > > there's a real problem=2E
>> > >
>> > > That's my naive thinking at least - what do other perf tests usuall=
y
>> > > do in this case?
>> >
>> > Yeah, I was going to try and check if this is the only way that, with
>> > the given arguments, perf_event_open would fail, but its better to at
>> > least check errno against -EINVAL or something?
>>
>> EINVAL would be too generic and the kernel returns it in many places=2E
>> I really wish we could have a better error reporting mechanism=2E
>>
>> Maybe we could retry perf_event_open with sigtrap and sig_data cleared=
=2E
>> If it succeeded, then we can skip the test=2E  If it still failed, then=
 report
>> the error=2E  But it still couldn't find a bug in the sigtrap code=2E
>> What do you think?
>
>Yes, that's what I meant, that it could point out an issue with
>sigtrap perf_event_open()=2E
>
>If there's no clear way to determine if it's just not supported or a
>bug, it'd be better to leave it as-is=2E

perf could go fancy and try to add a probe using a source file+line where =
older kernels would fail 8-)

Anyway, perf already does all sorts of kernel capability checks, perhaps t=
his is one of can for sure detect it's something available :-/

One new way could be to look at BTF?

>
>Some other options:
>
>1=2E Provide a way to disable certain tests, if it's known they will
>fail for otherwise benign reasons i=2Ee=2E no support=2E
>
>2=2E Provide a command line option to skip instead of fail tests where
>perf_event_open() returns some particular errnos=2E The default will be
>fail, but you can then choose to trust that failure of
>perf_event_open() means no support, and pass the option=2E