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 03/15] bpf: Introduce device-bound XDP programs
Date: Wed, 14 Dec 2022 10:42:10 -0800	[thread overview]
Message-ID: <CAKH8qBsg1hYnkmurnSGCCzTFOzQrV4DKCw1gefgXNb6UN57+Vg@mail.gmail.com> (raw)
In-Reply-To: <94d8cd3a-fc07-88aa-94f8-6b08940a2087@linux.dev>

On Tue, Dec 13, 2022 at 3:25 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> > New flag BPF_F_XDP_DEV_BOUND_ONLY plus all the infra to have a way
> > to associate a netdev with a BPF program at load time.
> >
> > Some existing 'offloaded' routines are renamed to 'dev_bound' for
> > consistency with the rest.
> >
> > Also moved a bunch of code around to avoid forward declarations.
>
> There are too many things in one patch.  It becomes quite hard to follow, eg. I
> have to go back-and-forth a few times within this patch to confirm what change
> is just a move.  Please put the "moved a bunch of code around to avoid forward
> declarations" in one individual patch and also the
> "late_initcall(bpf_offload_init)" change in another individual patch.

Ugh, sorry, good point will definitely split more :-(

> [ ... ]
>
> > -int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> > +static int __bpf_offload_dev_netdev_register(struct bpf_offload_dev *offdev,
> > +                                          struct net_device *netdev)
> > +{
> > +     struct bpf_offload_netdev *ondev;
> > +     int err;
> > +
> > +     ondev = kzalloc(sizeof(*ondev), GFP_KERNEL);
> > +     if (!ondev)
> > +             return -ENOMEM;
> > +
> > +     ondev->netdev = netdev;
> > +     ondev->offdev = offdev;
> > +     INIT_LIST_HEAD(&ondev->progs);
> > +     INIT_LIST_HEAD(&ondev->maps);
> > +
> > +     err = rhashtable_insert_fast(&offdevs, &ondev->l, offdevs_params);
> > +     if (err) {
> > +             netdev_warn(netdev, "failed to register for BPF offload\n");
> > +             goto err_unlock_free;
> > +     }
> > +
> > +     if (offdev)
> > +             list_add(&ondev->offdev_netdevs, &offdev->netdevs);
> > +     return 0;
> > +
> > +err_unlock_free:
> > +     up_write(&bpf_devs_lock);
>
> No need to handle bpf_devs_lock in the "__" version of the register() helper?
> The goto label probably also needs another name, eg. "err_free".

Ah, not sure how I missed that, thanks!

> > +     kfree(ondev);
> > +     return err;
> > +}
> > +
>
> [ ... ]
>
> > +int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> >   {
> >       struct bpf_offload_netdev *ondev;
> >       struct bpf_prog_offload *offload;
> > @@ -87,7 +198,7 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> >           attr->prog_type != BPF_PROG_TYPE_XDP)
> >               return -EINVAL;
> >
> > -     if (attr->prog_flags)
> > +     if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
> >               return -EINVAL;
> >
> >       offload = kzalloc(sizeof(*offload), GFP_USER);
> > @@ -102,11 +213,25 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> >       if (err)
> >               goto err_maybe_put;
> >
> > +     prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
> > +
> >       down_write(&bpf_devs_lock);
> >       ondev = bpf_offload_find_netdev(offload->netdev);
> >       if (!ondev) {
> > -             err = -EINVAL;
> > -             goto err_unlock;
> > +             if (!bpf_prog_is_offloaded(prog->aux)) {
> > +                     /* When only binding to the device, explicitly
> > +                      * create an entry in the hashtable. See related
> > +                      * bpf_dev_bound_try_remove_netdev.
> > +                      */
> > +                     err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
> > +                     if (err)
> > +                             goto err_unlock;
> > +                     ondev = bpf_offload_find_netdev(offload->netdev);
> > +             }
> > +             if (!ondev) {
>
> nit.  A bit confusing because the "ondev = bpf_offload_find_netdev(...)" above
> should not fail but "!ondev" is tested again here.  I think the intention is to
> fail on the 'bpf_prog_is_offloaded() == true' case. May be:
>
>                 if (bpf_prog_is_offloaded(prog->aux)) {
>                         err = -EINVAL;
>                         goto err_unlock;
>                 }
>                 /* When only binding to the device, explicitly
>                  * ...
>                  */
>                 err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
>                 if (err)
>                         goto err_unlock;
>                 ondev = bpf_offload_find_netdev(offload->netdev);
>

Yeah, that looks better, thx!

> > +                     err = -EINVAL;
> > +                     goto err_unlock;
> > +             }
> >       }
> >       offload->offdev = ondev->offdev;
> >       prog->aux->offload = offload;
> > @@ -209,27 +334,28 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
> >       up_read(&bpf_devs_lock);
> >   }
> >
> > -static void __bpf_prog_offload_destroy(struct bpf_prog *prog)
> > +static void bpf_dev_bound_try_remove_netdev(struct net_device *dev)
> >   {
> > -     struct bpf_prog_offload *offload = prog->aux->offload;
> > -
> > -     if (offload->dev_state)
> > -             offload->offdev->ops->destroy(prog);
> > +     struct bpf_offload_netdev *ondev;
> >
> > -     /* Make sure BPF_PROG_GET_NEXT_ID can't find this dead program */
> > -     bpf_prog_free_id(prog, true);
> > +     if (!dev)
> > +             return;
> >
> > -     list_del_init(&offload->offloads);
> > -     kfree(offload);
> > -     prog->aux->offload = NULL;
> > +     ondev = bpf_offload_find_netdev(dev);
> > +     if (ondev && !ondev->offdev && list_empty(&ondev->progs))
>
> hmm....list_empty(&ondev->progs) is tested here but will it be empty? ...

Ugh, yeah, need to move that list_del_init(&offload->offloads) to
somewhere before bpf_dev_bound_try_remove_netdev.

> > +             __bpf_offload_dev_netdev_unregister(NULL, dev);
> >   }
> >
> > -void bpf_prog_offload_destroy(struct bpf_prog *prog)
> > +void bpf_prog_dev_bound_destroy(struct bpf_prog *prog)
> >   {
> > +     rtnl_lock();
> >       down_write(&bpf_devs_lock);
> > -     if (prog->aux->offload)
> > -             __bpf_prog_offload_destroy(prog);
> > +     if (prog->aux->offload) {
> > +             bpf_dev_bound_try_remove_netdev(prog->aux->offload->netdev);
>
> ... the "prog" here is still linked to ondev->progs, right?
> because __bpf_prog_dev_bound_destroy() is called later below.

Agreed, right.

> nit. May be the bpf_dev_bound_try_remove_netdev() should be folded/merged back
> into bpf_prog_dev_bound_destroy() to make things more clear.

Makes sense.

> > +             __bpf_prog_dev_bound_destroy(prog); > + }
> >       up_write(&bpf_devs_lock);
> > +     rtnl_unlock();
> >   }
>
> [ ... ]
>
> > +static int __init bpf_offload_init(void)
> > +{
> > +     int err;
> > +
> > +     down_write(&bpf_devs_lock);
>
> lock is probably not needed.

Sure, will drop.

> > +     err = rhashtable_init(&offdevs, &offdevs_params);
> > +     up_write(&bpf_devs_lock);
> > +
> > +     return err;
> > +}
> > +
> > +late_initcall(bpf_offload_init);
>
> [ ... ]
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 5d51999cba30..194f8116aad4 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9228,6 +9228,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
> >                       NL_SET_ERR_MSG(extack, "Using offloaded program without HW_MODE flag is not supported");
> >                       return -EINVAL;
> >               }
> > +             if (bpf_prog_is_dev_bound(new_prog->aux) && !bpf_offload_dev_match(new_prog, dev)) {
> > +                     NL_SET_ERR_MSG(extack, "Program bound to different device");
> > +                     return -EINVAL;
> > +             }
> >               if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) {
> >                       NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device");
> >                       return -EINVAL;
> > @@ -10813,6 +10817,7 @@ void unregister_netdevice_many_notify(struct list_head *head,
> >               /* Shutdown queueing discipline. */
> >               dev_shutdown(dev);
> >
> > +             bpf_dev_bound_netdev_unregister(dev);
>
> Does it matter if bpf_dev_bound_netdev_unregister(dev) is called before
> dev_xdp_uninstall(dev)?  Asking because it seems more logic to unregister dev
> after detaching xdp progs.

By running it first I was hoping to catch any possible issues. Agreed
that doing it after makes more sense, will move.

> >               dev_xdp_uninstall(dev);
> >
> >               netdev_offload_xstats_disable_all(dev);
>
>

  reply	other threads:[~2022-12-14 18:42 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 [this message]
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
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=CAKH8qBsg1hYnkmurnSGCCzTFOzQrV4DKCw1gefgXNb6UN57+Vg@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