XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Stanislav Fomichev <sdf@google.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	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,
	bpf@vger.kernel.org
Subject: [xdp-hints] Re: [PATCH bpf-next v4 05/15] bpf: XDP metadata RX kfuncs
Date: Wed, 14 Dec 2022 10:43:51 -0800	[thread overview]
Message-ID: <CAKH8qBsB2epy_s-NFs3TEuNBhXHfq-WuNvAOM9t16SUe=V8RNA@mail.gmail.com> (raw)
In-Reply-To: <74e48fc9-8f5d-4183-9f39-c4587c74a74e@linux.dev>

On Tue, Dec 13, 2022 at 5:54 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index ca22e8b8bd82..de6279725f41 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2477,6 +2477,8 @@ void bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> >                                      struct net_device *netdev);
> >   bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev);
> >
> > +void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id);
> > +
>
> This probably requires an inline version for !CONFIG_NET.

Yeah, not sure why my confings didn't catch this :-(

> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index d434a994ee04..c3e501e3e39c 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -2097,6 +2097,13 @@ bool bpf_prog_map_compatible(struct bpf_map *map,
> >       if (fp->kprobe_override)
> >               return false;
> >
> > +     /* When tail-calling from a non-dev-bound program to a dev-bound one,
> > +      * XDP metadata helpers should be disabled. Until it's implemented,
> > +      * prohibit adding dev-bound programs to tail-call maps.
> > +      */
> > +     if (bpf_prog_is_dev_bound(fp->aux))
> > +             return false;
> > +
> >       spin_lock(&map->owner.lock);
> >       if (!map->owner.type) {
> >               /* There's no owner yet where we could check for
> > diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> > index f714c941f8ea..3b6c9023f24d 100644
> > --- a/kernel/bpf/offload.c
> > +++ b/kernel/bpf/offload.c
> > @@ -757,6 +757,29 @@ void bpf_dev_bound_netdev_unregister(struct net_device *dev)
> >       up_write(&bpf_devs_lock);
> >   }
> >
> > +void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id)
> > +{
> > +     const struct xdp_metadata_ops *ops;
> > +     void *p = NULL;
> > +
> > +     down_read(&bpf_devs_lock);
> > +     if (!prog->aux->offload || !prog->aux->offload->netdev)
>
> This happens when netdev is unregistered in the middle of bpf_prog_load and the
> bpf_offload_dev_match() will eventually fail during dev_xdp_attach()? A comment
> will be useful.

Right, that's the expectation - we load/verify the prog but it's
essentially un-attach-able.
Will try to clarify here.

> > +             goto out;
> > +
> > +     ops = prog->aux->offload->netdev->xdp_metadata_ops;
> > +     if (!ops)
> > +             goto out;
> > +
> > +     if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_TIMESTAMP))
> > +             p = ops->xmo_rx_timestamp;
> > +     else if (func_id == xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_HASH))
> > +             p = ops->xmo_rx_hash;
> > +out:
> > +     up_read(&bpf_devs_lock);
> > +
> > +     return p;
> > +}
> > +
> >   static int __init bpf_offload_init(void)
> >   {
> >       int err;
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 203d8cfeda70..e61fe0472b9b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15479,12 +15479,35 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >                           struct bpf_insn *insn_buf, int insn_idx, int *cnt)
> >   {
> >       const struct bpf_kfunc_desc *desc;
> > +     void *xdp_kfunc;
> >
> >       if (!insn->imm) {
> >               verbose(env, "invalid kernel function call not eliminated in verifier pass\n");
> >               return -EINVAL;
> >       }
> >
> > +     *cnt = 0;
> > +
> > +     if (xdp_is_metadata_kfunc_id(insn->imm)) {
> > +             if (!bpf_prog_is_dev_bound(env->prog->aux)) {
>
> The "xdp_is_metadata_kfunc_id() && (!bpf_prog_is_dev_bound() ||
> bpf_prog_is_offloaded())" test should have been done much earlier in
> add_kfunc_call(). Then the later stage of the verifier does not have to keep
> worrying about it like here.
>
> nit. may be rename xdp_is_metadata_kfunc_id() to bpf_dev_bound_kfunc_id() and
> hide the "!bpf_prog_is_dev_bound() || bpf_prog_is_offloaded()" test into
> bpf_dev_bound_kfunc_check(&env->log, env->prog).
>
> The change in fixup_kfunc_call could then become:
>
>         if (bpf_dev_bound_kfunc_id(insn->imm)) {
>                 xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog, insn->imm);
>                 /* ... */
>         }

Makes sense, ty!


> > +                     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;
> > +             }
> > +
> > +             /* fallback to default kfunc when not supported by netdev */
> > +     }
> > +
>
>

  reply	other threads:[~2022-12-14 18:44 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
2022-12-14  1:53   ` Martin KaFai Lau
2022-12-14 18:43     ` Stanislav Fomichev [this message]
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='CAKH8qBsB2epy_s-NFs3TEuNBhXHfq-WuNvAOM9t16SUe=V8RNA@mail.gmail.com' \
    --to=sdf@google.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=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