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

William Tu u9012063 at gmail.com
Mon Oct 28 23:32:43 UTC 2019


On Mon, Oct 28, 2019 at 12:11 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> 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):
>
Hi Ilya,

Yes, I'm ok with using dashes.
diff below looks good to me. Thanks
William

> 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