Received-SPF: pass (google.com: domain of linux-kernel+bounces-31419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240119001352.9396-1-khuey@kylehuey.com> <20240119001352.9396-5-khuey@kylehuey.com>
In-Reply-To: <20240119001352.9396-5-khuey@kylehuey.com>
From: Song Liu <song@kernel.org>
Date: Fri, 19 Jan 2024 09:19:31 -0800
Message-ID: <CAPhsuW7Qr_sDDDR_PLZsLKHFM+KHScwonKVvprTWVbK=dEZJ4A@mail.gmail.com>
Subject: Re: [PATCH v4 4/4] selftest/bpf: Test a perf bpf program that
 suppresses side effects.
To: Kyle Huey <me@kylehuey.com>
Cc: Kyle Huey <khuey@kylehuey.com>, linux-kernel@vger.kernel.org, 
	Andrii Nakryiko <andrii.nakryiko@gmail.com>, Jiri Olsa <jolsa@kernel.org>, 
	Namhyung Kim <namhyung@kernel.org>, Marco Elver <elver@google.com>, 
	Yonghong Song <yonghong.song@linux.dev>, "Robert O'Callahan" <robert@ocallahan.org>, 
	Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, 
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Mykola Lysenko <mykolal@fb.com>, 
	Shuah Khan <shuah@kernel.org>, bpf@vger.kernel.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 18, 2024 at 4:14=E2=80=AFPM Kyle Huey <me@kylehuey.com> wrote:
>

Acked-by: Song Liu <song@kernel.org>

with a couple nitpicks below.

[...]
> +int sigio_count, sigtrap_count;
> +
> +static void handle_sigio(int sig __always_unused)
> +{
> +       ++sigio_count;
> +}
> +
> +static void handle_sigtrap(int signum __always_unused,
> +                          siginfo_t *info,
> +                          void *ucontext __always_unused)
> +{
> +       ASSERT_EQ(info->si_code, TRAP_PERF, "wrong si_code");

nit: I would just call it "si_code", not "wrong si_code".

> +       ++sigtrap_count;
> +}
[...]
> +       if (!ASSERT_OK(sigaction(SIGTRAP, &action, &previous_sigtrap), "s=
igaction"))
> +               return;
> +
> +       previous_sigio =3D signal(SIGIO, handle_sigio);
> +       if (!ASSERT_NEQ(previous_sigio, SIG_ERR, "signal"))
> +               goto cleanup;

nit: If we goto cleanup here, we will do

       signal(SIGIO, SIG_ERR);

This is a no-op, so it is not a real issue. But it is not very clean.

[...]