On Tue, Jun 24, 2025 at 5:15 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2025-06-24 at 17:11 -0700, Alexei Starovoitov wrote:
>
> [...]
>
> > > enum bpf_features {
> > > - __MAX_BPF_FEAT = 0,
> > > + BPF_FEAT_RDONLY_CAST_TO_VOID = 0,
> > > + __MAX_BPF_FEAT = 1,
> >
> > and the idea is to manually adjust it every time?!
> > That's way too much churn.
> > Either remove it or keep it without assignment.
> > Just as __MAX_BPF_FEAT. Like similar thing in enum /.
>
> I probably did not understand your previous message:
>
> > > +enum bpf_features {
> > > + BPF_FEAT_RDONLY_CAST_TO_VOID = 0,
> > > + BPF_FEAT_TOTAL,
> >
> > I don't see the value of 'total', but not strongly against it.
> > But pls be consistent with __MAX_BPF_CMD, __MAX_BPF_MAP_TYPE, ...
> > Say, __MAX_BPF_FEAT ?
> >
> >
> > Also it's better to introduce this enum in some earlier patch,
> > and then always add BTF_FEAT_... to this enum
> > in the same patch that adds the feature to make
> > sure backports won't screw it up.
> > Another rule should be to always assign a number to it.
>
>
> Specifically: "Another rule should be to always assign a number to it."
> The BPF_FEAT_RDONLY_CAST_TO_VOID already had a number, so I assumed
> you were talking about __MAX_BPF_FEAT.
> What did you mean?
I mean to add " = 123," to actual features, so when they're
backported the number stays the same.
Not to __MAX_BPF_FEAT.
I doubt it matters though,
since bpf progs suppose to use
bpf_core_enum_value_exists(enum bpf_features, name)
that doesn't care about the actual id.
In bpf helpers we got burned by broken backports and added
constants to ___BPF_FUNC_MAPPER macro.
Here I don't see it ever matter.
Just like I don't think __MAX_BPF_FEAT is needed,
but if we follow old steps, then let's do both __MAX_BPF_FEAT
without number and every feature with the number.
The end result will look like bpf_link_type.
