diff mbox series

[ovs-dev,v2] netdev-afxdp: Allow using user's prog instead of libxdp default prog

Message ID 20221214132434.70210-1-wanjunjie@bytedance.com
State Changes Requested
Headers show
Series [ovs-dev,v2] netdev-afxdp: Allow using user's prog instead of libxdp default prog | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Wan Junjie Dec. 14, 2022, 1:24 p.m. UTC 
Libxdp will load an embeded program by default. And OVS will
remove the program when it destroy the xsks. This is not a
good experience if we pre-load an xdp program. Every time we
restart OVS, it will remove the program.

Add an option 'inhibit', so we can disable the default program
load and remove. Use command for a afxdp type interface like below:
ovs-vsctl set int xxx options:inhibit=true

--
v2: fix some code missing

Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
---
 lib/netdev-afxdp.c         | 45 +++++++++++++++++++++++++++-----------
 lib/netdev-linux-private.h |  3 +++
 2 files changed, 35 insertions(+), 13 deletions(-)

Comments

0-day Robot Dec. 14, 2022, 1:40 p.m. UTC | #1 
Bleep bloop.  Greetings Wan Junjie, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Wan Junjie <wanjunjie@bytedance.com> needs to sign off.
Lines checked: 191, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ilya Maximets June 16, 2023, 3:26 p.m. UTC | #2 
On 12/14/22 14:24, Wan Junjie wrote:
> Libxdp will load an embeded program by default. And OVS will
> remove the program when it destroy the xsks. This is not a
> good experience if we pre-load an xdp program. Every time we
> restart OVS, it will remove the program.
> 
> Add an option 'inhibit', so we can disable the default program
> load and remove. Use command for a afxdp type interface like below:
> ovs-vsctl set int xxx options:inhibit=true
> 
> --
> v2: fix some code missing
> 
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> ---
>  lib/netdev-afxdp.c         | 45 +++++++++++++++++++++++++++-----------
>  lib/netdev-linux-private.h |  3 +++
>  2 files changed, 35 insertions(+), 13 deletions(-)

Hi, Wan.  Thanks for the patch!  It seems to miss some essential parts
though.  In order for the interface to work with an XDP program, the
socket fd has to be added into xsks_map of that program.  This should
be done with bpf_map_update_elem().  However, there is no code in this
patch that would get an xsks_map fd and update the map.  This way the
interface will not be able to receive any traffic.

Or am I missing something?

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index ca3f2431e..e43353cc0 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -95,7 +95,8 @@  BUILD_ASSERT_DECL(PROD_NUM_DESCS == CONS_NUM_DESCS);
 static struct xsk_socket_info *xsk_configure(int ifindex, int xdp_queue_id,
                                              enum afxdp_mode mode,
                                              bool use_need_wakeup,
-                                             bool report_socket_failures);
+                                             bool report_socket_failures,
+                                             bool inhibit);
 static void xsk_remove_xdp_program(uint32_t ifindex, enum afxdp_mode);
 static void xsk_destroy(struct xsk_socket_info *xsk);
 static int xsk_configure_all(struct netdev *netdev);
@@ -334,7 +335,8 @@  xsk_configure_umem(void *buffer, uint64_t size)
 static struct xsk_socket_info *
 xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex,
                      uint32_t queue_id, enum afxdp_mode mode,
