Received-SPF: pass (google.com: domain of linux-kernel+bounces-78878-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240221175210.19936-1-khuey@kylehuey.com> <20240221175210.19936-2-khuey@kylehuey.com>
 <CAP-5=fXsv7TJ_SVOZc38fN0gn+7cWBcMWt3FdVLcs5v0_vO=uw@mail.gmail.com>
 <CAP045AoSHWoOP3TN=6Hf2wZj7X9Y41sThBQWCDZ3BEP68qeTBw@mail.gmail.com> <ZdemibZepMqWvv6U@x1>
In-Reply-To: <ZdemibZepMqWvv6U@x1>
From: Kyle Huey <me@kylehuey.com>
Date: Fri, 23 Feb 2024 09:35:29 -0800
Message-ID: <CAP045Aqc6woHGYKJG4d=x4gPd0=PmrLeFv25Rkts1a8xFuocOQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf test: Test FASYNC with watermark wakeups.
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ian Rogers <irogers@google.com>, Kyle Huey <khuey@kylehuey.com>, 
	"Robert O'Callahan" <robert@ocallahan.org>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@redhat.com>, Namhyung Kim <namhyung@kernel.org>, 
	Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 22, 2024 at 11:54=E2=80=AFAM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Thu, Feb 22, 2024 at 09:55:36AM -0800, Kyle Huey wrote:
> > On Wed, Feb 21, 2024 at 10:36=E2=80=AFAM Ian Rogers <irogers@google.com=
> wrote:
> > > On Wed, Feb 21, 2024 at 9:52=E2=80=AFAM Kyle Huey <me@kylehuey.com> w=
rote:
> > > > +       if (fcntl(fd, F_SETFL, FASYNC)) {
> > > > +               pr_debug("failed F_SETFL FASYNC %d\n", errno);
> > > > +               goto cleanup;
> > > > +       }
>
> > > Thanks for the work! The perf tool and perf test should run on older
> > > kernels ideally without failure. I'm assuming this would fail on an
> > > older kernel. Could we make the return value skip in that case?
>
> > Ah, hmm, I wasn't aware of that. This would fail on an older kernel,
> > yes. It's not possible to distinguish between an older kernel and a
> > kernel where this fix broke (at least not without hardcoding in an
> > expected good kernel version, which seems undesirable), so that would
>
> Take a look at:
>
>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commi=
t/tools/perf/tests/sigtrap.c?id=3D650e0bde43f35bb675e87e30f679a57cfa22e0e5
>
> To see how introspecting using BTF can be used to figure out if internal
> data structures have what is needed, or if functions with some specific
> arguments are present, etc, for sigtrap we have, in the patch above:
>
> -       TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_TH=
READS * ctx.iterate_on);
> +       expected_sigtraps =3D NUM_THREADS * ctx.iterate_on;
> +
> +       if (ctx.signal_count < expected_sigtraps && kernel_with_sleepable=
_spinlocks()) {
> +               pr_debug("Expected %d sigtraps, got %d, running on a kern=
el with sleepable spinlocks.\n",
> +                        expected_sigtraps, ctx.signal_count);
> +               pr_debug("See https://lore.kernel.org/all/e368f2c848d77fb=
c8d259f44e2055fe469c219cf.camel@gmx.de/\n");
> +               return TEST_SKIP;
> +       } else
> +               TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count=
, expected_sigtraps);
>
> And then:
>
> +static bool kernel_with_sleepable_spinlocks(void)
> +{
> +       const struct btf_member *member;
> +       const struct btf_type *type;
> +       const char *type_name;
> +       int id;
> +
> +       if (!btf__available())
> +               return false;
> +
> +       id =3D btf__find_by_name_kind(btf, "spinlock", BTF_KIND_STRUCT);
> +       if (id < 0)
> +               return false;
> +
> +       // Only RT has a "lock" member for "struct spinlock"
> +       member =3D __btf_type__find_member_by_name(id, "lock");
> +       if (member =3D=3D NULL)
> +               return false;
> +
> +       // But check its type as well
> +       type =3D btf__type_by_id(btf, member->type);
> +       if (!type || !btf_is_struct(type))
> +               return false;
> +
> +       type_name =3D btf__name_by_offset(btf, type->name_off);
> +       return type_name && !strcmp(type_name, "rt_mutex_base");
> +}
>
> > mean the test would always return ok or skip, not ok or fail. Is that
> > ok?
>
> It should return:
>
> Ok if the kernel has what is needed and the test passes
> Skip if the test fails and the kernel doesn't have what is needed
> FAIL if the test fails and the kernel HAS what is needed.
>
> 'perf test sigtrap' also checks for the presence of the feature it
> requires:
>
> static bool attr_has_sigtrap(void)
> {
>         int id;
>
>         if (!btf__available()) {
>                 /* should be an old kernel */
>                 return false;
>         }
>
>         id =3D btf__find_by_name_kind(btf, "perf_event_attr", BTF_KIND_ST=
RUCT);
>         if (id < 0)
>                 return false;
>
>         return __btf_type__find_member_by_name(id, "sigtrap") !=3D NULL;
> }
>
>         fd =3D sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloe=
xec_flag());
>         if (fd < 0) {
>                 if (attr_has_sigtrap()) {
>                         pr_debug("FAILED sys_perf_event_open(): %s\n",
>                                  str_error_r(errno, sbuf, sizeof(sbuf)));
>                 } else {
>                         pr_debug("perf_event_attr doesn't have sigtrap\n"=
);
>                         ret =3D TEST_SKIP;
>                 }
>                 goto out_restore_sigaction;
>         }
>
> - Arnaldo

I think perhaps I'm barking up the wrong tree here. This seems like a
ton of work just to write a regression test. Maybe I should be doing
this in tools/testing/selftests instead?

- Kyle