Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Shay Agroskin <shayagr@amazon.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	David Ahern <dsahern@gmail.com>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic
Date: Mon, 27 Jul 2020 11:51:35 -0700	[thread overview]
Message-ID: <CAEf4Bzb1_mQVxjmLEK0OFBdfnFi8To4fH-=kJTs8nz6xq7zUMw@mail.gmail.com> (raw)
In-Reply-To: <pj41zla6zl88le.fsf@ua97a68a4e7db56.ant.amazon.com>

On Mon, Jul 27, 2020 at 5:08 AM Shay Agroskin <shayagr@amazon.com> wrote:
>
>
> Andrii Nakryiko <andriin@fb.com> writes:
>
> > Further refactor XDP attachment code. dev_change_xdp_fd() is
> > split into two
> > parts: getting bpf_progs from FDs and attachment logic, working
> > with
> > bpf_progs. This makes attachment  logic a bit more
> > straightforward and
> > prepares code for bpf_xdp_link inclusion, which will share the
> > common logic.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  net/core/dev.c | 165
> >  +++++++++++++++++++++++++++----------------------
> >  1 file changed, 91 insertions(+), 74 deletions(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 7e753e248cef..abf573b2dcf4 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct
> > net_device *dev)
> >       }
> >  }
> >
> > -/**
> > - *   dev_change_xdp_fd - set or clear a bpf program for a
> > device rx path
> > - *   @dev: device
> > - *   @extack: netlink extended ack
> > - *   @fd: new program fd or negative value to clear
> > - *   @expected_fd: old program fd that userspace expects to
> > replace or clear
> > - *   @flags: xdp-related flags
> > - *
> > - *   Set or clear a bpf program for a device
> > - */
> > -int dev_change_xdp_fd(struct net_device *dev, struct
> > netlink_ext_ack *extack,
> > -                   int fd, int expected_fd, u32 flags)
> > +static int dev_xdp_attach(struct net_device *dev, struct
> > netlink_ext_ack *extack,
> > +                       struct bpf_prog *new_prog, struct
> > bpf_prog *old_prog,
> > +                       u32 flags)
> >  {
> > -     const struct net_device_ops *ops = dev->netdev_ops;
> > -     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > -     bool offload = mode == XDP_MODE_HW;
> > -     u32 prog_id, expected_id = 0;
> > -     struct bpf_prog *prog;
> > +     struct bpf_prog *cur_prog;
> > +     enum bpf_xdp_mode mode;
> >       bpf_op_t bpf_op;
> >       int err;
> >
> >       ASSERT_RTNL();
> >
> > -     bpf_op = dev_xdp_bpf_op(dev, mode);
> > -     if (!bpf_op) {
> > -             NL_SET_ERR_MSG(extack, "underlying driver does not
> > support XDP in native mode");
> > -             return -EOPNOTSUPP;
> > +     /* just one XDP mode bit should be set, zero defaults to
> > SKB mode */
> > +     if (hweight32(flags & XDP_FLAGS_MODES) > 1) {
>
> Not sure if it's more efficient but running
>     if ((flags & XDP) & ((flags & XDP) - 1) != 0)
>
> returns whether a number is a multiple of 2.
> Should be equivalent to what you checked with hweight32. It is
> less readable though. Just thought I'd throw that in.

so I just preserved what is there in netlink-handling code. It also is
not a performance-critical part. What you propose might work, but
using hweight32 is more explicit about allowing zero or one bits set.

> Taken from
> https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
>
> > +             NL_SET_ERR_MSG(extack, "Only one XDP mode flag can
> > be set");
> > +             return -EINVAL;
> > +     }
> > +     /* old_prog != NULL implies XDP_FLAGS_REPLACE is set */
> > +     if (old_prog && !(flags & XDP_FLAGS_REPLACE)) {
> > +             NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not
> > specified");
> > +             return -EINVAL;
> >       }
> >

[...]