Received-SPF: pass (google.com: domain of linux-kernel+bounces-77019-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::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>
In-Reply-To: <CAP-5=fXsv7TJ_SVOZc38fN0gn+7cWBcMWt3FdVLcs5v0_vO=uw@mail.gmail.com>
From: Kyle Huey <me@kylehuey.com>
Date: Thu, 22 Feb 2024 09:55:36 -0800
Message-ID: <CAP045AoSHWoOP3TN=6Hf2wZj7X9Y41sThBQWCDZ3BEP68qeTBw@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf test: Test FASYNC with watermark wakeups.
To: Ian Rogers <irogers@google.com>
Cc: Kyle Huey <khuey@kylehuey.com>, "Robert O'Callahan" <robert@ocallahan.org>, 
	Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, 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 Wed, Feb 21, 2024 at 10:36=E2=80=AFAM Ian Rogers <irogers@google.com> wr=
ote:
>
> On Wed, Feb 21, 2024 at 9:52=E2=80=AFAM Kyle Huey <me@kylehuey.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@kylehuey.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 +=3D sigtrap.o
> >  perf-y +=3D event_groups.o
> >  perf-y +=3D symbols.o
> >  perf-y +=3D util.o
> > +perf-y +=3D watermark_signal.o
> >
> >  ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc))
> >  perf-$(CONFIG_DWARF_UNWIND) +=3D 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[] =3D {
> >         &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/wat=
ermark_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 =3D 0; i < 20; ++i)
> > +               sleep(1);
> > +
> > +       exit(0);
> > +}
> > +
> > +static int test__watermark_signal(struct test_suite *test __maybe_unus=
ed, int subtest __maybe_unused)
> > +{
> > +       struct perf_event_attr attr;
> > +       struct perf_event_mmap_page *p =3D NULL;
> > +       sighandler_t previous_sigio =3D SIG_ERR;
> > +       pid_t child =3D -1;
> > +       int fd =3D -1;
> > +       int ret =3D TEST_FAIL;
> > +
> > +       previous_sigio =3D signal(SIGIO, handle_sigio);
> > +       if (previous_sigio =3D=3D SIG_ERR) {
> > +               pr_debug("failed setting SIGIO handler\n");
> > +               goto cleanup;
> > +       }
> > +
> > +       memset(&attr, 0, sizeof(attr));
> > +       attr.size =3D sizeof(attr);
> > +       attr.type =3D PERF_TYPE_SOFTWARE;
> > +       attr.config =3D PERF_COUNT_SW_DUMMY;
> > +       attr.sample_period =3D 1;
> > +       attr.disabled =3D 1;
> > +       attr.watermark =3D 1;
> > +       attr.context_switch =3D 1;
> > +       attr.wakeup_watermark =3D 1;
> > +
> > +       child =3D fork();
> > +       if (child =3D=3D 0)
> > +               do_child();
> > +       else if (child < 0) {
> > +               pr_debug("failed fork() %d\n", errno);
> > +               goto cleanup;
> > +       }
> > +
> > +       fd =3D 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?

> > +
> > +       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 =3D mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_SHA=
RED, fd, 0);
> > +       if (p =3D=3D 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.

- Kyle

> Thanks,
> Ian
>
> > +
> > +       ret =3D (sigio_count =3D=3D 1) ? TEST_OK : TEST_FAIL;
> > +
> > +cleanup:
> > +       if (p !=3D NULL)
> > +               munmap(p, 2 * page_size);
> > +
> > +       if (fd >=3D 0)
> > +               close(fd);
> > +
> > +       if (child > 0) {
> > +               kill(child, SIGTERM);
> > +               waitpid(child, NULL, 0);
> > +       }
> > +
> > +       if (previous_sigio !=3D SIG_ERR)
> > +               signal(SIGIO, previous_sigio);
> > +
> > +       return ret;
> > +}
> > +
> > +DEFINE_SUITE("Watermark signal handler", watermark_signal);
> > --
> > 2.34.1
> >