Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 3741 bytes --]

On Wed, Feb 28, 2024 at 07:40:33PM +0200, Eduard Zingerman wrote:
> On Wed, 2024-02-28 at 11:23 -0600, David Vernet wrote:
> > On Tue, Feb 27, 2024 at 10:45:50PM +0200, Eduard Zingerman wrote:
> > > Enforce the following existing limitation on struct_ops programs based
> > > on kernel BTF id instead of program-local BTF id:
> > > 
> > >     struct_ops BPF prog can be re-used between multiple .struct_ops &
> > >     .struct_ops.link as long as it's the same struct_ops struct
> > >     definition and the same function pointer field
> > 
> > Am I correct in understanding the code that the prog also has to be at the same
> > offset in the new type?
> 
> Yes, but after this patch it would be offset in current kernel BTF type,
> not local BTF type.

Yes, indeed. I didn't mean to imply that your patch was changing that. I was
asking more generally -- sorry for the confusion.

> > So if we have for example:
> > 
> > SEC("struct_ops/test")
> > int BPF_PROG(foo) { ... }
> > 
> > struct some_ops___v1 {
> > 	int (*test)(void);
> > };
> > 
> > struct some_ops___v2 {
> > 	int (*init)(void);
> > 	int (*test)(void);
> > };
> 
> From pov of kernel BTF there is only one 'struct some_ops'.

Ack

> > Then this wouldn't work? If so, would it be possible for libbpf to do something
> > like invisibly duplicate the prog and create a separate one for each struct_ops
> > map where it's encountered? It feels like a rather awkward restriction to
> > impose given that the idea behind the feature is to enable loading one of
> > multiple possible definitions of a struct_ops type. 
> 
> In combination with the next patch, the idea is to not assign offset
> in struct_ops maps which have autocreate == false.
> 
> If object corresponding to program above would be opened and
> autocreate would be disabled either for some_ops___v1 or some_ops___v2
> before load, the program 'test' would get it's offset entry only from
> one map. Thus no program duplication would be necessary.
> 
> For example, see test case in patch #6:
> 
>     struct bpf_testmod_ops___v1 {
>     	int (*test_1)(void);
>     };
> 
>     struct bpf_testmod_ops___v2 {
>     	int (*test_1)(void);
>     	int (*does_not_exist)(void);
>     };
> 
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v1 testmod_1 = {
>     	.test_1 = (void *)test_1
>     };
> 
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v2 testmod_2 = {
>     	.test_1 = (void *)test_1,
>     	.does_not_exist = (void *)test_2
>     };
> 
> 
> static void can_load_partial_object(void)
> {
> 	...
> 	skel = struct_ops_autocreate__open_opts(&opts);
> 	bpf_program__set_autoload(skel->progs.test_2, false);
> 	bpf_map__set_autocreate(skel->maps.testmod_2, false);
> 	struct_ops_autocreate__load(skel);
>         ...
> }
> 
> This should handle your example as well.
> Do you find this sufficient or would you still like to have implicit
> program duplication logic?

It's definitely fine for now, but IMO it's something to keep in mind for the
future as a usability improvement. Ideally libbpf could internally handle just
creating and loading the type that's actually present on the system, and handle
applying the prog to the correct map, etc on the caller's behalf. Given that
there's only ever a single instance of a struct_ops type on the system, this
seems like a reasonable feature for the library to provide.

Note that this doesn't necessarily require duplicating the prog either, if
libbpf can instead _deduplicate_ the struct_ops maps to only create and load
the one that matches the type on the system.

Not a blocker by any means, but possibly a nice to have down the road.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]