Received-SPF: pass (google.com: domain of linux-kernel+bounces-132220-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 4 Apr 2024 17:49:19 -0500
From: David Vernet <void@manifault.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Yonghong Song <yonghong.song@linux.dev>, ast@kernel.org,
	daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev,
	song@kernel.org, john.fastabend@gmail.com, kpsingh@kernel.org,
	sdf@google.com, haoluo@google.com, jolsa@kernel.org,
	linux-kernel@vger.kernel.org, kernel-team@meta.com,
	bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Verify calling core kfuncs
 from BPF_PROG_TYPE_SYCALL
Message-ID: <20240404224919.GB521321@maniforge>
References: <20240404010308.334604-1-void@manifault.com>
 <20240404010308.334604-2-void@manifault.com>
 <36bb0747-bff4-4fad-93ca-dae406f14099@linux.dev>
 <20240404163316.GA385240@maniforge>
 <CAEf4Bza3rGMh=S7yb_Crf8v_SBTrp+NiKn0_frZnWpEi30DkPg@mail.gmail.com>
 <20240404223044.GA521321@maniforge>
 <CAEf4BzbnG8QufsLmsvwHFG2JKjUCNtbKtU8c5Cb56rWR+s8UBg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="KS4izKu7yfk4waCZ"
Content-Disposition: inline
In-Reply-To: <CAEf4BzbnG8QufsLmsvwHFG2JKjUCNtbKtU8c5Cb56rWR+s8UBg@mail.gmail.com>
User-Agent: Mutt/2.2.13 (2024-03-09)


--KS4izKu7yfk4waCZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 04, 2024 at 03:35:32PM -0700, Andrii Nakryiko wrote:

[...]

> > > > > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_=
success)
> > > > > will do duplicate work for *existing* bpf programs in their respe=
ctive
> > > > > files. I think we still prefer to have cgrp_kfunc_success tests
> > > > > in cgrp_kfunc.c to make it easy to cross check. But in order to
> > > > > remove duplicate work, one option is to make other non-RUN_TESTS
> > > > > programs in those files not auto-loaded and their corresponding
> > > > > prog_tests/*.c file need to explicitly enable loading the problem.
> > > >
> > > > Good point, and yes I agree with that approach of not auto-loading
> > > > non-RUN_TESTS programs. Considering that we have a  __success BTF t=
ag to
> > > > say, "this prog should successfully load", it seems odd that we'd a=
lso
> > > > automatically load and validate progs that _didn't_ specify that ta=
g as
> > > > well. At that point, I'm not sure what value the tag is bringing. A=
lso,
> > >
> > > Just more explicitness (if desired). Normally __success would be
> > > augmented by __msg() or __retval(). I'd feel uncomfortable just
> >
> > But __success really has no actual purpose, right? Isn't it identical to
> > if it's just left off? You don't need __success to specify __msg() or
> > __retval() right?
>=20
> right, it's just a more explicit documentation-like annotation, if you wi=
ll
>=20
> >
> > > silently skipping programs that are not marked with __success, as it
> > > would be too easy to accidentally forget to add it and not know that
> > > the BPF program is not tested.
> > >
> > > I'd say that RUN_TESTS-based programs should be kept separate from any
> > > other BPF programs that have a custom user-space testing part, though.
> >
> > IF we do go this way, maybe just a __skip or something tag would be
> > sufficient?
>=20
> if we go this way we wouldn't need __skip, but if we do not go, then
> sure, why not. But in general, __skip makes sense either way, I guess,
> I have no problem with it.

Sorry, by "if we go this way" what I meant was "if we continue to have
RUN_TESTS() run all progs by default." Given that we're doing that, it
sounds like we're on the same page page and that __skip is the way to
go.

>=20
> >
> > > About the patch itself. I don't really see much point in adding
> > > *_KFUNC_LOAD_TEST macros. They are used once or twice in total, while
> > > obscuring *what* is actually being tested. Unless you expect to add 5+
> > > more copies of them, I'd just inline them explicitly.
> >
> > It's not really important what's in the actual prog though -- the point
> > is that we're verifying we can invoke some kfuncs in a certain prog
> > type. But yes, it does obscure what's there, and I'm fine with
> > copy-pasting them if that's your preference. The reason I went with a
> > macro was to make it easy for us to quickly test new prog types as we
> > add support for them, or to add other negative testcases for unsafe prog
> > types. Right now we're just testing tracing progs.
>=20
> I'm always for less macro usage, if possible :)
>=20
> For the use case you are describing I'd just add static subprog that
> exercises all the kfuncs of interest, and then call this subprog from
> all the (explicitly defined) main entry program of desired program
> types

Will do for v2. Thanks!

--KS4izKu7yfk4waCZ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHQEARYKAB0WIQRBxU1So5MTLwphjdFZ5LhpZcTzZAUCZg8ubwAKCRBZ5LhpZcTz
ZBAJAP4xft/59FVfLsetXvVi/i02GQFWrdMZaheEq03ofwyJcAD1HS3/JG6N7HgM
tRJ9O7beU7xaYuNWkRLmJw/skFjMBQ==
=AXl+
-----END PGP SIGNATURE-----

--KS4izKu7yfk4waCZ--