XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: David Vernet <void@manifault.com>
To: Stanislav Fomichev <sdf@google.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
	haoluo@google.com, jolsa@kernel.org,
	David Ahern <dsahern@gmail.com>, Jakub Kicinski <kuba@kernel.org>,
	Willem de Bruijn <willemb@google.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	Alexander Lobakin <alexandr.lobakin@intel.com>,
	Magnus Karlsson <magnus.karlsson@gmail.com>,
	Maryam Tahhan <mtahhan@redhat.com>,
	xdp-hints@xdp-project.net, netdev@vger.kernel.org
Subject: [xdp-hints] Re: [PATCH bpf-next v4 05/15] bpf: XDP metadata RX kfuncs
Date: Tue, 13 Dec 2022 15:45:58 -0600	[thread overview]
Message-ID: <Y5jylgvFu7WCqiIU@maniforge.lan> (raw)
In-Reply-To: <CAKH8qBtU6_aeVrgfUVEyOW2JrGRWf4o=d=H3hnM+aD_UW-gcEA@mail.gmail.com>

On Tue, Dec 13, 2022 at 12:42:30PM -0800, Stanislav Fomichev wrote:

[...]

> > We don't usually export function signatures like this for kfuncs as
> > nobody in the main kernel should be linking against it. See [0].
> >
> > [0]: https://docs.kernel.org/bpf/kfuncs.html#creating-a-wrapper-kfunc
> 
> Oh, thanks, that's very helpful. As you might have guessed, I've added
> those signatures to make the compiler happy :-(

No problem, and yeah, it's a pain :-( It would be really nice if we
could do something like this:

#define __kfunc __attribute__((nowarn("Wmissing-protoypes")))

But that attribute doesn't exist.

> > > +     if (xdp_is_metadata_kfunc_id(insn->imm)) {
> > > +             if (!bpf_prog_is_dev_bound(env->prog->aux)) {
> > > +                     verbose(env, "metadata kfuncs require device-bound program\n");
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             if (bpf_prog_is_offloaded(env->prog->aux)) {
> > > +                     verbose(env, "metadata kfuncs can't be offloaded\n");
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog, insn->imm);
> > > +             if (xdp_kfunc) {
> > > +                     insn->imm = BPF_CALL_IMM(xdp_kfunc);
> > > +                     return 0;
> > > +             }
> >
> > Per another comment, should these xdp kfuncs use special_kfunc_list, or
> > some other variant that lives in verifier.c? I'll admit that I'm not
> > quite following why you wouldn't need to do the find_kfunc_desc() call
> > below, so apologies if I'm just totally off here.
> 
> Here I'm trying to short-circuit that generic verifier handling and do
> kfunc resolving myself, so not sure. Will comment about
> special_kfunc_list below.

Understood -- if it's totally separate then do what you need to do. My
only "objection" is that it's a bit sad when we have divergent /
special-case handling in the verifier between all these different kfunc
/ helpers / etc, but I think it's inevitable until we do a larger
refactoring.  It's contained to fixup_kfunc_call() at least, so IMO it's
fine.

[...]

> 
> > > +
> > > +             /* fallback to default kfunc when not supported by netdev */
> > > +     }
> > > +
> > >       /* insn->imm has the btf func_id. Replace it with
> > >        * an address (relative to __bpf_call_base).
> > >        */
> > > @@ -15495,7 +15518,6 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> > >               return -EFAULT;
> > >       }
> > >
> > > -     *cnt = 0;
> > >       insn->imm = desc->imm;
> > >       if (insn->off)
> > >               return 0;
> > > @@ -16502,6 +16524,11 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > >       if (tgt_prog) {
> > >               struct bpf_prog_aux *aux = tgt_prog->aux;
> > >
> > > +             if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
> > > +                     bpf_log(log, "Replacing device-bound programs not supported\n");
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > >               for (i = 0; i < aux->func_info_cnt; i++)
> > >                       if (aux->func_info[i].type_id == btf_id) {
> > >                               subprog = i;
> > > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > index 844c9d99dc0e..b0d4080249d7 100644
> > > --- a/net/core/xdp.c
> > > +++ b/net/core/xdp.c
> > > @@ -4,6 +4,7 @@
> > >   * Copyright (c) 2017 Jesper Dangaard Brouer, Red Hat Inc.
> > >   */
> > >  #include <linux/bpf.h>
> > > +#include <linux/btf_ids.h>
> > >  #include <linux/filter.h>
> > >  #include <linux/types.h>
> > >  #include <linux/mm.h>
> > > @@ -709,3 +710,46 @@ struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf)
> > >
> > >       return nxdpf;
> > >  }
> > > +
> > > +noinline int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp)
> > > +{
> > > +     return -EOPNOTSUPP;
> > > +}
> > > +
> > > +noinline int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > +{
> > > +     return -EOPNOTSUPP;
> > > +}
> >
> > I don't _think_ noinline should be necessary here given that the
> > function is global, though tbh I'm not sure if leaving it off will break
> > LTO. We currently don't use any attributes like this on other kfuncs
> > (e.g. [1]), but maybe we should?
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/helpers.c#n2034
> 
> Hm, I guess since I'm not really directly calling these anywhere,
> there is no chance they are going to be inlined? Will try to drop and
> see what happens..