-                     bool use_need_wakeup, bool report_socket_failures)
+                     bool use_need_wakeup, bool report_socket_failures,
+                     bool inhibit)
 {
     struct xsk_socket_config cfg;
     struct xsk_socket_info *xsk;
@@ -348,6 +350,9 @@  xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex,
     cfg.rx_size = CONS_NUM_DESCS;
     cfg.tx_size = PROD_NUM_DESCS;
     cfg.libbpf_flags = 0;
+    if (inhibit) {
+        cfg.libbpf_flags = XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD;
+    }
     cfg.bind_flags = xdp_modes[mode].bind_flags;
     cfg.xdp_flags = xdp_modes[mode].xdp_flags | XDP_FLAGS_UPDATE_IF_NOEXIST;
 
@@ -413,7 +418,7 @@  xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex,
 
 static struct xsk_socket_info *
 xsk_configure(int ifindex, int xdp_queue_id, enum afxdp_mode mode,
-              bool use_need_wakeup, bool report_socket_failures)
+              bool use_need_wakeup, bool report_socket_failures, bool inhibit)
 {
     struct xsk_socket_info *xsk;
     struct xsk_umem_info *umem;
@@ -435,7 +440,8 @@  xsk_configure(int ifindex, int xdp_queue_id, enum afxdp_mode mode,
     VLOG_DBG("Allocated umem pool at 0x%"PRIxPTR, (uintptr_t) umem);
 
     xsk = xsk_configure_socket(umem, ifindex, xdp_queue_id, mode,
-                               use_need_wakeup, report_socket_failures);
+                               use_need_wakeup, report_socket_failures,
+                               inhibit);
     if (!xsk) {
         /* Clean up umem and xpacket pool. */
         if (xsk_umem__delete(umem->umem)) {
@@ -459,7 +465,7 @@  xsk_configure_queue(struct netdev_linux *dev, int ifindex, int queue_id,
              netdev_get_name(&dev->up), queue_id, xdp_modes[mode].name,
              dev->use_need_wakeup ? "true" : "false");
     xsk_info = xsk_configure(ifindex, queue_id, mode, dev->use_need_wakeup,
-                             report_socket_failures);
+                             report_socket_failures, dev->inhibit);
     if (!xsk_info) {
         VLOG(report_socket_failures ? VLL_ERR : VLL_DBG,
              "%s: Failed to create AF_XDP socket on queue %d in %s mode.",
@@ -582,9 +588,11 @@  xsk_destroy_all(struct netdev *netdev)
         dev->xsks = NULL;
     }
 
-    VLOG_INFO("%s: Removing xdp program.", netdev_get_name(netdev));
-    ifindex = linux_get_ifindex(netdev_get_name(netdev));
-    xsk_remove_xdp_program(ifindex, dev->xdp_mode_in_use);
+    if (!dev->inhibit) {
+        VLOG_INFO("%s: Removing xdp program.", netdev_get_name(netdev));
+        ifindex = linux_get_ifindex(netdev_get_name(netdev));
+        xsk_remove_xdp_program(ifindex, dev->xdp_mode_in_use);
+    }
 
     if (dev->tx_locks) {
         for (i = 0; i < netdev_n_txq(netdev); i++) {
@@ -602,7 +610,7 @@  netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
     struct netdev_linux *dev = netdev_linux_cast(netdev);
     const char *str_xdp_mode;
     enum afxdp_mode xdp_mode;
-    bool need_wakeup;
+    bool need_wakeup, need_inhibit;
     int new_n_rxq;
 
     ovs_mutex_lock(&dev->mutex);
@@ -636,13 +644,16 @@  netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
         need_wakeup = false;
     }
 #endif
+    need_inhibit = smap_get_bool(args, "inhibit", false);
 
     if (dev->requested_n_rxq != new_n_rxq
         || dev->requested_xdp_mode != xdp_mode
-        || dev->requested_need_wakeup != need_wakeup) {
+        || dev->requested_need_wakeup != need_wakeup
+        || dev->requested_inhibit != need_inhibit) {
         dev->requested_n_rxq = new_n_rxq;
         dev->requested_xdp_mode = xdp_mode;
         dev->requested_need_wakeup = need_wakeup;
+        dev->requested_inhibit = need_inhibit;
         netdev_request_reconfigure(netdev);
     }
     ovs_mutex_unlock(&dev->mutex);
@@ -661,6 +672,8 @@  netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args)
                     xdp_modes[dev->xdp_mode_in_use].name);
     smap_add_format(args, "use-need-wakeup", "%s",
                     dev->use_need_wakeup ? "true" : "false");
+    smap_add_format(args, "inhibit", "%s",
+                    dev->inhibit ? "true" : "false");
     ovs_mutex_unlock(&dev->mutex);
     return 0;
 }
@@ -696,6 +709,7 @@  netdev_afxdp_reconfigure(struct netdev *netdev)
     if (netdev->n_rxq == dev->requested_n_rxq
         && dev->xdp_mode == dev->requested_xdp_mode
         && dev->use_need_wakeup == dev->requested_need_wakeup
+        && dev->inhibit == dev->requested_inhibit
         && dev->xsks) {
         goto out;
     }
@@ -713,6 +727,7 @@  netdev_afxdp_reconfigure(struct netdev *netdev)
         VLOG_ERR("setrlimit(RLIMIT_MEMLOCK) failed: %s", ovs_strerror(errno));
     }
     dev->use_need_wakeup = dev->requested_need_wakeup;
+    dev->inhibit = dev->requested_inhibit;
 
     err = xsk_configure_all(netdev);
     if (err) {
@@ -762,10 +777,12 @@  signal_remove_xdp(struct netdev *netdev)
     struct netdev_linux *dev = netdev_linux_cast(netdev);
     int ifindex;
 
-    ifindex = linux_get_ifindex(netdev_get_name(netdev));
+    if (!dev->inhibit) {
+        ifindex = linux_get_ifindex(netdev_get_name(netdev));
 
-    VLOG_WARN("Force removing xdp program.");
-    xsk_remove_xdp_program(ifindex, dev->xdp_mode_in_use);
+        VLOG_WARN("Force removing xdp program.");
+        xsk_remove_xdp_program(ifindex, dev->xdp_mode_in_use);
+    }
 }
 
 static struct dp_packet_afxdp *
@@ -1183,6 +1200,8 @@  netdev_afxdp_construct(struct netdev *netdev)
     dev->requested_n_rxq = NR_QUEUE;
     dev->requested_xdp_mode = OVS_AF_XDP_MODE_BEST_EFFORT;
     dev->requested_need_wakeup = NEED_WAKEUP_DEFAULT;
+    dev->requested_inhibit = false;
+    dev->inhibit = false;
 
     dev->xsks = NULL;
     dev->tx_locks = NULL;
diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
index deb015bdb..6939b465c 100644
--- a/lib/netdev-linux-private.h
+++ b/lib/netdev-linux-private.h
@@ -119,6 +119,9 @@  struct netdev_linux {
     bool use_need_wakeup;
     bool requested_need_wakeup;
 
+    bool inhibit;
+    bool requested_inhibit;
+
     struct netdev_afxdp_tx_lock *tx_locks;  /* Array of locks for TX queues. */
 #endif
 };