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 <[email protected]>
Signed-off-by: William Tu <[email protected]>
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 <[email protected]>
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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