Yeah, if it's a global symbol I think you should be OK. Again, we need
to figure out the story for LTO though. Later on I think we should add a
__kfunc macro which handles this invisibly for all kfunc definitions.

> > > +
> > > +BTF_SET8_START(xdp_metadata_kfunc_ids)
> > > +#define XDP_METADATA_KFUNC(name, str) BTF_ID_FLAGS(func, str, 0)
> >
> > IMO 'str' isn't the right parameter name here given that it's the actual
> > symbol and is not a string. What about _func or _symbol instead? Also
> > IMO 'name' is a bit misleading -- I'd go with something like '_enum'. I
> > wish there were a way for the preprocessor to auto-uppercase so you
> > could just define a single field that was used both for defining the
> > enum and for defining the symbol name.
> 
> How about I do the following:
> 
> enum {
> #define XDP_METADATA_KFUNC(name, _) name,
> XDP_METADATA_KFUNC_xxx
> #undef XDP_METADATA_KFUNC
> MAX_XDP_METADATA_KFUNC,
> };

Looks good!

> 
> And then this in the .c file:
> 
> BTF_SET8_START(xdp_metadata_kfunc_ids)
> #define XDP_METADATA_KFUNC(_, name) BTF_ID_FLAGS(func, name, 0)
> XDP_METADATA_KFUNC_xxx
> #undef XDP_METADATA_KFUNC
> BTF_SET8_END(xdp_metadata_kfunc_ids)

Here as well.

> 
> Should be a bit more clear what and where I use? Otherwise, using
> _func might seem a bit confusing in:
> #define XDP_METADATA_KFUNC(_enum, _func) BTF_ID_FLAGS(func, _func, 0)
> 
> The "func, _func" part. Or maybe that's fine.. WDYT?

LGTM, thanks!

> > > +XDP_METADATA_KFUNC_xxx
> > > +#undef XDP_METADATA_KFUNC
> > > +BTF_SET8_END(xdp_metadata_kfunc_ids)
> > > +
> > > +static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
> > > +     .owner = THIS_MODULE,
> > > +     .set   = &xdp_metadata_kfunc_ids,
> > > +};
> > > +
> > > +BTF_ID_LIST(xdp_metadata_kfunc_ids_unsorted)
> > > +#define XDP_METADATA_KFUNC(name, str) BTF_ID(func, str)
> > > +XDP_METADATA_KFUNC_xxx
> > > +#undef XDP_METADATA_KFUNC
> > > +
> > > +u32 xdp_metadata_kfunc_id(int id)
> > > +{
> > > +     /* xdp_metadata_kfunc_ids is sorted and can't be used */
> > > +     return xdp_metadata_kfunc_ids_unsorted[id];
> > > +}
> > > +
> > > +bool xdp_is_metadata_kfunc_id(u32 btf_id)
> > > +{
> > > +     return btf_id_set8_contains(&xdp_metadata_kfunc_ids, btf_id);
> > > +}
> >
> > The verifier already has a notion of "special kfuncs" via a
> > special_kfunc_list that exists in verifier.c. Maybe we should be using
> > that given that is only used in the verifier anyways? OTOH, it's nice
> > that all of the complexity of e.g. accounting for #ifdef CONFIG_NET is
> > contained here, so I also like your approach. It just seems like a
> > divergence from how things are being done for other kfuncs so I figured
> > it was worth discussing.
> 
> Yeah, idk, I've tried not to add more to the already huge verifier.c file :-(
> If we were to put everything into verifier.c, I'd still need some
> extra special_xdp_kfunc_list for those xdp kfuncs to be able to
> distinguish them from the rest...
> So yeah, not sure, I'd prefer to keep everything in xdp.c and not
> pollute the more generic verifier.c, but I'm fine either way. LMK if
> you feel strongly about it, can move.

