From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: David Ahern <dsahern@gmail.com>, bpf <bpf@vger.kernel.org>,
Network Development <netdev@vger.kernel.org>,
Daniel Borkmann <borkmann@iogearbox.net>,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Lorenzo Bianconi <lorenzo@kernel.org>
Subject: Re: [PATCH bpf-next RFC 2/3] bpf: devmap dynamic map-value storage area based on BTF
Date: Tue, 2 Jun 2020 11:27:03 -0700 [thread overview]
Message-ID: <CAADnVQJDj_5i=g0S1UhxP1EUKwNUx1KuQ=V7y089+oy8rdnZ=g@mail.gmail.com> (raw)
In-Reply-To: <20200602090005.5a6eb50c@carbon>
On Tue, Jun 2, 2020 at 12:00 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Mon, 1 Jun 2020 14:30:12 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Fri, May 29, 2020 at 05:59:45PM +0200, Jesper Dangaard Brouer wrote:
> > > +
> > > +/* Expected BTF layout that match struct bpf_devmap_val */
> > > +static const struct expect layout[] = {
> > > + {BTF_KIND_INT, true, 0, 4, "ifindex"},
> > > + {BTF_KIND_UNION, false, 32, 4, "bpf_prog"},
> > > + {BTF_KIND_STRUCT, false, -1, -1, "storage"}
> > > +};
> > > +
> > > +static int dev_map_check_btf(const struct bpf_map *map,
> > > + const struct btf *btf,
> > > + const struct btf_type *key_type,
> > > + const struct btf_type *value_type)
> > > +{
> > > + struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> > > + u32 found_members_cnt = 0;
> > > + u32 int_data;
> > > + int off;
> > > + u32 i;
> > > +
> > > + /* Validate KEY type and size */
> > > + if (BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
> > > + return -EOPNOTSUPP;
> > > +
> > > + int_data = *(u32 *)(key_type + 1);
> > > + if (BTF_INT_BITS(int_data) != 32 || BTF_INT_OFFSET(int_data) != 0)
> > > + return -EOPNOTSUPP;
> > > +
> > > + /* Validate VALUE have layout that match/map-to struct bpf_devmap_val
> > > + * - With a flexible size of member 'storage'.
> > > + */
> > > +
> > > + if (BTF_INFO_KIND(value_type->info) != BTF_KIND_STRUCT)
> > > + return -EOPNOTSUPP;
> > > +
> > > + /* Struct/union members in BTF must not exceed (max) expected members */
> > > + if (btf_type_vlen(value_type) > ARRAY_SIZE(layout))
> > > + return -E2BIG;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(layout); i++) {
> > > + off = btf_find_expect_layout_offset(btf, value_type, &layout[i]);
> > > +
> > > + if (off < 0 && layout[i].mandatory)
> > > + return -EUCLEAN;
> > > +
> > > + if (off >= 0)
> > > + found_members_cnt++;
> > > +
> > > + /* Transfer layout config to map */
> > > + switch (i) {
> > > + case 0:
> > > + dtab->cfg.btf_offset.ifindex = off;
> > > + break;
> > > + case 1:
> > > + dtab->cfg.btf_offset.bpf_prog = off;
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > + }
> > > +
> > > + /* Detect if BTF/vlen have members that were not found */
> > > + if (btf_type_vlen(value_type) > found_members_cnt)
> > > + return -E2BIG;
> > > +
> > > + return 0;
> > > +}
> >
> > This layout validation looks really weird to me.
> > That layout[] array sort of complements BTF to describe the data,
> > but double describe of the layout feels like hack.
>
> This is the kind of feedback I'm looking for. I want to make the
> map-value more dynamic. It seems so old school to keep extending the
> map-value with a size and fixed binary layout, when we have BTF
> available. I'm open to input on how to better verify/parse/desc the
> expected BTF layout for kernel-code side.
>
> The patch demonstrates that this is possible, I'm open for changes.
> E.g. devmap is now extended with a bpf_prog, but most end-users will
> not be using this feature. Today they can use value_size=4 to avoid
> using this field. When we extend map-value again, then end-users are
> force into providing 'bpf_prog.fd' if they want to use the newer
> options. In this patch end-users don't need to provide 'bpf_prog' if
> they don't use it. Via BTF we can see this struct member can be skipped.
I think 'struct bpf_devmap_val' should be in uapi/bpf.h.
That's what it is and it will be extended with new fields at the end
just like all other structs in uapi/bpf.h
I don't think BTF can become a substitute for uapi
where uapi struct has to have all fields defined and backwards supported
by the kernel.
BTF is for flexible structs where fields may disappear.
BTF is there to define a meaning of a binary blob.
'struct bpf_devmap_val' is not such thing. It's very much known with
fixed fields and fixed meaning.
next prev parent reply other threads:[~2020-06-02 18:27 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-29 15:59 [PATCH bpf-next RFC 0/3] bpf: dynamic map-value config layout via BTF Jesper Dangaard Brouer
2020-05-29 15:59 ` [PATCH bpf-next RFC 1/3] bpf: move struct bpf_devmap_val out of UAPI Jesper Dangaard Brouer
2020-05-29 16:06 ` David Ahern
2020-05-29 16:28 ` Jesper Dangaard Brouer
2020-05-29 15:59 ` [PATCH bpf-next RFC 2/3] bpf: devmap dynamic map-value storage area based on BTF Jesper Dangaard Brouer
2020-05-29 16:39 ` Toke Høiland-Jørgensen
2020-06-02 8:59 ` Jesper Dangaard Brouer
2020-06-02 9:23 ` Toke Høiland-Jørgensen
2020-06-02 10:01 ` Jesper Dangaard Brouer
2020-05-30 7:19 ` Andrii Nakryiko
2020-05-30 14:36 ` Jesper Dangaard Brouer
2020-06-01 21:30 ` Alexei Starovoitov
2020-06-02 7:00 ` Jesper Dangaard Brouer
2020-06-02 18:27 ` Alexei Starovoitov [this message]
2020-06-03 9:11 ` Jesper Dangaard Brouer
2020-06-03 16:20 ` Alexei Starovoitov
2020-05-29 15:59 ` [PATCH bpf-next RFC 3/3] samples/bpf: change xdp_fwd to use new BTF config interface Jesper Dangaard Brouer
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='CAADnVQJDj_5i=g0S1UhxP1EUKwNUx1KuQ=V7y089+oy8rdnZ=g@mail.gmail.com' \
--to=alexei.starovoitov@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=borkmann@iogearbox.net \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=dsahern@gmail.com \
--cc=lorenzo@kernel.org \
--cc=netdev@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).