Received-SPF: pass (google.com: domain of linux-kernel+bounces-77277-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
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>
 <CAP-5=fVWUAWa4vZ7_Zb_3DjaMiTGT6grfFO2j_kOFbuZemJZTA@mail.gmail.com> <CAP045Arp+1j_W_iiHWwOGRCvvXCg3pC=gH5WMho-RpQXi4H1xg@mail.gmail.com>
In-Reply-To: <CAP045Arp+1j_W_iiHWwOGRCvvXCg3pC=gH5WMho-RpQXi4H1xg@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Thu, 22 Feb 2024 11:44:29 -0800
Message-ID: <CAP-5=fXh0OunzyZdky9BCqA1QfsqwPifCbbFRpsB5QY+5DP6+A@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf test: Test FASYNC with watermark wakeups.
To: Kyle Huey <me@kylehuey.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 Thu, Feb 22, 2024 at 10:41=E2=80=AFAM Kyle Huey <me@kylehuey.com> wrote:
>
> On Thu, Feb 22, 2024 at 10:33=E2=80=AFAM Ian Rogers <irogers@google.com> =
wrote:
> >
> > On Thu, Feb 22, 2024 at 9:55=E2=80=AFAM Kyle Huey <me@kylehuey.com> wro=
te:
> > >
> > > On Wed, Feb 21, 2024 at 10:36=E2=80=AFAM Ian Rogers <irogers@google.c=
om> wrote:
> > > >
> > > > 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/b=
uiltin-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 break=
points using the
> > > > > diff --git a/tools/perf/tests/watermark_signal.c b/tools/perf/tes=
ts/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 =3D 0; i < 20; ++i)
> > > > > +               sleep(1);
> > > > > +
> > > > > +       exit(0);
> > > > > +}
> > > > > +
> > > > > +static int test__watermark_signal(struct test_suite *test __mayb=
e_unused, 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.conf=
ig);
> > > > > +               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 olde=
r
> > > > 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 =3D (errno =3D=3D EINVAL) ? TEST_SKIP : TEST_FAIL;
>
> Ah, I see. The issue I am trying to fix is not that setting FASYNC is
> rejected with an error code, it's that it doesn't actually send the IO
> signal when the watermark is reached.

That's unfortunate. Perhaps similar to errno you could use uname,
return skip for failures on older kernels and fail on newer kernels.

Thanks,
Ian

> - Kyle
>
> > > > > +
> > > > > +       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, M=
AP_SHARED, 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", err=
no);
> > > > > +               goto cleanup;
> > > > > +       }
> > > > > +
> > > > > +       sleep(30);
> > > >
> > > > This is kind of a painful wait, could it be less? No sleeps would b=
e 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 =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
> > > > >