IMO not polluting the already enormous verifier.c is definitely the
right thing to do -- especially for kfuncs like this which are going to
be defined throughout the kernel. So yeah, you can keep what you have.
And maybe at some point we should pull more logic out of verifier.c and
into the locations where the kfuncs are implemented as you're doing
here.

  reply	other threads:[~2022-12-13 21:46 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-13  2:35 [xdp-hints] [PATCH bpf-next v4 00/15] xdp: hints via kfuncs Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 01/15] bpf: Document XDP RX metadata Stanislav Fomichev
2022-12-13 16:37   ` [xdp-hints] " David Vernet
2022-12-13 20:42     ` Stanislav Fomichev
2022-12-14 10:34       ` Toke Høiland-Jørgensen
2022-12-14 18:42         ` Stanislav Fomichev
2022-12-14 23:46           ` Toke Høiland-Jørgensen
2022-12-15  3:48             ` Stanislav Fomichev
2022-12-15 14:04               ` Toke Høiland-Jørgensen
2022-12-14 23:46   ` Toke Høiland-Jørgensen
2022-12-17  4:20   ` kernel test robot
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 02/15] bpf: Rename bpf_{prog,map}_is_dev_bound to is_offloaded Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 03/15] bpf: Introduce device-bound XDP programs Stanislav Fomichev
2022-12-13 23:25   ` [xdp-hints] " Martin KaFai Lau
2022-12-14 18:42     ` Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 04/15] selftests/bpf: Update expected test_offload.py messages Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 05/15] bpf: XDP metadata RX kfuncs Stanislav Fomichev
2022-12-13 17:00   ` [xdp-hints] " David Vernet
2022-12-13 20:42     ` Stanislav Fomichev
2022-12-13 21:45       ` David Vernet [this message]
2022-12-14  1:53   ` Martin KaFai Lau
2022-12-14 18:43     ` Stanislav Fomichev
2022-12-14 10:54   ` Toke Høiland-Jørgensen
2022-12-14 18:43     ` Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 07/15] veth: Introduce veth_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 08/15] veth: Support RX XDP metadata Stanislav Fomichev
2022-12-13 15:55   ` [xdp-hints] " Jesper Dangaard Brouer
2022-12-13 20:42     ` Stanislav Fomichev
2022-12-14  9:48       ` Jesper Dangaard Brouer
2022-12-14 10:47         ` Toke Høiland-Jørgensen
2022-12-14 18:09           ` Martin KaFai Lau
2022-12-14 18:44             ` Stanislav Fomichev
2022-12-13  2:35 ` [xdp-hints] [PATCH bpf-next v4 09/15] selftests/bpf: Verify xdp_metadata xdp->af_xdp path Stanislav Fomichev
2022-12-13  2:36 ` [xdp-hints] [PATCH bpf-next v4 10/15] net/mlx4_en: Introduce wrapper for xdp_buff Stanislav Fomichev
2022-12-13  8:56   ` [xdp-hints] " Tariq Toukan
2022-12-13  2:36 ` [xdp-hints] [PATCH bpf-next v4 11/15] net/mlx4_en: Support RX XDP metadata Stanislav Fomichev
2022-12-13  8:56   ` [xdp-hints] " Tariq Toukan
2022-12-13  2:36 ` [xdp-hints] [PATCH bpf-next v4 12/15] xsk: Add cb area to struct xdp_buff_xsk Stanislav Fomichev
2022-12-13  2:36 ` [xdp-hints] [PATCH bpf-next v4 13/15] net/mlx5e: Introduce wrapper for xdp_buff Stanislav Fomichev
2022-12-13  2:36 ` [xdp-hints] [PATCH bpf-next v4 14/15] net/mlx5e: Support RX XDP metadata Stanislav Fomichev
2022-12-13  2:36 ` [xdp-hints] [PATCH bpf-next v4 15/15] selftests/bpf: Simple program to dump XDP RX metadata Stanislav Fomichev

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

  List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/

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

  git send-email \
    --in-reply-to=Y5jylgvFu7WCqiIU@maniforge.lan \
    --to=void@manifault.com \
    --cc=alexandr.lobakin@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=magnus.karlsson@gmail.com \
    --cc=martin.lau@linux.dev \
    --cc=mtahhan@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=willemb@google.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=yhs@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
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