BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Martin KaFai Lau <kafai@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Kernel Team <kernel-team@fb.com>,
	Networking <netdev@vger.kernel.org>, Andrey Ignatov <rdna@fb.com>
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: Consolidate inner-map-compatible properties into bpf_types.h
Date: Fri, 29 May 2020 13:40:46 -0700
Message-ID: <CAEf4BzbpwfFPUruwdbcarTeT_7pcxNw5PPO0RE81QLvJxkXOBw@mail.gmail.com> (raw)
In-Reply-To: <20200529063054.edz7qfiqgfgjzj43@kafai-mbp>

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.

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22  2:23 [PATCH v2 bpf-next 0/3] bpf: Allow inner map with different max_entries Martin KaFai Lau
2020-05-22  2:23 ` [PATCH v2 bpf-next 1/3] bpf: Consolidate inner-map-compatible properties into bpf_types.h Martin KaFai Lau
2020-05-22 22:22   ` Daniel Borkmann
2020-05-23  1:00     ` Martin KaFai Lau
2020-05-26 17:54       ` Andrii Nakryiko
2020-05-29  6:30         ` Martin KaFai Lau
2020-05-29 20:40           ` Andrii Nakryiko [this message]
2020-05-22  2:23 ` [PATCH v2 bpf-next 2/3] bpf: Relax the max_entries check for inner map Martin KaFai Lau
2020-05-22 22:26   ` Daniel Borkmann
2020-05-22  2:23 ` [PATCH v2 bpf-next 3/3] bpf: selftests: Add test for different inner map size Martin KaFai Lau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEf4BzbpwfFPUruwdbcarTeT_7pcxNw5PPO0RE81QLvJxkXOBw@mail.gmail.com \
    --to=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kafai@fb.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=rdna@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git