Re: [PATCH v2 bpf-next 1/3] bpf: Consolidate inner-map-compatible properties into bpf_types.h

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, May 28, 2020 at 11:31 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, May 26, 2020 at 10:54:26AM -0700, Andrii Nakryiko wrote:
> > On Fri, May 22, 2020 at 6:01 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > On Sat, May 23, 2020 at 12:22:48AM +0200, Daniel Borkmann wrote:
> > > > On 5/22/20 4:23 AM, Martin KaFai Lau wrote:
> > > > [...]
> > > > >   };

[...]


> > >
> > > >
> > > > Like explicit annotating via struct bpf_map_ops where everything is visible in one
> > > > single place where the map is defined:
> > > >
> > > > const struct bpf_map_ops array_map_ops = {
> > > >         .map_alloc_check = array_map_alloc_check,
> > > >         [...]
> > > >         .map_flags_fixed = BPF_MAP_IN_MAP_OK,
> > > > };
> > > I am not sure about adding it to bpf_map_ops instead of bpf_types.h.
> > > It will be easier to figure out what map types do not support MAP_IN_MAP (and
> > > other future map's fixed properties) in one place "bpf_types.h" instead of
> > > having to dig into each map src file.
> >
> > I'm 100% with Daniel here. If we are consolidating such things, I'd
> > rather have them in one place where differences between maps are
> > defined, which is ops. Despite an "ops" name, this seems like a
> > perfect place for specifying all those per-map-type properties and
> > behaviors. Adding flags into bpf_types.h just splits everything into
> > two places: bpf_types.h specifies some differences, while ops specify
> > all the other ones.
> >
> > Figuring out map-in-map support is just one of many questions one
> > might ask about differences between map types, I don't think that
> > justifies adding them to bpf_types.h. Grepping for struct bpf_map_ops
> > with search context (i.e., -A15 or something like that) should be
> > enough to get a quick glance at all possible maps and what they
> > define/override.
> >
> > It also feels like adding this as bool field for each aspect instead
> > of a collection of bits is cleaner and a bit more scalable. If we need
> > to add another property with some parameter/constant, or just enum,
> > defining one of few possible behaviors, it would be easier to just add
> > another field, instead of trying to cram that into u32. It also solves
> > your problem of "at the glance" view of map-in-map support features.
> > Just name that field unique enough to grep by it :)
> How about another way.  What patch 2 want is each map could have its own
> bpf_map_meta_equal().  Instead of adding 2 flags, add the bpf_map_meta_equal()
> as a ops to bpf_map_ops.  Each map supports to be used as an inner_map
> needs to set this ops.  Then it will be an opt-in.
> A default implementation can be provided for most maps' use.
> The maps (e.g. arraymap and other future maps) that has different requirement
> can implement its own.  For the existing maps, when we address those
> limitations (e.g. arraymap's gen_lookup) later,  we can then change its
> bpf_map_meta_equal.
>
> Thoughts?
>

I think that would work as well, I don't mind.

> >
> > >
> > > If the objective is to have the future map "consciously" opt-in, how about
> > > keeping the "BPF_MAP_TYPE" name as is but add a fixed_flags param as the
> > > earlier v1 and flip it from NO to OK flag.  It will be clear that,
> > > it is a decision that the new map needs to make instead of a quiet 0
> > > in "struct bpf_map_ops".
> > >
> > > For example,
> > > BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops, BPF_MAP_IN_MAP_OK)
> > > BPF_MAP_TYPE(BPF_MAP_TYPE_PROG_ARRAY, prog_array_map_ops, 0)
> > > BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops, BPF_MAP_IN_MAP_OK | BPF_MAP_IN_MAP_DYNAMIC_SIZE_OK)
> > >
> > > >
> > > > That way, if someone forgets to add .map_flags_fixed to a new map type, it's okay since
> > > > it's _safe_ to forget to add these flags (and okay to add in future uapi-wise) as opposed
> > > > to the other way round where one can easily miss the opt-out case and potentially crash
> > > > the machine worst case.