If it runs on an old kernel, perf_event_open would fail because of the
new fields sigtrap and sig_data. Just skip the test if it failed.
Cc: Marco Elver <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/tests/sigtrap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
index e32ece90e164..7057566e6ae4 100644
--- a/tools/perf/tests/sigtrap.c
+++ b/tools/perf/tests/sigtrap.c
@@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
if (fd < 0) {
pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
+ ret = TEST_SKIP;
goto out_restore_sigaction;
}
--
2.37.2.789.g6183377224-goog
On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
>
> If it runs on an old kernel, perf_event_open would fail because of the
> new fields sigtrap and sig_data. Just skip the test if it failed.
>
> Cc: Marco Elver <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/tests/sigtrap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> index e32ece90e164..7057566e6ae4 100644
> --- a/tools/perf/tests/sigtrap.c
> +++ b/tools/perf/tests/sigtrap.c
> @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
> fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
> if (fd < 0) {
> pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
> + ret = TEST_SKIP;
Wouldn't we be interested if perf_event_open() fails because it could
actually be a bug? By skipping we'll be more likely to miss the fact
there's a real problem.
That's my naive thinking at least - what do other perf tests usually
do in this case?
Thanks,
-- Marco
Em Sat, Sep 03, 2022 at 08:52:01AM +0200, Marco Elver escreveu:
> On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
> >
> > If it runs on an old kernel, perf_event_open would fail because of the
> > new fields sigtrap and sig_data. Just skip the test if it failed.
> >
> > Cc: Marco Elver <[email protected]>
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
> > tools/perf/tests/sigtrap.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> > index e32ece90e164..7057566e6ae4 100644
> > --- a/tools/perf/tests/sigtrap.c
> > +++ b/tools/perf/tests/sigtrap.c
> > @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
> > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
> > if (fd < 0) {
> > pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
> > + ret = TEST_SKIP;
>
> Wouldn't we be interested if perf_event_open() fails because it could
> actually be a bug? By skipping we'll be more likely to miss the fact
> there's a real problem.
>
> That's my naive thinking at least - what do other perf tests usually
> 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?
- Arnaldo
On Tue, Sep 6, 2022 at 5:45 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> Em Sat, Sep 03, 2022 at 08:52:01AM +0200, Marco Elver escreveu:
> > On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
> > >
> > > If it runs on an old kernel, perf_event_open would fail because of the
> > > new fields sigtrap and sig_data. Just skip the test if it failed.
> > >
> > > Cc: Marco Elver <[email protected]>
> > > Signed-off-by: Namhyung Kim <[email protected]>
> > > ---
> > > tools/perf/tests/sigtrap.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> > > index e32ece90e164..7057566e6ae4 100644
> > > --- a/tools/perf/tests/sigtrap.c
> > > +++ b/tools/perf/tests/sigtrap.c
> > > @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
> > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
> > > if (fd < 0) {
> > > pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
> > > + ret = TEST_SKIP;
> >
> > Wouldn't we be interested if perf_event_open() fails because it could
> > actually be a bug? By skipping we'll be more likely to miss the fact
> > there's a real problem.
> >
> > That's my naive thinking at least - what do other perf tests usually
> > 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.
I really wish we could have a better error reporting mechanism.
Maybe we could retry perf_event_open with sigtrap and sig_data cleared.
If it succeeded, then we can skip the test. If it still failed, then report
the error. But it still couldn't find a bug in the sigtrap code.
What do you think?
Thanks,
Namhyung
On September 6, 2022 5:50:05 PM GMT-03:00, Marco Elver <[email protected]> wrote:
>On Tue, 6 Sept 2022 at 20:31, Namhyung Kim <[email protected]> wrote:
>>
>> On Tue, Sep 6, 2022 at 5:45 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
>> >
>> > Em Sat, Sep 03, 2022 at 08:52:01AM +0200, Marco Elver escreveu:
>> > > On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
>> > > >
>> > > > If it runs on an old kernel, perf_event_open would fail because of the
>> > > > new fields sigtrap and sig_data. Just skip the test if it failed.
>> > > >
>> > > > Cc: Marco Elver <[email protected]>
>> > > > Signed-off-by: Namhyung Kim <[email protected]>
>> > > > ---
>> > > > tools/perf/tests/sigtrap.c | 1 +
>> > > > 1 file changed, 1 insertion(+)
>> > > >
>> > > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
>> > > > index e32ece90e164..7057566e6ae4 100644
>> > > > --- a/tools/perf/tests/sigtrap.c
>> > > > +++ b/tools/perf/tests/sigtrap.c
>> > > > @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
>> > > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
>> > > > if (fd < 0) {
>> > > > pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
>> > > > + ret = TEST_SKIP;
>> > >
>> > > Wouldn't we be interested if perf_event_open() fails because it could
>> > > actually be a bug? By skipping we'll be more likely to miss the fact
>> > > there's a real problem.
>> > >
>> > > That's my naive thinking at least - what do other perf tests usually
>> > > 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.
>> I really wish we could have a better error reporting mechanism.
>>
>> Maybe we could retry perf_event_open with sigtrap and sig_data cleared.
>> If it succeeded, then we can skip the test. If it still failed, then report
>> the error. But it still couldn't find a bug in the sigtrap code.
>> What do you think?
>
>Yes, that's what I meant, that it could point out an issue with
>sigtrap perf_event_open().
>
>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.
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 this is one of can for sure detect it's something available :-/
One new way could be to look at BTF?
>
>Some other options:
>
>1. Provide a way to disable certain tests, if it's known they will
>fail for otherwise benign reasons i.e. no support.
>
>2. Provide a command line option to skip instead of fail tests where
>perf_event_open() returns some particular errnos. 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.
On Tue, 6 Sept 2022 at 20:31, Namhyung Kim <[email protected]> wrote:
>
> On Tue, Sep 6, 2022 at 5:45 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> > Em Sat, Sep 03, 2022 at 08:52:01AM +0200, Marco Elver escreveu:
> > > On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
> > > >
> > > > If it runs on an old kernel, perf_event_open would fail because of the
> > > > new fields sigtrap and sig_data. Just skip the test if it failed.
> > > >
> > > > Cc: Marco Elver <[email protected]>
> > > > Signed-off-by: Namhyung Kim <[email protected]>
> > > > ---
> > > > tools/perf/tests/sigtrap.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> > > > index e32ece90e164..7057566e6ae4 100644
> > > > --- a/tools/perf/tests/sigtrap.c
> > > > +++ b/tools/perf/tests/sigtrap.c
> > > > @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
> > > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
> > > > if (fd < 0) {
> > > > pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
> > > > + ret = TEST_SKIP;
> > >
> > > Wouldn't we be interested if perf_event_open() fails because it could
> > > actually be a bug? By skipping we'll be more likely to miss the fact
> > > there's a real problem.
> > >
> > > That's my naive thinking at least - what do other perf tests usually
> > > 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.
> I really wish we could have a better error reporting mechanism.
>
> Maybe we could retry perf_event_open with sigtrap and sig_data cleared.
> If it succeeded, then we can skip the test. If it still failed, then report
> the error. But it still couldn't find a bug in the sigtrap code.
> What do you think?
Yes, that's what I meant, that it could point out an issue with
sigtrap perf_event_open().
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.
Some other options:
1. Provide a way to disable certain tests, if it's known they will
fail for otherwise benign reasons i.e. no support.
2. Provide a command line option to skip instead of fail tests where
perf_event_open() returns some particular errnos. 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.
On Tue, Sep 6, 2022 at 1:56 PM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
>
>
> On September 6, 2022 5:50:05 PM GMT-03:00, Marco Elver <[email protected]> wrote:
> >On Tue, 6 Sept 2022 at 20:31, Namhyung Kim <[email protected]> wrote:
> >>
> >> On Tue, Sep 6, 2022 at 5:45 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> >> >
> >> > Em Sat, Sep 03, 2022 at 08:52:01AM +0200, Marco Elver escreveu:
> >> > > On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
> >> > > >
> >> > > > If it runs on an old kernel, perf_event_open would fail because of the
> >> > > > new fields sigtrap and sig_data. Just skip the test if it failed.
> >> > > >
> >> > > > Cc: Marco Elver <[email protected]>
> >> > > > Signed-off-by: Namhyung Kim <[email protected]>
> >> > > > ---
> >> > > > tools/perf/tests/sigtrap.c | 1 +
> >> > > > 1 file changed, 1 insertion(+)
> >> > > >
> >> > > > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> >> > > > index e32ece90e164..7057566e6ae4 100644
> >> > > > --- a/tools/perf/tests/sigtrap.c
> >> > > > +++ b/tools/perf/tests/sigtrap.c
> >> > > > @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
> >> > > > fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
> >> > > > if (fd < 0) {
> >> > > > pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
> >> > > > + ret = TEST_SKIP;
> >> > >
> >> > > Wouldn't we be interested if perf_event_open() fails because it could
> >> > > actually be a bug? By skipping we'll be more likely to miss the fact
> >> > > there's a real problem.
> >> > >
> >> > > That's my naive thinking at least - what do other perf tests usually
> >> > > 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.
> >> I really wish we could have a better error reporting mechanism.
> >>
> >> Maybe we could retry perf_event_open with sigtrap and sig_data cleared.
> >> If it succeeded, then we can skip the test. If it still failed, then report
> >> the error. But it still couldn't find a bug in the sigtrap code.
> >> What do you think?
> >
> >Yes, that's what I meant, that it could point out an issue with
> >sigtrap perf_event_open().
> >
> >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.
>
> 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 this is one of can for sure detect it's something available :-/
>
> One new way could be to look at BTF?
Yeah, we could check BTF if it had the attr.sigtrap field and skip if not.
Let me see how I can do that. :)
Thanks,
Namhyung
On 03/09/2022 07:52, Marco Elver wrote:
> On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
>>
>> If it runs on an old kernel, perf_event_open would fail because of the
>> new fields sigtrap and sig_data. Just skip the test if it failed.
>>
>> Cc: Marco Elver <[email protected]>
>> Signed-off-by: Namhyung Kim <[email protected]>
>> ---
>> tools/perf/tests/sigtrap.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
>> index e32ece90e164..7057566e6ae4 100644
>> --- a/tools/perf/tests/sigtrap.c
>> +++ b/tools/perf/tests/sigtrap.c
>> @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
>> fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
>> if (fd < 0) {
>> pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
>> + ret = TEST_SKIP;
>
> Wouldn't we be interested if perf_event_open() fails because it could
> actually be a bug? By skipping we'll be more likely to miss the fact
> there's a real problem.
>
> That's my naive thinking at least - what do other perf tests usually
> do in this case?
I missed this discussion but I just submitted a patch with a similar
issue [1]. To me, it doesn't make sense to have the tests pass on older
kernels if this lowers the value of the tests by accepting possibly
invalid values. If you want to test older kernels then just use older
tests, but maybe there is some use case that I'm not aware of.
[1]: "[PATCH 0/1] perf test: Fix attr tests for PERF_FORMAT_LOST"
>
> Thanks,
> -- Marco
Hello,
On Mon, Sep 26, 2022 at 8:06 AM James Clark <[email protected]> wrote:
>
>
>
> On 03/09/2022 07:52, Marco Elver wrote:
> > On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
> >>
> >> If it runs on an old kernel, perf_event_open would fail because of the
> >> new fields sigtrap and sig_data. Just skip the test if it failed.
> >>
> >> Cc: Marco Elver <[email protected]>
> >> Signed-off-by: Namhyung Kim <[email protected]>
> >> ---
> >> tools/perf/tests/sigtrap.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> >> index e32ece90e164..7057566e6ae4 100644
> >> --- a/tools/perf/tests/sigtrap.c
> >> +++ b/tools/perf/tests/sigtrap.c
> >> @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
> >> fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
> >> if (fd < 0) {
> >> pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
> >> + ret = TEST_SKIP;
> >
> > Wouldn't we be interested if perf_event_open() fails because it could
> > actually be a bug? By skipping we'll be more likely to miss the fact
> > there's a real problem.
> >
> > That's my naive thinking at least - what do other perf tests usually
> > do in this case?
>
> I missed this discussion but I just submitted a patch with a similar
> issue [1]. To me, it doesn't make sense to have the tests pass on older
> kernels if this lowers the value of the tests by accepting possibly
> invalid values. If you want to test older kernels then just use older
> tests, but maybe there is some use case that I'm not aware of.
Thanks for your opinion. But my test environment is running the tests
on random machines which may run some old kernel. I agree that it
should not skip the real problems but I think we can find a good way
to detect old, unsupported kernels reliably like using BTF.
Thanks,
Namhyung
On 26/09/2022 17:19, Namhyung Kim wrote:
> Hello,
>
> On Mon, Sep 26, 2022 at 8:06 AM James Clark <[email protected]> wrote:
>>
>>
>>
>> On 03/09/2022 07:52, Marco Elver wrote:
>>> On Sat, 3 Sept 2022 at 02:02, Namhyung Kim <[email protected]> wrote:
>>>>
>>>> If it runs on an old kernel, perf_event_open would fail because of the
>>>> new fields sigtrap and sig_data. Just skip the test if it failed.
>>>>
>>>> Cc: Marco Elver <[email protected]>
>>>> Signed-off-by: Namhyung Kim <[email protected]>
>>>> ---
>>>> tools/perf/tests/sigtrap.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
>>>> index e32ece90e164..7057566e6ae4 100644
>>>> --- a/tools/perf/tests/sigtrap.c
>>>> +++ b/tools/perf/tests/sigtrap.c
>>>> @@ -140,6 +140,7 @@ static int test__sigtrap(struct test_suite *test __maybe_unused, int subtest __m
>>>> fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag());
>>>> if (fd < 0) {
>>>> pr_debug("FAILED sys_perf_event_open(): %s\n", str_error_r(errno, sbuf, sizeof(sbuf)));
>>>> + ret = TEST_SKIP;
>>>
>>> Wouldn't we be interested if perf_event_open() fails because it could
>>> actually be a bug? By skipping we'll be more likely to miss the fact
>>> there's a real problem.
>>>
>>> That's my naive thinking at least - what do other perf tests usually
>>> do in this case?
>>
>> I missed this discussion but I just submitted a patch with a similar
>> issue [1]. To me, it doesn't make sense to have the tests pass on older
>> kernels if this lowers the value of the tests by accepting possibly
>> invalid values. If you want to test older kernels then just use older
>> tests, but maybe there is some use case that I'm not aware of.
>
> Thanks for your opinion. But my test environment is running the tests
> on random machines which may run some old kernel. I agree that it
> should not skip the real problems but I think we can find a good way
> to detect old, unsupported kernels reliably like using BTF.
I'm currently adding a test for the new VG user register [1]. I'd like
to test that it's available when the system has SVE but not when it
hasn't so that the ABI is locked down. But that test will also depend on
the kernel version, because it will never be available on older ones.
So for new tests should we always add two versions of the test, one for
newer and one for older kernels? That seems like that has not been done
so far in the tests. I'd still question the value of running the new
tests on old kernels because they are mainly to validate that new code
doesn't cause regressions. It's a lot of work to keep them backwards
compatible for the case of running them on old kernels when old tests
can be used instead.
If I am do add a version check would just looking at the uname be
enough? Or how would the BTF version of that look?
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?h=for-next/perf&id=cbb0c02caf4bd98b9e0cd6d7420734b8e9a35703
>
> Thanks,
> Namhyung