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.
> 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'.
> 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?
