[ovs-dev] [PATCH] netdev-dpdk : vhost-user port link state fix

[Daniele Di Proietto]

Thanks for the patch!

I have a few comments inline, otherwise this looks good to me

2016-05-12 9:13 GMT-07:00 Zoltán Balogh <zoltan.balogh at ericsson.com>:

> Hi,
>
> OVS reports that link state of a vhost-user port (type=dpdkvhostuser) is
> DOWN, even when traffic is running through the port between a Virtual
> Machine and the vSwitch.
> Changing admin state with the "ovs-ofctl mod-port <BR> <PORT> up/down"
> command over OpenFlow does affect neither the reported link state nor the
> traffic.
>
> The patch below does the flowing:
>  - Triggers link state change by altering netdev's change_seq member.
>  - Controls sending/receiving of packets through vhost-user port according
> to the port's current admin state.
>  - Sets admin state of newly created vhost-user port to UP.
>
> Signed-off-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
> Co-authored-by: Jan Scheurich <jan.scheurich at ericsson.com>
> Signed-off-by: Jan Scheurich <jan.scheurich at ericsson.com>
>
> ---
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index af86d19..155efe1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -772,6 +772,8 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int
> port_no,
>          }
>      } else {
>          netdev_dpdk_alloc_txq(dev, OVS_VHOST_MAX_QUEUE_NUM);
> +        /* Enable DPDK_DEV_VHOST device and set promiscuous mode flag. */
> +        dev->flags = NETDEV_UP | NETDEV_PROMISC;
>      }
>
>      ovs_list_push_back(&dpdk_list, &dev->list_node);
> @@ -1256,6 +1258,21 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
>          return EAGAIN;
>      }
>
> +    /* Delete received packets if device is disabled. */
> +    if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
> +        uint16_t i;
> +
> +        VLOG_WARN_RL(&rl, "error receiving Ethernet packet on %s: %s",
> +                     netdev_rxq_get_name(rxq), ovs_strerror(ENONET));
> +
> +        for (i = 0; i < nb_rx; i++) {
> +            dp_packet_delete(packets[i]);
> +        }
> +
> +        *c = 0;
> +        return EAGAIN;
> +    }
> +
>

Is there a particular reason we need to call rte_vhost_dequeue_burst() and
then
delete the packets?

A few lines above, we already have

if (OVS_UNLIKELY(!is_vhost_running(virtio_dev))) {

we might as well check for NETDEV_UP there.

Also, I don't think we should log a warning if the device is disabled.

     rte_spinlock_lock(&dev->stats_lock);
>      netdev_dpdk_vhost_update_rx_counters(&dev->stats, packets, nb_rx);
>      rte_spinlock_unlock(&dev->stats_lock);
> @@ -1516,6 +1533,23 @@ static int
>  netdev_dpdk_vhost_send(struct netdev *netdev, int qid, struct dp_packet
> **pkts,
>                   int cnt, bool may_steal)
>  {
> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +
> +    /* Do not send anything if device is disabled. */
> +    if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
> +        int i;
> +
> +        VLOG_WARN_RL(&rl, "error sending Ethernet packet on %s: %s",
> +                     netdev_get_name(netdev), ovs_strerror(ENONET));
> +
> +        if (may_steal) {
> +            for (i = 0; i < cnt; i++) {
> +                dp_packet_delete(pkts[i]);
> +            }
> +        }
> +        return ENONET;
> +    }
> +
>

In __netdev_dpdk_vhost_send() we have this check:

if (OVS_UNLIKELY(!is_vhost_running(virtio_dev) || qid < 0)) {

I think we should check for NETDEV_UP there and avoid printing the warning.


>      if (OVS_UNLIKELY(pkts[0]->source != DPBUF_DPDK)) {
>          int i;
>
> @@ -2004,6 +2038,23 @@ netdev_dpdk_update_flags__(struct netdev_dpdk *dev,
>          if (!(dev->flags & NETDEV_UP)) {
>              rte_eth_dev_stop(dev->port_id);
>          }
> +    } else {
> +        /* If DPDK_DEV_VHOST device's NETDEV_UP flag was changed and
> vhost is
> +         * running then change netdev's change_seq to trigger link state
> +         * update. */
> +        struct virtio_net *virtio_dev = netdev_dpdk_get_virtio(dev);
> +
> +        if ((NETDEV_UP & ((*old_flagsp ^ on) | (*old_flagsp ^ off)))
> +            && is_vhost_running(virtio_dev)) {
> +            netdev_change_seq_changed(&dev->up);
> +
> +            /* Clear statistics if device is getting up. */
> +            if (NETDEV_UP & on) {
> +                rte_spinlock_lock(&dev->stats_lock);
> +                memset(&dev->stats, 0x00, sizeof(dev->stats));
>

Could you use 0, instead of 0x00?


> +                rte_spinlock_unlock(&dev->stats_lock);
> +            }
> +        }
>      }
>
>      return 0;
> @@ -2226,6 +2277,7 @@ new_device(struct virtio_net *virtio_dev)
>              virtio_dev->flags |= VIRTIO_DEV_RUNNING;
>              /* Disable notifications. */
>              set_irq_status(virtio_dev);
> +            netdev_change_seq_changed(&dev->up);
>              ovs_mutex_unlock(&dev->mutex);
>              break;
>          }
> @@ -2277,6 +2329,7 @@ destroy_device(volatile struct virtio_net
> *virtio_dev)
>              ovsrcu_set(&dev->virtio_dev, NULL);
>              netdev_dpdk_txq_map_clear(dev);
>              exists = true;
> +            netdev_change_seq_changed(&dev->up);
>              ovs_mutex_unlock(&dev->mutex);
>              break;
>          }
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>