[ovs-dev] [PATCHv4] netdev-afxdp: Add need_wakeup supprt.
William Tu
u9012063 at gmail.com
Wed Sep 25 23:01:39 UTC 2019
On Tue, Sep 24, 2019 at 7:48 AM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> Hi.
> Thanks for a new version.
>
> Comments inline.
Hi Ilya,
Thanks for your feedback.
<snip>
> > static struct xsk_socket_info *
> > -xsk_configure(int ifindex, int xdp_queue_id, int xdpmode)
> > +xsk_configure(int ifindex, int xdp_queue_id, int xdpmode,
> > + bool use_need_wakeup)
> > {
> > struct xsk_socket_info *xsk;
> > struct xsk_umem_info *umem;
> > @@ -334,7 +392,8 @@ xsk_configure(int ifindex, int xdp_queue_id, int xdpmode)
> >
> > VLOG_DBG("Allocated umem pool at 0x%"PRIxPTR, (uintptr_t) umem);
> >
> > - xsk = xsk_configure_socket(umem, ifindex, xdp_queue_id, xdpmode);
> > + xsk = xsk_configure_socket(umem, ifindex, xdp_queue_id, xdpmode,
> > + use_need_wakeup);
> > if (!xsk) {
> > /* Clean up umem and xpacket pool. */
> > if (xsk_umem__delete(umem->umem)) {
> > @@ -365,9 +424,12 @@ xsk_configure_all(struct netdev *netdev)
> >
> > /* Configure each queue. */
> > for (i = 0; i < n_rxq; i++) {
> > - VLOG_INFO("%s: configure queue %d mode %s", __func__, i,
> > - dev->xdpmode == XDP_COPY ? "SKB" : "DRV");
> > - xsk_info = xsk_configure(ifindex, i, dev->xdpmode);
> > + VLOG_INFO("%s: configure queue %d mode %s use_need_wakeup %s.",
> > + __func__, i,
>
> __func__ seems unnecessary here. Netdev name would make more sense.
>
> BTW, maybe DBG will be more suitable for this kind of log?
Sure, I will fix it in next version.
>
> > + dev->xdpmode == XDP_COPY ? "SKB" : "DRV",
> > + dev->use_need_wakeup ? "true" : "false");
> > + xsk_info = xsk_configure(ifindex, i, dev->xdpmode,
> > + dev->use_need_wakeup);
> > if (!xsk_info) {
> > VLOG_ERR("Failed to create AF_XDP socket on queue %d.", i);
> > dev->xsks[i] = NULL;
> > @@ -459,6 +521,7 @@ netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
> > struct netdev_linux *dev = netdev_linux_cast(netdev);
> > const char *str_xdpmode;
> > int xdpmode, new_n_rxq;
> > + bool need_wakeup;
> >
> > ovs_mutex_lock(&dev->mutex);
> > new_n_rxq = MAX(smap_get_int(args, "n_rxq", NR_QUEUE), 1);
> > @@ -481,10 +544,17 @@ netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
> > return EINVAL;
> > }
> >
> > + need_wakeup = smap_get_bool(args, "use_need_wakeup", true);
> > + if (need_wakeup && !HAVE_XDP_NEED_WAKEUP) {
>
> I'm not sure if this will compile if HAVE_XDP_NEED_WAKEUP will
> not be defined. Anyway it seems dangerous to mix up this kind
> of macro definitions with code and even using an arithmetic on them.
>
At acinclude, it's either define as 0 or 1
AC_DEFINE([HAVE_XDP_NEED_WAKEUP], [0],...
Let me think about other way.
> > + VLOG_WARN("XDP need_wakeup is not supported in libbpf.");
> > + }
> > +
> > if (dev->requested_n_rxq != new_n_rxq
> > - || dev->requested_xdpmode != xdpmode) {
> > + || dev->requested_xdpmode != xdpmode
> > + || dev->requested_need_wakeup != need_wakeup) {
> > dev->requested_n_rxq = new_n_rxq;
> > dev->requested_xdpmode = xdpmode;
> > + dev->requested_need_wakeup = need_wakeup;
> > netdev_request_reconfigure(netdev);
> > }
> > ovs_mutex_unlock(&dev->mutex);
> > @@ -500,6 +570,8 @@ 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",
> > + dev->use_need_wakeup ? "true" : "false");
>
> Could you, please, align this line to the '('.
> I see that above line is not aligned too, but you may align it too.
> It's better to not reproduce bad style patterns.
Sorry for making the mistake again and again...
Will fix it in next version.
>
> > ovs_mutex_unlock(&dev->mutex);
> > return 0;
> > }
> > @@ -515,6 +587,7 @@ netdev_afxdp_reconfigure(struct netdev *netdev)
> >
> > if (netdev->n_rxq == dev->requested_n_rxq
> > && dev->xdpmode == dev->requested_xdpmode
> > + && dev->use_need_wakeup == dev->requested_need_wakeup
> > && dev->xsks) {
> > goto out;
> > }
> > @@ -538,6 +611,7 @@ netdev_afxdp_reconfigure(struct netdev *netdev)
> > * when no device is in DRV mode.
> > */
> > }
> > + dev->use_need_wakeup = dev->requested_need_wakeup;
> >
> > err = xsk_configure_all(netdev);
> > if (err) {
> > @@ -660,6 +734,7 @@ netdev_afxdp_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> >
> > rcvd = xsk_ring_cons__peek(&xsk_info->rx, BATCH_SIZE, &idx_rx);
> > if (!rcvd) {
> > + xsk_rx_wakeup_if_needed(umem, netdev, rx->fd);
> > return EAGAIN;
> > }
> >
> > @@ -704,11 +779,15 @@ netdev_afxdp_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
> > }
> >
> > static inline int
> > -kick_tx(struct xsk_socket_info *xsk_info, int xdpmode)
> > +kick_tx(struct xsk_socket_info *xsk_info, int xdpmode, bool use_need_wakeup)
> > {
> > int ret, retries;
> > static const int KERNEL_TX_BATCH_SIZE = 16;
> >
> > + if (use_need_wakeup && !xsk_tx_need_wakeup(xsk_info)) {
> > + return 0;
> > + }
> > +
> > /* In SKB_MODE packet transmission is synchronous, and the kernel xmits
> > * only TX_BATCH_SIZE(16) packets for a single sendmsg syscall.
> > * So, we have to kick the kernel (n_packets / 16) times to be sure that
> > @@ -876,7 +955,7 @@ __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
> > atomic_add_relaxed(&xsk_info->tx_dropped, batch->count, &orig);
> > COVERAGE_INC(afxdp_tx_full);
> > afxdp_complete_tx(xsk_info);
> > - kick_tx(xsk_info, dev->xdpmode);
> > + kick_tx(xsk_info, dev->xdpmode, dev->use_need_wakeup);
> > error = ENOMEM;
> > goto out;
> > }
> > @@ -900,7 +979,7 @@ __netdev_afxdp_batch_send(struct netdev *netdev, int qid,
> > xsk_ring_prod__submit(&xsk_info->tx, batch->count);
> > xsk_info->outstanding_tx += batch->count;
> >
> > - ret = kick_tx(xsk_info, dev->xdpmode);
> > + ret = kick_tx(xsk_info, dev->xdpmode, dev->use_need_wakeup);
> > if (OVS_UNLIKELY(ret)) {
> > VLOG_WARN_RL(&rl, "%s: error sending AF_XDP packet: %s.",
> > netdev_get_name(netdev), ovs_strerror(ret));
> > @@ -970,6 +1049,7 @@ netdev_afxdp_construct(struct netdev *netdev)
> >
> > dev->requested_n_rxq = NR_QUEUE;
> > dev->requested_xdpmode = XDP_COPY;
> > + dev->requested_need_wakeup = true;
> >
> > dev->xsks = NULL;
> > dev->tx_locks = NULL;
> > diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> > index a350be151147..c14f2fb81bb0 100644
> > --- a/lib/netdev-linux-private.h
> > +++ b/lib/netdev-linux-private.h
> > @@ -102,6 +102,8 @@ struct netdev_linux {
> > int requested_n_rxq;
> > int xdpmode; /* AF_XDP running mode: driver or skb. */
> > int requested_xdpmode;
> > + bool use_need_wakeup;
> > + bool requested_need_wakeup;
> > struct ovs_spin *tx_locks; /* spin lock array for TX queues. */
> > #endif
> > };
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 9a743c05b4bf..e8b428d323c7 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -3147,6 +3147,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
> > </p>
> > </column>
> >
> > + <column name="options" key="use_need_wakeup"
> > + type='{"type": "boolean"}'>
> > + <p>
> > + Specifies whether to use need_wakeup feature in afxdp netdev.
> > + (required kernel version later than 5.3.0-rc1)
>
> AFAIK, there is no support for need-wakeup feature in released v5.3
> kernel. It's only available in current master.
> I guess, you've taken this version from the net-next tree which is
> rebased continuously. So, the version info is not correct.
>
> On the mainline kernel feature will be available only starting from
> v5.4, but there is no such tag yet.
OK, then maybe we can document it later when v5.4 is available.
Regards,
William
More information about the dev
mailing list