On Mon, Jul 27, 2020 at 5:08 AM Shay Agroskin <shayagr@xxxxxxxxxx> wrote:
>
>
> Andrii Nakryiko <andriin@xxxxxx> 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@xxxxxx>
> > ---
> > 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;
> > }
> >
[...]
