| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fVWUAWa4vZ7_Zb_3DjaMiTGT6grfFO2j_kOFbuZemJZTA@mail.gmail.com>
Date: Thu, 22 Feb 2024 10:32:54 -0800
From: Ian Rogers <irogers@...gle.com>
To: Kyle Huey <me@...ehuey.com>
Cc: Kyle Huey <khuey@...ehuey.com>, "Robert O'Callahan" <robert@...llahan.org>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] perf test: Test FASYNC with watermark wakeups.
On Thu, Feb 22, 2024 at 9:55 AM Kyle Huey <me@...ehuey.com> wrote:
>
> On Wed, Feb 21, 2024 at 10:36 AM Ian Rogers <irogers@...gle.com> wrote:
> >
> > On Wed, Feb 21, 2024 at 9:52 AM Kyle Huey <me@...ehuey.com> wrote:
> > >
> > > The test uses PERF_RECORD_SWITCH records to fill the ring buffer and
> > > trigger the watermark wakeup, which in turn should trigger an IO
> > > signal.
> > >
> > > Signed-off-by: Kyle Huey <khuey@...ehuey.com>
> > > ---
> > > tools/perf/tests/Build | 1 +
> > > tools/perf/tests/builtin-test.c | 1 +
> > > tools/perf/tests/tests.h | 1 +
> > > tools/perf/tests/watermark_signal.c | 123 ++++++++++++++++++++++++++++
> > > 4 files changed, 126 insertions(+)
> > > create mode 100644 tools/perf/tests/watermark_signal.c
> > >
> > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> > > index 53ba9c3e20e0..de43eb60b280 100644
> > > --- a/tools/perf/tests/Build
> > > +++ b/tools/perf/tests/Build
> > > @@ -67,6 +67,7 @@ perf-y += sigtrap.o
> > > perf-y += event_groups.o
> > > perf-y += symbols.o
> > > perf-y += util.o
> > > +perf-y += watermark_signal.o
> > >
> > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc))
> > > perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
> > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > > index 4a5973f9bb9b..715c01a2172a 100644
> > > --- a/tools/perf/tests/builtin-test.c
> > > +++ b/tools/perf/tests/builtin-test.c
> > > @@ -124,6 +124,7 @@ static struct test_suite *generic_tests[] = {
> > > &suite__event_groups,
> > > &suite__symbols,
> > > &suite__util,
> > > + &suite__watermark_signal,
> > > NULL,
> > > };
> > >
> > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> > > index dad3d7414142..7ef4e0d0a77b 100644
> > > --- a/tools/perf/tests/tests.h
> > > +++ b/tools/perf/tests/tests.h
> > > @@ -146,6 +146,7 @@ DECLARE_SUITE(sigtrap);
> > > DECLARE_SUITE(event_groups);
> > > DECLARE_SUITE(symbols);
> > > DECLARE_SUITE(util);
> > > +DECLARE_SUITE(watermark_signal);
> > >
> > > /*
> > > * PowerPC and S390 do not support creation of instruction breakpoints using the
> > > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tests/watermark_signal.c
> > > new file mode 100644
> > > index 000000000000..ae4abedc4b7c
> > > --- /dev/null
> > > +++ b/tools/perf/tests/watermark_signal.c
> > > @@ -0,0 +1,123 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <stddef.h>
> > > +#include <signal.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/wait.h>
> > > +#include <unistd.h>
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +
> > > +#include "tests.h"
> > > +#include "debug.h"
> > > +#include "event.h"
> > > +#include "../perf-sys.h"
> > > +#include "cloexec.h"
> > > +#include <internal/lib.h> // page_size
> > > +
> > > +static int sigio_count;
> > > +
> > > +static void handle_sigio(int sig __always_unused)
> > > +{
> > > + ++sigio_count;
> > > +}
> > > +
> > > +static void do_child(void)
> > > +{
> > > + for (int i = 0; i < 20; ++i)
> > > + sleep(1);
> > > +
> > > + exit(0);
> > > +}
> > > +
> > > +static int test__watermark_signal(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> > > +{
> > > + struct perf_event_attr attr;
> > > + struct perf_event_mmap_page *p = NULL;
> > > + sighandler_t previous_sigio = SIG_ERR;
> > > + pid_t child = -1;
> > > + int fd = -1;
> > > + int ret = TEST_FAIL;
> > > +
> > > + previous_sigio = signal(SIGIO, handle_sigio);
> > > + if (previous_sigio == SIG_ERR) {
> > > + pr_debug("failed setting SIGIO handler\n");
> > > + goto cleanup;
> > > + }
> > > +
> > > + memset(&attr, 0, sizeof(attr));
> > > + attr.size = sizeof(attr);
> > > + attr.type = PERF_TYPE_SOFTWARE;
> > > + attr.config = PERF_COUNT_SW_DUMMY;
> > > + attr.sample_period = 1;
> > > + attr.disabled = 1;
> > > + attr.watermark = 1;
> > > + attr.context_switch = 1;
> > > + attr.wakeup_watermark = 1;
> > > +
> > > + child = fork();
> > > + if (child == 0)
> > > + do_child();
> > > + else if (child < 0) {
> > > + pr_debug("failed fork() %d\n", errno);
> > > + goto cleanup;
> > > + }
> > > +
> > > + fd = sys_perf_event_open(&attr, child, -1, -1,
> > > + perf_event_open_cloexec_flag());
> > > + if (fd < 0) {
> > > + pr_debug("failed opening event %llx\n", attr.config);
> > > + goto cleanup;
> > > + }
> > > +
> > > + 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
> mean the test would always return ok or skip, not ok or fail. Is that
> ok?
Not great :-) Is there any hint from the errno? I was hoping for
EOPNOTSUPP but I'm guessing it will be EINVAL, in which case probably
something like:
/* Older kernels lack support. */
ret = (errno == EINVAL) ? TEST_SKIP : TEST_FAIL;
> > > +
> > > + if (fcntl(fd, F_SETOWN, getpid())) {
> > > + pr_debug("failed F_SETOWN getpid() %d\n", errno);
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (fcntl(fd, F_SETSIG, SIGIO)) {
> > > + pr_debug("failed F_SETSIG SIGIO %d\n", errno);
> > > + goto cleanup;
> > > + }
> > > +
> > > + p = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> > > + if (p == NULL) {
> > > + pr_debug("failed to mmap\n");
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (ioctl(fd, PERF_EVENT_IOC_ENABLE, 0)) {
> > > + pr_debug("failed PERF_EVENT_IOC_ENABLE %d\n", errno);
> > > + goto cleanup;
> > > + }
> > > +
> > > + sleep(30);
> >
> > This is kind of a painful wait, could it be less? No sleeps would be best :-)
>
> We could synchronize with the child using SIGSTOP instead of sleeping
> here. Not sure about getting rid of the tiny sleeps in the child. I
> have observed that sched_yield() doesn't reliably trigger context
> switches (which isn't surprising). I'll experiment with blocking on a
> pipe or something.
Great, thanks!
Ian
> - Kyle
>
> > Thanks,
> > Ian
> >
> > > +
> > > + ret = (sigio_count == 1) ? TEST_OK : TEST_FAIL;
> > > +
> > > +cleanup:
> > > + if (p != NULL)
> > > + munmap(p, 2 * page_size);
> > > +
> > > + if (fd >= 0)
> > > + close(fd);
> > > +
> > > + if (child > 0) {
> > > + kill(child, SIGTERM);
> > > + waitpid(child, NULL, 0);
> > > + }
> > > +
> > > + if (previous_sigio != SIG_ERR)
> > > + signal(SIGIO, previous_sigio);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +DEFINE_SUITE("Watermark signal handler", watermark_signal);
> > > --
> > > 2.34.1
> > >
Powered by blists - more mailing lists