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.bal...@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.bal...@ericsson.com> > Co-authored-by: Jan Scheurich <jan.scheur...@ericsson.com> > Signed-off-by: Jan Scheurich <jan.scheur...@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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev