On Mon, Jul 27, 2020 at 5:08 AM Shay Agroskin [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

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 <redacted>
---
 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

quoted

+             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;
      }

[...]