[ovs-dev] [PATCHv6] netdev-afxdp: Add need_wakeup supprt.

Ilya Maximets i.maximets at ovn.org
Mon Oct 28 19:11:55 UTC 2019


On 28.10.2019 11:46, Eelco Chaudron wrote:
> 
> 
> On 23 Oct 2019, at 23:06, William Tu wrote:
> 
>> The patch adds support for using need_wakeup flag in AF_XDP rings.
>> A new option, use_need_wakeup, is added.  When this option is used,
>> it means that OVS has to explicitly wake up the kernel RX, using poll()
>> syscall and wake up TX, using sendto() syscall. This feature improves
>> the performance by avoiding unnecessary sendto syscalls for TX.
>> For RX, instead of kernel always busy-spinning on fille queue, OVS wakes
>> up the kernel RX processing when fill queue is replenished.
>>
>> The need_wakeup feature is merged into Linux kernel bpf-next tee with commit
>> 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings") and
>> OVS enables it by default, if libbpf supports it.  If users enable it but
>> runs in an older version of libbpf, then the need_wakeup feature has no effect,
>> and a warning message is logged.
>>
>> For virtual interface, it's better set use_need_wakeup=false, since
>> the virtual device's AF_XDP xmit is synchronous: the sendto syscall
>> enters kernel and process the TX packet on tx queue directly.
>>
>> On Intel Xeon E5-2620 v3 2.4GHz system, performance of physical port
>> to physical port improves from 6.1Mpps to 7.3Mpps.
>>
>> Suggested-by: Ilya Maximets <i.maximets at ovn.org>
>> Signed-off-by: William Tu <u9012063 at gmail.com>
> 
> Reviewed based on diff from previous version, also did quick compile test with and without need_wakeup supported kernel.
> No actual performance tests ran, as my setup is in a messed up state :(
> 
> One small issue (guess can be fixed at apply time), the subject mentions “supprt”, it’s missing an o.
> 
> Acked-by: Eelco Chaudron <echaudro at redhat.com>
> 

Thanks, I could fix the 'supprt' while applying the patch.

But I have one more question.
William, Eelco, what do you think about renaming the option itself from
"options:use_need_wakeup" to "options:use-need-wakeup"?
Most of the netdev options for netdev-dpdk and netdev-linux except few of them
(e.g. n_rxq, n_rxq_desc) uses dashes in their names instead of underscores.
I agree that right now there is a mess in OVS and there is no specified convention
for options naming, but it might be good idea to name all the new options in a
similar way, to keep the API more or less consistent.  What do you think?

I could squash in below diff while applying the patch (I also added a NEWS entry):

diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst
index 202b6cdf7..fa898912b 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -182,7 +182,7 @@ more details):
  
   * **xdpmode**: use "drv" for driver mode, or "skb" for skb mode.
  
- * **use_need_wakeup**: default "true" if libbpf supports it, otherwise false.
+ * **use-need-wakeup**: default "true" if libbpf supports it, otherwise false.
  
  For example, to use 1 PMD (on core 4) on 1 queue (queue 0) device,
  configure these options: **pmd-cpu-mask, pmd-rxq-affinity, and n_rxq**.
diff --git a/NEWS b/NEWS
index 330ab3832..4d8092d62 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,9 @@ Post-v2.12.0
         separate project. You can find it at
         https://github.com/ovn-org/ovn.git
     - Userspace datapath:
+     * New option 'use-need-wakeup' for netdev-afxdp to control enabling
+       of corresponding 'need_wakup' flag in AF_XDP rings.  Enabled by default
+       if supported by libbpf.
       * Add option to enable, disable and query TCP sequence checking in
         conntrack.
  
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index b6b03e39b..270a5ab0c 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -330,7 +330,7 @@ xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex,
                               &xsk->rx, &xsk->tx, &cfg);
      if (ret) {
          VLOG_ERR("xsk_socket__create failed (%s) mode: %s "
-                 "use_need_wakeup: %s qid: %d",
+                 "use-need-wakeup: %s qid: %d",
                   ovs_strerror(errno),
                   xdpmode == XDP_COPY ? "SKB": "DRV",
                   use_need_wakeup ? "true" : "false",
@@ -431,7 +431,7 @@ xsk_configure_all(struct netdev *netdev)
  
      /* Configure each queue. */
      for (i = 0; i < n_rxq; i++) {
-        VLOG_DBG("%s: configure queue %d mode %s use_need_wakeup %s.",
+        VLOG_DBG("%s: configure queue %d mode %s use-need-wakeup %s.",
                   netdev_get_name(netdev), i,
                   dev->xdpmode == XDP_COPY ? "SKB" : "DRV",
                   dev->use_need_wakeup ? "true" : "false");
@@ -551,7 +551,7 @@ netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
          return EINVAL;
      }
  
-    need_wakeup = smap_get_bool(args, "use_need_wakeup", NEED_WAKEUP_DEFAULT);
+    need_wakeup = smap_get_bool(args, "use-need-wakeup", NEED_WAKEUP_DEFAULT);
  #ifndef HAVE_XDP_NEED_WAKEUP
      if (need_wakeup) {
          VLOG_WARN("XDP need_wakeup is not supported in libbpf.");
@@ -580,7 +580,7 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args)
      smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
      smap_add_format(args, "xdpmode", "%s",
                      dev->xdpmode == XDP_ZEROCOPY ? "drv" : "skb");
-    smap_add_format(args, "use_need_wakeup", "%s",
+    smap_add_format(args, "use-need-wakeup", "%s",
                      dev->use_need_wakeup ? "true" : "false");
      ovs_mutex_unlock(&dev->mutex);
      return 0;
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index decccfe84..00c6bd2d4 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3122,7 +3122,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
          </p>
        </column>
  
-      <column name="options" key="use_need_wakeup"
+      <column name="options" key="use-need-wakeup"
                type='{"type": "boolean"}'>
          <p>
            Specifies whether to use need_wakeup feature in afxdp netdev.
---

Best regards, Ilya Maximets.


More information about the dev mailing list