[ovs-dev] [PATCH] netdev-dpdk: Add vHost User PMD

Loftus, Ciara ciara.loftus at intel.com
Tue Dec 6 15:10:35 UTC 2016


> 
> Thanks for the patch.
> 
> I experience a crash with this patch applied by starting ovs and
> immediately adding a vhostuserclient port.  It's not reproducible 100%
> of the times.
> 
> Program received signal SIGSEGV, Segmentation fault.
> rte_eth_xstats_get (port_id=3 '\003', xstats=xstats at entry=0x2e6eaa0,
> n=n at entry=5160) at
> /home/daniele/dpdk-16.11/lib/librte_ether/rte_ethdev.c:1466
> 1466                val = *stats_ptr;
> (gdb) bt
> #0  rte_eth_xstats_get (port_id=3 '\003',
> xstats=xstats at entry=0x2e6eaa0, n=n at entry=5160) at
> /home/daniele/dpdk-16.11/lib/librte_ether/rte_ethdev.c:1466
> #1  0x000000000077b88c in netdev_dpdk_get_stats
> (netdev=0x7f4f3ffb58c0, stats=0x7fff1239a900) at
> ../lib/netdev-dpdk.c:1871
> #2  0x00000000006af9c3 in netdev_get_stats (netdev=<optimized out>,
> stats=stats at entry=0x7fff1239a900) at ../lib/netdev.c:1338
> #3  0x000000000060b25f in iface_refresh_stats
> (iface=iface at entry=0x2e6ea20) at ../vswitchd/bridge.c:2365
> #4  0x000000000060fa59 in iface_refresh_stats (iface=0x2e6ea20) at
> ../vswitchd/bridge.c:2359
> #5  iface_create (port_cfg=0x2e69af0, iface_cfg=0x2e69e40,
> br=0x2e31d20) at ../vswitchd/bridge.c:1829
> #6  bridge_add_ports__ (br=br at entry=0x2e31d20,
> wanted_ports=wanted_ports at entry=0x2e31e00,
> with_requested_port=with_requested_port at entry=true) at
> ../vswitchd/bridge.c:912
> #7  0x00000000006105c2 in bridge_add_ports (wanted_ports=0x2e31e00,
> br=0x2e31d20) at ../vswitchd/bridge.c:923
> #8  bridge_reconfigure (ovs_cfg=ovs_cfg at entry=0x2de83b0) at
> ../vswitchd/bridge.c:644
> #9  0x000000000061372b in bridge_run () at ../vswitchd/bridge.c:2959
> #10 0x000000000040c48d in main (argc=6, argv=0x7fff1239af48) at
> ../vswitchd/ovs-vswitchd.c:111
> (gdb) info locals
> eth_stats = {ipackets = 0, opackets = 0, ibytes = 0, obytes = 0,
> imissed = 0, ierrors = 0, oerrors = 0, rx_nombuf = 0, q_ipackets = {0
> <repeats 16 times>}, q_opackets = {0 <repeats 16 times>}, q_ibytes =
> {0 <repeats 16 times>}, q_obytes = {0 <repeats 16 times>},
>   q_errors = {0 <repeats 16 times>}}
> dev = 0xd8fbc0 <rte_eth_devices+49536>
> count = <optimized out>
> i = 2
> q = 934
> xcount = <optimized out>
> val = 5548549585684803438
> stats_ptr = 0x7fff1239c000
> 
> Maybe this is happening because q is 934, while eth_stats only
> includes room for 16 queues.
> 

I'm looking into this at the moment - possibly an issue in the vhost pmd code.

> 
> 
> I suspect netdev_dpdk_vhost_reconfigure() should do much more than
> what it does.  I think that if we change the memory pool we should
> call dpdk_eth_dev_init(), because we need to pass the mempool to the
> PMD via rte_eth_rx_queue_setup.

You're right. Unfortunately this means we'll probably need to call rte_eth_dev_stop in vhost_reconfigure which will cause a link status change and thus trigger the lsc callback where we try to acquire the dev->mutex a second time & encounter a deadlock. I'll look into a way around this too!

> 
> 
> 
> Couple more comments inline
> 
> 2016-11-29 3:44 GMT-08:00 Ciara Loftus <ciara.loftus at intel.com>:
> > The vHost PMD allows vHost User ports to be controlled by the
> > librte_ether API, like physical 'dpdk' ports and IVSHM 'dpdkr' ports.
> > This commit integrates this PMD into OVS and removes direct calls to the
> > librte_vhost DPDK library.
> >
> > This commit requires DPDK v16.11 functionality that isn't available in
> > previous releases, and thus breaks compatibility with such releases.
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>
> >
> > ---
> > Change log:
> > * Rebase
> > * Alloc netdev->n_txq instead of max (1024) during netdev_dpdk_init
> > * Don't unregister unregistered callbacks.
> > * Move callback register to avoid need to call unregister in error case
> > * Detach and free pool ID if failure during client_reconfigure.
> > * Unregister callbacks before detach
> > * change vhost_pmd_id to signed int and use -1 value to indicate an
> >   ID from the pool hasn't been alloced for it yet.
> > * free pool ID if rte_eth_dev_attach fails.
> > * check link status on destruct and print warning if the port is live.
> > * only free ID during destruct if it has been allocated.
> > * Use id-pool implementation for allocating vHost PMD IDs
> > * Added DPDK_DEV_VHOST_CLIENT netdev_dpdk "type"
> > * Reintroduced socket-id logic to correctly set client type sid
> > * Removed magic number & used var instead
> > * Remove race condition in netdev_dpdk_init by reordering code.
> > * Detach vHost PMD if netdev_dpdk_init fails
> > * Introduce "out" in client_reconfigure for when txq alloc fails
> > * Remove set_tx_multiq fn for vHost ports
> >
> >  NEWS              |   2 +
> >  lib/dpdk.c        |  11 +
> >  lib/dpdk.h        |   1 +
> >  lib/netdev-dpdk.c | 956 ++++++++++++++++++++-----------------------------
> -----
> >  4 files changed, 373 insertions(+), 597 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 22beeb2..b979d2a 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -39,6 +39,8 @@ Post-v2.6.0
> >       * New option 'n_rxq_desc' and 'n_txq_desc' fields for DPDK interfaces
> >         which set the number of rx and tx descriptors to use for the given port.
> >       * Support for DPDK v16.11.
> > +     * vHost PMD integration brings vhost-user ports under control of the
> > +       rte_ether DPDK API.
> >     - Fedora packaging:
> >       * A package upgrade does not automatically restart OVS service.
> >     - ovs-vswitchd/ovs-vsctl:
> > diff --git a/lib/dpdk.c b/lib/dpdk.c
> > index 49a589a..dffc6f9 100644
> > --- a/lib/dpdk.c
> > +++ b/lib/dpdk.c
> > @@ -29,6 +29,7 @@
> >
> >  #include "dirs.h"
> >  #include "fatal-signal.h"
> > +#include "id-pool.h"
> >  #include "netdev-dpdk.h"
> >  #include "openvswitch/dynamic-string.h"
> >  #include "openvswitch/vlog.h"
> > @@ -37,6 +38,7 @@
> >  VLOG_DEFINE_THIS_MODULE(dpdk);
> >
> >  static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
> > +static struct id_pool *vhost_driver_ids;  /* Pool of IDs for vHost PMDs */
> >
> >  static int
> >  process_vhost_flags(char *flag, char *default_val, int size,
> > @@ -407,6 +409,9 @@ dpdk_init__(const struct smap *ovs_other_config)
> >      }
> >  #endif
> >
> > +    /* Create and initialize the pool of IDs for vHost PMD devices */
> > +    vhost_driver_ids = id_pool_create(0, RTE_MAX_ETHPORTS - 1);
> > +
> >      /* Finally, register the dpdk classes */
> >      netdev_dpdk_register();
> >  }
> > @@ -428,6 +433,12 @@ dpdk_get_vhost_sock_dir(void)
> >      return vhost_sock_dir;
> >  }
> >
> > +struct id_pool *
> > +dpdk_get_vhost_id_pool(void)
> > +{
> > +    return vhost_driver_ids;
> > +}
> > +
> 
> I'd prefer if we moved this to netdev-dpdk.  We could register it in
> netdev_dpdk_vhost_class_init().

Ok - will rearrange this in v2.

> 
> >  void
> >  dpdk_set_lcore_id(unsigned cpu)
> >  {
> > diff --git a/lib/dpdk.h b/lib/dpdk.h
> > index 673a1f1..fb85c06 100644
> > --- a/lib/dpdk.h
> > +++ b/lib/dpdk.h
> > @@ -35,5 +35,6 @@ struct smap;
> >  void dpdk_init(const struct smap *ovs_other_config);
> >  void dpdk_set_lcore_id(unsigned cpu);
> >  const char *dpdk_get_vhost_sock_dir(void);
> > +struct id_pool *dpdk_get_vhost_id_pool(void);
> >
> >  #endif /* dpdk.h */
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index b51f329..ede257f 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -27,6 +27,7 @@
> >  #include <rte_cycles.h>
> >  #include <rte_errno.h>
> >  #include <rte_eth_ring.h>
> > +#include <rte_eth_vhost.h>
> >  #include <rte_ethdev.h>
> >  #include <rte_malloc.h>
> >  #include <rte_mbuf.h>
> > @@ -38,6 +39,7 @@
> >  #include "dpdk.h"
> >  #include "dpif-netdev.h"
> >  #include "fatal-signal.h"
> > +#include "id-pool.h"
> >  #include "netdev-provider.h"
> >  #include "netdev-vport.h"
> >  #include "odp-util.h"
> > @@ -122,6 +124,7 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
> >  #define XSTAT_RX_BROADCAST_PACKETS       "rx_broadcast_packets"
> >  #define XSTAT_TX_BROADCAST_PACKETS       "tx_broadcast_packets"
> >  #define XSTAT_RX_UNDERSIZED_ERRORS       "rx_undersized_errors"
> > +#define XSTAT_RX_UNDERSIZE_PACKETS       "rx_undersize_packets"
> >  #define XSTAT_RX_OVERSIZE_ERRORS         "rx_oversize_errors"
> >  #define XSTAT_RX_FRAGMENTED_ERRORS       "rx_fragmented_errors"
> >  #define XSTAT_RX_JABBER_ERRORS           "rx_jabber_errors"
> > @@ -171,6 +174,7 @@ enum { DRAIN_TSC = 200000ULL };
> >  enum dpdk_dev_type {
> >      DPDK_DEV_ETH = 0,
> >      DPDK_DEV_VHOST = 1,
> > +    DPDK_DEV_VHOST_CLIENT = 2,
> >  };
> >
> >  /* Quality of Service */
> > @@ -343,15 +347,12 @@ struct netdev_dpdk {
> >      struct rte_eth_link link;
> >      int link_reset_cnt;
> >
> > -    /* virtio identifier for vhost devices */
> > -    ovsrcu_index vid;
> > -
> > -    /* True if vHost device is 'up' and has been reconfigured at least once */
> > -    bool vhost_reconfigured;
> > -
> >      /* Identifier used to distinguish vhost devices from each other. */
> >      char vhost_id[PATH_MAX];
> >
> > +    /* ID of vhost user port given to the PMD driver */
> > +    int32_t vhost_pmd_id;
> > +
> >      /* In dpdk_list. */
> >      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
> >
> > @@ -394,11 +395,16 @@ struct netdev_rxq_dpdk {
> >  static int netdev_dpdk_class_init(void);
> >  static int netdev_dpdk_vhost_class_init(void);
> >
> > -int netdev_dpdk_get_vid(const struct netdev_dpdk *dev);
> > -
> >  struct ingress_policer *
> >  netdev_dpdk_get_ingress_policer(const struct netdev_dpdk *dev);
> >
> > +static void link_status_changed_callback(uint8_t port_id,
> > +        enum rte_eth_event_type type, void *param);
> > +static void vring_state_changed_callback(uint8_t port_id,
> > +        enum rte_eth_event_type type, void *param);
> > +static void netdev_dpdk_remap_txqs(struct netdev_dpdk *dev);
> > +static void netdev_dpdk_txq_map_clear(struct netdev_dpdk *dev);
> > +
> >  static bool
> >  is_dpdk_class(const struct netdev_class *class)
> >  {
> > @@ -683,8 +689,13 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk
> *dev, int n_rxq, int n_txq)
> >              continue;
> >          }
> >
> > -        dev->up.n_rxq = n_rxq;
> > -        dev->up.n_txq = n_txq;
> > +        /* Only set n_*xq for physical devices. vHost User devices will set
> > +         * this value correctly using info from the virtio backend.
> > +         */
> > +        if (dev->type == DPDK_DEV_ETH) {
> > +            dev->up.n_rxq = n_rxq;
> > +            dev->up.n_txq = n_txq;
> > +        }
> >
> >          return 0;
> >      }
> > @@ -716,8 +727,16 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> >
> >      rte_eth_dev_info_get(dev->port_id, &info);
> >
> > -    n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
> > -    n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
> > +    if (dev->type != DPDK_DEV_ETH) {
> > +        /* We don't know how many queues QEMU will use so we need to
> set up
> > +         * the max. If we configure less than what QEMU configures, those
> > +         * additional queues will not be available to us. */
> > +        n_rxq = MIN(OVS_VHOST_MAX_QUEUE_NUM,
> RTE_MAX_QUEUES_PER_PORT);
> > +        n_txq = MIN(OVS_VHOST_MAX_QUEUE_NUM,
> RTE_MAX_QUEUES_PER_PORT);
> > +    } else {
> > +        n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
> > +        n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
> > +    }
> >
> >      diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
> >      if (diag) {
> > @@ -796,6 +815,93 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
> >      return txqs;
> >  }
> >
> > +void
> > +link_status_changed_callback(uint8_t port_id,
> > +                             enum rte_eth_event_type type OVS_UNUSED,
> > +                             void *param OVS_UNUSED)
> > +{
> > +    struct netdev_dpdk *dev;
> > +    int newnode = -1;
> > +
> > +    ovs_mutex_lock(&dpdk_mutex);
> > +    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > +        if (port_id == dev->port_id) {
> > +            ovs_mutex_lock(&dev->mutex);
> > +            check_link_status(dev);
> > +            if (dev->link.link_status == ETH_LINK_UP) {
> > +                /* Device brought up */
> > +                /* Get queue information */
> > +                int vid = rte_eth_vhost_get_vid_from_port_id(dev->port_id);
> > +                uint32_t qp_num = rte_vhost_get_queue_num(vid);
> > +                if (qp_num <= 0) {
> > +                    qp_num = dev->requested_n_rxq;
> > +                }
> > +                /* Get NUMA information */
> > +                newnode = rte_eth_dev_socket_id(dev->port_id);
> > +                if (newnode == -1) {
> > +                    newnode = dev->socket_id;
> > +                }
> > +                if (dev->requested_n_txq != qp_num
> > +                                || dev->requested_n_rxq != qp_num
> > +                                || dev->requested_socket_id != newnode) {
> > +                    dev->requested_socket_id = newnode;
> > +                    dev->requested_n_rxq = qp_num;
> > +                    dev->requested_n_txq = qp_num;
> > +                    netdev_request_reconfigure(&dev->up);
> > +                }
> > +                netdev_change_seq_changed(&dev->up);
> > +                VLOG_INFO("vHost Device '%s' has been added on numa node
> %i",
> > +                          dev->vhost_id, newnode);
> > +            } else {
> > +                /* Device brought down */
> > +                /* Clear tx/rx queue settings. */
> > +                netdev_dpdk_txq_map_clear(dev);
> > +                netdev_change_seq_changed(&dev->up);
> > +                VLOG_INFO("vHost Device '%s' has been removed", dev-
> >vhost_id);
> > +            }
> > +            ovs_mutex_unlock(&dev->mutex);
> > +            break;
> > +        }
> > +    }
> > +
> > +    ovs_mutex_unlock(&dpdk_mutex);
> > +
> > +    return;
> > +}
> > +
> > +void
> > +vring_state_changed_callback(uint8_t port_id,
> > +                             enum rte_eth_event_type type OVS_UNUSED,
> > +                             void *param OVS_UNUSED)
> > +{
> > +    struct netdev_dpdk *dev;
> > +    struct rte_eth_vhost_queue_event event;
> > +    int err = 0;
> > +
> > +    err = rte_eth_vhost_get_queue_event(port_id, &event);
> > +    if (err || event.rx) {
> > +        return;
> > +    }
> > +
> > +    ovs_mutex_lock(&dpdk_mutex);
> > +    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > +        if (port_id == dev->port_id) {
> > +            ovs_mutex_lock(&dev->mutex);
> > +            if (event.enable) {
> > +                dev->tx_q[event.queue_id].map = event.queue_id;
> > +            } else {
> > +                dev->tx_q[event.queue_id].map =
> OVS_VHOST_QUEUE_DISABLED;
> > +            }
> > +            netdev_dpdk_remap_txqs(dev);
> > +            ovs_mutex_unlock(&dev->mutex);
> > +            break;
> > +        }
> > +    }
> > +    ovs_mutex_unlock(&dpdk_mutex);
> > +
> > +    return;
> > +}
> > +
> >  static int
> >  netdev_dpdk_init(struct netdev *netdev, unsigned int port_no,
> >                   enum dpdk_dev_type type)
> > @@ -813,7 +919,7 @@ netdev_dpdk_init(struct netdev *netdev, unsigned
> int port_no,
> >      /* If the 'sid' is negative, it means that the kernel fails
> >       * to obtain the pci numa info.  In that situation, always
> >       * use 'SOCKET0'. */
> > -    if (type == DPDK_DEV_ETH) {
> > +    if (type != DPDK_DEV_VHOST_CLIENT) {
> >          sid = rte_eth_dev_socket_id(port_no);
> >      } else {
> >          sid = rte_lcore_to_socket_id(rte_get_master_lcore());
> > @@ -826,8 +932,8 @@ netdev_dpdk_init(struct netdev *netdev, unsigned
> int port_no,
> >      dev->flags = 0;
> >      dev->requested_mtu = dev->mtu = ETHER_MTU;
> >      dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
> > -    ovsrcu_index_init(&dev->vid, -1);
> > -    dev->vhost_reconfigured = false;
> > +    dev->vhost_driver_flags &= ~RTE_VHOST_USER_CLIENT;
> > +    dev->vhost_pmd_id = -1;
> >
> >      err = netdev_dpdk_mempool_configure(dev);
> >      if (err) {
> > @@ -851,23 +957,26 @@ netdev_dpdk_init(struct netdev *netdev,
> unsigned int port_no,
> >
> >      /* Initialize the flow control to NULL */
> >      memset(&dev->fc_conf, 0, sizeof dev->fc_conf);
> > -    if (type == DPDK_DEV_ETH) {
> > +    if (type != DPDK_DEV_VHOST_CLIENT) {
> >          err = dpdk_eth_dev_init(dev);
> >          if (err) {
> >              goto unlock;
> >          }
> > -        dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> > -    } else {
> > -        dev->tx_q =
> netdev_dpdk_alloc_txq(OVS_VHOST_MAX_QUEUE_NUM);
> > -        /* Enable DPDK_DEV_VHOST device and set promiscuous mode flag.
> */
> > -        dev->flags = NETDEV_UP | NETDEV_PROMISC;
> >      }
> > +    dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> >
> >      if (!dev->tx_q) {
> >          err = ENOMEM;
> >          goto unlock;
> >      }
> >
> > +    if (type == DPDK_DEV_VHOST) {
> > +        rte_eth_dev_callback_register(port_no,
> RTE_ETH_EVENT_QUEUE_STATE,
> > +                                      vring_state_changed_callback, NULL);
> > +        rte_eth_dev_callback_register(port_no, RTE_ETH_EVENT_INTR_LSC,
> > +                                      link_status_changed_callback, NULL);
> > +    }
> > +
> >      ovs_list_push_back(&dpdk_list, &dev->list_node);
> >
> >  unlock:
> > @@ -897,6 +1006,37 @@ dpdk_dev_parse_name(const char dev_name[],
> const char prefix[],
> >  }
> >
> >  static int
> > +dpdk_attach_vhost_pmd(struct netdev_dpdk *dev, int mode)
> > +{
> > +    char *devargs;
> > +    int err = 0;
> > +    uint8_t port_no = 0;
> > +    uint32_t driver_id = 0;
> > +
> > +    if (id_pool_alloc_id(dpdk_get_vhost_id_pool(), &driver_id)) {
> > +        devargs = xasprintf("net_vhost%u,iface=%s,queues=%i,client=%i",
> > +                 driver_id, dev->vhost_id,
> > +                 MIN(OVS_VHOST_MAX_QUEUE_NUM,
> RTE_MAX_QUEUES_PER_PORT),
> > +                 mode);
> > +        err = rte_eth_dev_attach(devargs, &port_no);
> > +        if (!err) {
> > +            dev->port_id = port_no;
> > +            dev->vhost_pmd_id = driver_id;
> > +        } else {
> > +            id_pool_free_id(dpdk_get_vhost_id_pool(), driver_id);
> > +            VLOG_ERR("Failed to attach vhost-user device %s to DPDK",
> > +                     dev->vhost_id);
> > +        }
> > +    } else {
> > +        VLOG_ERR("Unable to create vhost-user device %s - too many vhost-
> user"
> > +                 "devices registered with PMD", dev->vhost_id);
> > +        err = ENODEV;
> > +    }
> > +
> > +    return err;
> > +}
> > +
> > +static int
> >  netdev_dpdk_vhost_construct(struct netdev *netdev)
> >  {
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > @@ -906,7 +1046,7 @@ netdev_dpdk_vhost_construct(struct netdev
> *netdev)
> >      /* 'name' is appended to 'vhost_sock_dir' and used to create a socket in
> >       * the file system. '/' or '\' would traverse directories, so they're not
> >       * acceptable in 'name'. */
> > -    if (strchr(name, '/') || strchr(name, '\\')) {
> > +    if (strchr(name, '/') || strchr(name, '\\') || strchr(name, ',')) {
> >          VLOG_ERR("\"%s\" is not a valid name for a vhost-user port. "
> >                   "A valid name must not include '/' or '\\'",
> >                   name);
> > @@ -919,18 +1059,23 @@ netdev_dpdk_vhost_construct(struct netdev
> *netdev)
> >       */
> >      snprintf(dev->vhost_id, sizeof dev->vhost_id, "%s/%s",
> >               dpdk_get_vhost_sock_dir(), name);
> > +    dev->port_id = -1;
> >
> > -    dev->vhost_driver_flags &= ~RTE_VHOST_USER_CLIENT;
> > -    err = rte_vhost_driver_register(dev->vhost_id, dev-
> >vhost_driver_flags);
> > -    if (err) {
> > -        VLOG_ERR("vhost-user socket device setup failure for socket %s\n",
> > -                 dev->vhost_id);
> > -    } else {
> > +    err = dpdk_attach_vhost_pmd(dev, 0);
> > +
> > +    if (!err) {
> >          fatal_signal_add_file_to_unlink(dev->vhost_id);
> >          VLOG_INFO("Socket %s created for vhost-user port %s\n",
> >                    dev->vhost_id, name);
> >      }
> > -    err = netdev_dpdk_init(netdev, -1, DPDK_DEV_VHOST);
> > +    err = netdev_dpdk_init(netdev, dev->port_id, DPDK_DEV_VHOST);
> > +
> > +    if (err) {
> > +        rte_eth_dev_detach(dev->port_id, dev->vhost_id);
> > +        if (dev->vhost_pmd_id >= 0) {
> > +            id_pool_free_id(dpdk_get_vhost_id_pool(), dev->vhost_pmd_id);
> > +        }
> > +    }
> >
> >      ovs_mutex_unlock(&dpdk_mutex);
> >      return err;
> > @@ -942,7 +1087,7 @@ netdev_dpdk_vhost_client_construct(struct
> netdev *netdev)
> >      int err;
> >
> >      ovs_mutex_lock(&dpdk_mutex);
> > -    err = netdev_dpdk_init(netdev, -1, DPDK_DEV_VHOST);
> > +    err = netdev_dpdk_init(netdev, -1, DPDK_DEV_VHOST_CLIENT);
> >      ovs_mutex_unlock(&dpdk_mutex);
> >      return err;
> >  }
> > @@ -966,13 +1111,10 @@ netdev_dpdk_construct(struct netdev *netdev)
> >  }
> >
> >  static void
> > -netdev_dpdk_destruct(struct netdev *netdev)
> > +dpdk_destruct_helper(struct netdev_dpdk *dev)
> > +    OVS_REQUIRES(dpdk_mutex)
> > +    OVS_REQUIRES(dev->mutex)
> >  {
> > -    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > -
> > -    ovs_mutex_lock(&dpdk_mutex);
> > -    ovs_mutex_lock(&dev->mutex);
> > -
> >      rte_eth_dev_stop(dev->port_id);
> >      free(ovsrcu_get_protected(struct ingress_policer *,
> >                                &dev->ingress_policer));
> > @@ -980,61 +1122,60 @@ netdev_dpdk_destruct(struct netdev *netdev)
> >      rte_free(dev->tx_q);
> >      ovs_list_remove(&dev->list_node);
> >      dpdk_mp_put(dev->dpdk_mp);
> > -
> > -    ovs_mutex_unlock(&dev->mutex);
> > -    ovs_mutex_unlock(&dpdk_mutex);
> >  }
> >
> > -/* rte_vhost_driver_unregister() can call back destroy_device(), which will
> > - * try to acquire 'dpdk_mutex' and possibly 'dev->mutex'.  To avoid a
> > - * deadlock, none of the mutexes must be held while calling this function.
> */
> 
> How do we avoid this deadlock now?

I think this already came up. See https://mail.openvswitch.org/pipermail/ovs-dev/2016-October/324577.html

> 
> > -static int
> > -dpdk_vhost_driver_unregister(struct netdev_dpdk *dev OVS_UNUSED,
> > -                             char *vhost_id)
> > -    OVS_EXCLUDED(dpdk_mutex)
> > -    OVS_EXCLUDED(dev->mutex)
> > +static void
> > +netdev_dpdk_destruct(struct netdev *netdev)
> >  {
> > -    return rte_vhost_driver_unregister(vhost_id);
> > +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > +
> > +    ovs_mutex_lock(&dpdk_mutex);
> > +    ovs_mutex_lock(&dev->mutex);
> > +
> > +    dpdk_destruct_helper(dev);
> > +
> > +    ovs_mutex_unlock(&dev->mutex);
> > +    ovs_mutex_unlock(&dpdk_mutex);
> >  }
> >
> >  static void
> >  netdev_dpdk_vhost_destruct(struct netdev *netdev)
> >  {
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > -    char *vhost_id;
> >
> >      ovs_mutex_lock(&dpdk_mutex);
> >      ovs_mutex_lock(&dev->mutex);
> >
> >      /* Guest becomes an orphan if still attached. */
> > -    if (netdev_dpdk_get_vid(dev) >= 0
> > -        && !(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
> > +    check_link_status(dev);
> > +    if (dev->link.link_status == ETH_LINK_UP) {
> >          VLOG_ERR("Removing port '%s' while vhost device still attached.",
> >                   netdev->name);
> >          VLOG_ERR("To restore connectivity after re-adding of port, VM on "
> >                   "socket '%s' must be restarted.", dev->vhost_id);
> >      }
> >
> > -    free(ovsrcu_get_protected(struct ingress_policer *,
> > -                              &dev->ingress_policer));
> > +    rte_eth_dev_callback_unregister(dev->port_id,
> > +                                    RTE_ETH_EVENT_QUEUE_STATE,
> > +                                    vring_state_changed_callback, NULL);
> > +    rte_eth_dev_callback_unregister(dev->port_id,
> > +                                    RTE_ETH_EVENT_INTR_LSC,
> > +                                    link_status_changed_callback, NULL);
> >
> > -    rte_free(dev->tx_q);
> > -    ovs_list_remove(&dev->list_node);
> > -    dpdk_mp_put(dev->dpdk_mp);
> > +    if (rte_eth_dev_detach(dev->port_id, dev->vhost_id)) {
> > +        VLOG_ERR("Error removing vhost device %s", dev->vhost_id);
> > +    } else if (dev->type == DPDK_DEV_VHOST) {
> > +        fatal_signal_remove_file_to_unlink(dev->vhost_id);
> > +    }
> >
> > -    vhost_id = xstrdup(dev->vhost_id);
> > +    if (dev->vhost_pmd_id >= 0) {
> > +        id_pool_free_id(dpdk_get_vhost_id_pool(), dev->vhost_pmd_id);
> > +    }
> > +
> > +    dpdk_destruct_helper(dev);
> >
> >      ovs_mutex_unlock(&dev->mutex);
> >      ovs_mutex_unlock(&dpdk_mutex);
> > -
> > -    if (dpdk_vhost_driver_unregister(dev, vhost_id)) {
> > -        VLOG_ERR("%s: Unable to unregister vhost driver for socket '%s'.\n",
> > -                 netdev->name, vhost_id);
> > -    } else if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
> > -        /* OVS server mode - remove this socket from list for deletion */
> > -        fatal_signal_remove_file_to_unlink(vhost_id);
> > -    }
> > -    free(vhost_id);
> >  }
> >
> >  static void
> > @@ -1327,119 +1468,8 @@ ingress_policer_run(struct ingress_policer
> *policer, struct rte_mbuf **pkts,
> >      return cnt;
> >  }
> >
> > -static bool
> > -is_vhost_running(struct netdev_dpdk *dev)
> > -{
> > -    return (netdev_dpdk_get_vid(dev) >= 0 && dev->vhost_reconfigured);
> > -}
> > -
> > -static inline void
> > -netdev_dpdk_vhost_update_rx_size_counters(struct netdev_stats
> *stats,
> > -                                          unsigned int packet_size)
> > -{
> > -    /* Hard-coded search for the size bucket. */
> > -    if (packet_size < 256) {
> > -        if (packet_size >= 128) {
> > -            stats->rx_128_to_255_packets++;
> > -        } else if (packet_size <= 64) {
> > -            stats->rx_1_to_64_packets++;
> > -        } else {
> > -            stats->rx_65_to_127_packets++;
> > -        }
> > -    } else {
> > -        if (packet_size >= 1523) {
> > -            stats->rx_1523_to_max_packets++;
> > -        } else if (packet_size >= 1024) {
> > -            stats->rx_1024_to_1522_packets++;
> > -        } else if (packet_size < 512) {
> > -            stats->rx_256_to_511_packets++;
> > -        } else {
> > -            stats->rx_512_to_1023_packets++;
> > -        }
> > -    }
> > -}
> > -
> > -static inline void
> > -netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
> > -                                     struct dp_packet **packets, int count,
> > -                                     int dropped)
> > -{
> > -    int i;
> > -    unsigned int packet_size;
> > -    struct dp_packet *packet;
> > -
> > -    stats->rx_packets += count;
> > -    stats->rx_dropped += dropped;
> > -    for (i = 0; i < count; i++) {
> > -        packet = packets[i];
> > -        packet_size = dp_packet_size(packet);
> > -
> > -        if (OVS_UNLIKELY(packet_size < ETH_HEADER_LEN)) {
> > -            /* This only protects the following multicast counting from
> > -             * too short packets, but it does not stop the packet from
> > -             * further processing. */
> > -            stats->rx_errors++;
> > -            stats->rx_length_errors++;
> > -            continue;
> > -        }
> > -
> > -        netdev_dpdk_vhost_update_rx_size_counters(stats, packet_size);
> > -
> > -        struct eth_header *eh = (struct eth_header *)
> dp_packet_data(packet);
> > -        if (OVS_UNLIKELY(eth_addr_is_multicast(eh->eth_dst))) {
> > -            stats->multicast++;
> > -        }
> > -
> > -        stats->rx_bytes += packet_size;
> > -    }
> > -}
> > -
> > -/*
> > - * The receive path for the vhost port is the TX path out from guest.
> > - */
> > -static int
> > -netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> > -                           struct dp_packet_batch *batch)
> > -{
> > -    struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> > -    int qid = rxq->queue_id;
> > -    struct ingress_policer *policer =
> netdev_dpdk_get_ingress_policer(dev);
> > -    uint16_t nb_rx = 0;
> > -    uint16_t dropped = 0;
> > -
> > -    if (OVS_UNLIKELY(!is_vhost_running(dev)
> > -                     || !(dev->flags & NETDEV_UP))) {
> > -        return EAGAIN;
> > -    }
> > -
> > -    nb_rx = rte_vhost_dequeue_burst(netdev_dpdk_get_vid(dev),
> > -                                    qid * VIRTIO_QNUM + VIRTIO_TXQ,
> > -                                    dev->dpdk_mp->mp,
> > -                                    (struct rte_mbuf **) batch->packets,
> > -                                    NETDEV_MAX_BURST);
> > -    if (!nb_rx) {
> > -        return EAGAIN;
> > -    }
> > -
> > -    if (policer) {
> > -        dropped = nb_rx;
> > -        nb_rx = ingress_policer_run(policer,
> > -                                    (struct rte_mbuf **) batch->packets,
> > -                                    nb_rx);
> > -        dropped -= nb_rx;
> > -    }
> > -
> > -    rte_spinlock_lock(&dev->stats_lock);
> > -    netdev_dpdk_vhost_update_rx_counters(&dev->stats, batch-
> >packets,
> > -                                         nb_rx, dropped);
> > -    rte_spinlock_unlock(&dev->stats_lock);
> > -
> > -    batch->count = (int) nb_rx;
> > -    return 0;
> > -}
> > -
> > -static int
> > -netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch
> *batch)
> > +static inline int
> > +dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)
> >  {
> >      struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq);
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> > @@ -1474,6 +1504,30 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,
> struct dp_packet_batch *batch)
> >      return 0;
> >  }
> >
> > +static int
> > +netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch
> *batch)
> > +{
> > +    return dpdk_rxq_recv(rxq, batch);
> > +}
> > +
> > +static int
> > +netdev_dpdk_vhost_client_rxq_recv(struct netdev_rxq *rxq,
> > +                                  struct dp_packet_batch *batch)
> > +{
> > +    struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq);
> > +
> > +    if (rte_eth_dev_is_valid_port(rx->port_id)) {
> > +        return dpdk_rxq_recv(rxq, batch);
> > +    } else {
> > +        struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> > +
> > +        rte_spinlock_lock(&dev->stats_lock);
> > +        dev->stats.rx_dropped += batch->count;
> > +        rte_spinlock_unlock(&dev->stats_lock);
> > +        return 0;
> > +    }
> > +}
> > +
> >  static inline int
> >  netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,
> >                      int cnt)
> > @@ -1515,80 +1569,6 @@ netdev_dpdk_filter_packet_len(struct
> netdev_dpdk *dev, struct rte_mbuf **pkts,
> >      return cnt;
> >  }
> >
> > -static inline void
> > -netdev_dpdk_vhost_update_tx_counters(struct netdev_stats *stats,
> > -                                     struct dp_packet **packets,
> > -                                     int attempted,
> > -                                     int dropped)
> > -{
> > -    int i;
> > -    int sent = attempted - dropped;
> > -
> > -    stats->tx_packets += sent;
> > -    stats->tx_dropped += dropped;
> > -
> > -    for (i = 0; i < sent; i++) {
> > -        stats->tx_bytes += dp_packet_size(packets[i]);
> > -    }
> > -}
> > -
> > -static void
> > -__netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
> > -                         struct dp_packet **pkts, int cnt)
> > -{
> > -    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > -    struct rte_mbuf **cur_pkts = (struct rte_mbuf **) pkts;
> > -    unsigned int total_pkts = cnt;
> > -    unsigned int dropped = 0;
> > -    int i, retries = 0;
> > -
> > -    qid = dev->tx_q[qid % netdev->n_txq].map;
> > -
> > -    if (OVS_UNLIKELY(!is_vhost_running(dev) || qid < 0
> > -                     || !(dev->flags & NETDEV_UP))) {
> > -        rte_spinlock_lock(&dev->stats_lock);
> > -        dev->stats.tx_dropped+= cnt;
> > -        rte_spinlock_unlock(&dev->stats_lock);
> > -        goto out;
> > -    }
> > -
> > -    rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > -
> > -    cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
> > -    /* Check has QoS has been configured for the netdev */
> > -    cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);
> > -    dropped = total_pkts - cnt;
> > -
> > -    do {
> > -        int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> > -        unsigned int tx_pkts;
> > -
> > -        tx_pkts = rte_vhost_enqueue_burst(netdev_dpdk_get_vid(dev),
> > -                                          vhost_qid, cur_pkts, cnt);
> > -        if (OVS_LIKELY(tx_pkts)) {
> > -            /* Packets have been sent.*/
> > -            cnt -= tx_pkts;
> > -            /* Prepare for possible retry.*/
> > -            cur_pkts = &cur_pkts[tx_pkts];
> > -        } else {
> > -            /* No packets sent - do not retry.*/
> > -            break;
> > -        }
> > -    } while (cnt && (retries++ <= VHOST_ENQ_RETRY_NUM));
> > -
> > -    rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > -
> > -    rte_spinlock_lock(&dev->stats_lock);
> > -    netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts,
> total_pkts,
> > -                                         cnt + dropped);
> > -    rte_spinlock_unlock(&dev->stats_lock);
> > -
> > -out:
> > -    for (i = 0; i < total_pkts - dropped; i++) {
> > -        dp_packet_delete(pkts[i]);
> > -    }
> > -}
> > -
> >  /* Tx function. Transmit packets indefinitely */
> >  static void
> >  dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch
> *batch)
> > @@ -1636,18 +1616,13 @@ dpdk_do_tx_copy(struct netdev *netdev, int
> qid, struct dp_packet_batch *batch)
> >          newcnt++;
> >      }
> >
> > -    if (dev->type == DPDK_DEV_VHOST) {
> > -        __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,
> > -                                 newcnt);
> > -    } else {
> > -        unsigned int qos_pkts = newcnt;
> > +    unsigned int qos_pkts = newcnt;
> >
> > -        /* Check if QoS has been configured for this netdev. */
> > -        newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);
> > +    /* Check if QoS has been configured for this netdev. */
> > +    newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);
> >
> > -        dropped += qos_pkts - newcnt;
> > -        dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);
> > -    }
> > +    dropped += qos_pkts - newcnt;
> > +    dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);
> >
> >      if (OVS_UNLIKELY(dropped)) {
> >          rte_spinlock_lock(&dev->stats_lock);
> > @@ -1656,32 +1631,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int
> qid, struct dp_packet_batch *batch)
> >      }
> >  }
> >
> > -static int
> > -netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
> > -                       struct dp_packet_batch *batch,
> > -                       bool may_steal, bool concurrent_txq OVS_UNUSED)
> > -{
> > -
> > -    if (OVS_UNLIKELY(!may_steal || batch->packets[0]->source !=
> DPBUF_DPDK)) {
> > -        dpdk_do_tx_copy(netdev, qid, batch);
> > -        dp_packet_delete_batch(batch, may_steal);
> > -    } else {
> > -        dp_packet_batch_apply_cutlen(batch);
> > -        __netdev_dpdk_vhost_send(netdev, qid, batch->packets, batch-
> >count);
> > -    }
> > -    return 0;
> > -}
> > -
> >  static inline void
> >  netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
> > -                   struct dp_packet_batch *batch, bool may_steal,
> > -                   bool concurrent_txq)
> > +                   struct dp_packet_batch *batch, bool may_steal)
> >  {
> > -    if (OVS_UNLIKELY(concurrent_txq)) {
> > -        qid = qid % dev->up.n_txq;
> > -        rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > -    }
> > -
> >      if (OVS_UNLIKELY(!may_steal ||
> >                       batch->packets[0]->source != DPBUF_DPDK)) {
> >          struct netdev *netdev = &dev->up;
> > @@ -1707,20 +1660,50 @@ netdev_dpdk_send__(struct netdev_dpdk
> *dev, int qid,
> >              rte_spinlock_unlock(&dev->stats_lock);
> >          }
> >      }
> > +}
> > +
> > +static int
> > +netdev_dpdk_eth_send(struct netdev *netdev, int qid,
> > +                     struct dp_packet_batch *batch, bool may_steal,
> > +                     bool concurrent_txq)
> > +{
> > +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > +
> > +    if (OVS_UNLIKELY(concurrent_txq)) {
> > +        qid = qid % dev->up.n_txq;
> > +        rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +    }
> > +
> > +    netdev_dpdk_send__(dev, qid, batch, may_steal);
> >
> >      if (OVS_UNLIKELY(concurrent_txq)) {
> >          rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> >      }
> > +
> > +    return 0;
> >  }
> >
> >  static int
> > -netdev_dpdk_eth_send(struct netdev *netdev, int qid,
> > -                     struct dp_packet_batch *batch, bool may_steal,
> > -                     bool concurrent_txq)
> > +netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
> > +                       struct dp_packet_batch *batch, bool may_steal,
> > +                       bool concurrent_txq OVS_UNUSED)
> >  {
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >
> > -    netdev_dpdk_send__(dev, qid, batch, may_steal, concurrent_txq);
> > +    qid = dev->tx_q[qid % netdev->n_txq].map;
> > +    if (qid == -1) {
> > +        rte_spinlock_lock(&dev->stats_lock);
> > +        dev->stats.tx_dropped+= batch->count;
> > +        rte_spinlock_unlock(&dev->stats_lock);
> > +        if (may_steal) {
> > +            dp_packet_delete_batch(batch, may_steal);
> > +        }
> > +    } else {
> > +        rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +        netdev_dpdk_send__(dev, qid, batch, may_steal);
> > +        rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > +    }
> > +
> >      return 0;
> >  }
> >
> > @@ -1787,41 +1770,6 @@ netdev_dpdk_set_mtu(struct netdev *netdev,
> int mtu)
> >  static int
> >  netdev_dpdk_get_carrier(const struct netdev *netdev, bool *carrier);
> >
> > -static int
> > -netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
> > -                            struct netdev_stats *stats)
> > -{
> > -    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > -
> > -    ovs_mutex_lock(&dev->mutex);
> > -
> > -    rte_spinlock_lock(&dev->stats_lock);
> > -    /* Supported Stats */
> > -    stats->rx_packets += dev->stats.rx_packets;
> > -    stats->tx_packets += dev->stats.tx_packets;
> > -    stats->rx_dropped = dev->stats.rx_dropped;
> > -    stats->tx_dropped += dev->stats.tx_dropped;
> > -    stats->multicast = dev->stats.multicast;
> > -    stats->rx_bytes = dev->stats.rx_bytes;
> > -    stats->tx_bytes = dev->stats.tx_bytes;
> > -    stats->rx_errors = dev->stats.rx_errors;
> > -    stats->rx_length_errors = dev->stats.rx_length_errors;
> > -
> > -    stats->rx_1_to_64_packets = dev->stats.rx_1_to_64_packets;
> > -    stats->rx_65_to_127_packets = dev->stats.rx_65_to_127_packets;
> > -    stats->rx_128_to_255_packets = dev->stats.rx_128_to_255_packets;
> > -    stats->rx_256_to_511_packets = dev->stats.rx_256_to_511_packets;
> > -    stats->rx_512_to_1023_packets = dev->stats.rx_512_to_1023_packets;
> > -    stats->rx_1024_to_1522_packets = dev-
> >stats.rx_1024_to_1522_packets;
> > -    stats->rx_1523_to_max_packets = dev-
> >stats.rx_1523_to_max_packets;
> > -
> > -    rte_spinlock_unlock(&dev->stats_lock);
> > -
> > -    ovs_mutex_unlock(&dev->mutex);
> > -
> > -    return 0;
> > -}
> > -
> >  static void
> >  netdev_dpdk_convert_xstats(struct netdev_stats *stats,
> >                             const struct rte_eth_xstat *xstats,
> > @@ -1865,6 +1813,8 @@ netdev_dpdk_convert_xstats(struct
> netdev_stats *stats,
> >              stats->tx_broadcast_packets = xstats[i].value;
> >          } else if (strcmp(XSTAT_RX_UNDERSIZED_ERRORS, names[i].name) ==
> 0) {
> >              stats->rx_undersized_errors = xstats[i].value;
> > +        } else if (strcmp(XSTAT_RX_UNDERSIZE_PACKETS, names[i].name) ==
> 0) {
> > +            stats->rx_undersized_errors = xstats[i].value;
> >          } else if (strcmp(XSTAT_RX_FRAGMENTED_ERRORS, names[i].name)
> == 0) {
> >              stats->rx_fragmented_errors = xstats[i].value;
> >          } else if (strcmp(XSTAT_RX_JABBER_ERRORS, names[i].name) == 0) {
> > @@ -1887,6 +1837,11 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
> >      struct rte_eth_xstat_name *rte_xstats_names = NULL;
> >      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
> >
> > +    if (!rte_eth_dev_is_valid_port(dev->port_id)) {
> > +        ovs_mutex_unlock(&dev->mutex);
> > +        return EPROTO;
> > +    }
> > +
> >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
> >          VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
> >          ovs_mutex_unlock(&dev->mutex);
> > @@ -2097,24 +2052,6 @@ netdev_dpdk_get_carrier(const struct netdev
> *netdev, bool *carrier)
> >      return 0;
> >  }
> >
> > -static int
> > -netdev_dpdk_vhost_get_carrier(const struct netdev *netdev, bool
> *carrier)
> > -{
> > -    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > -
> > -    ovs_mutex_lock(&dev->mutex);
> > -
> > -    if (is_vhost_running(dev)) {
> > -        *carrier = 1;
> > -    } else {
> > -        *carrier = 0;
> > -    }
> > -
> > -    ovs_mutex_unlock(&dev->mutex);
> > -
> > -    return 0;
> > -}
> > -
> >  static long long int
> >  netdev_dpdk_get_carrier_resets(const struct netdev *netdev)
> >  {
> > @@ -2172,12 +2109,10 @@ netdev_dpdk_update_flags__(struct
> netdev_dpdk *dev,
> >
> >          netdev_change_seq_changed(&dev->up);
> >      } 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. */
> > +        /* If the vhost device's NETDEV_UP flag was changed then change
> > +         * netdev's change_seq to trigger link state update. */
> >
> > -        if ((NETDEV_UP & ((*old_flagsp ^ on) | (*old_flagsp ^ off)))
> > -            && is_vhost_running(dev)) {
> > +        if ((NETDEV_UP & ((*old_flagsp ^ on) | (*old_flagsp ^ off)))) {
> >              netdev_change_seq_changed(&dev->up);
> >
> >              /* Clear statistics if device is getting up. */
> > @@ -2230,16 +2165,18 @@ netdev_dpdk_get_status(const struct netdev
> *netdev, struct smap *args)
> >      smap_add_format(args, "max_rx_queues", "%u",
> dev_info.max_rx_queues);
> >      smap_add_format(args, "max_tx_queues", "%u",
> dev_info.max_tx_queues);
> >      smap_add_format(args, "max_mac_addrs", "%u",
> dev_info.max_mac_addrs);
> > -    smap_add_format(args, "max_hash_mac_addrs", "%u",
> > -                           dev_info.max_hash_mac_addrs);
> > -    smap_add_format(args, "max_vfs", "%u", dev_info.max_vfs);
> > -    smap_add_format(args, "max_vmdq_pools", "%u",
> dev_info.max_vmdq_pools);
> >
> > -    if (dev_info.pci_dev) {
> > -        smap_add_format(args, "pci-vendor_id", "0x%u",
> > -                        dev_info.pci_dev->id.vendor_id);
> > -        smap_add_format(args, "pci-device_id", "0x%x",
> > -                        dev_info.pci_dev->id.device_id);
> > +    if (dev->type == DPDK_DEV_ETH) {
> > +        smap_add_format(args, "max_hash_mac_addrs", "%u",
> > +                        dev_info.max_hash_mac_addrs);
> > +        smap_add_format(args, "max_vfs", "%u", dev_info.max_vfs);
> > +        smap_add_format(args, "max_vmdq_pools", "%u",
> dev_info.max_vmdq_pools);
> > +        if (dev_info.pci_dev) {
> > +            smap_add_format(args, "pci-vendor_id", "0x%u",
> > +                            dev_info.pci_dev->id.vendor_id);
> > +            smap_add_format(args, "pci-device_id", "0x%x",
> > +                            dev_info.pci_dev->id.device_id);
> > +        }
> >      }
> >
> >      return 0;
> > @@ -2303,22 +2240,6 @@ netdev_dpdk_set_admin_state(struct
> unixctl_conn *conn, int argc,
> >  }
> >
> >  /*
> > - * Set virtqueue flags so that we do not receive interrupts.
> > - */
> > -static void
> > -set_irq_status(int vid)
> > -{
> > -    uint32_t i;
> > -    uint64_t idx;
> > -
> > -    for (i = 0; i < rte_vhost_get_queue_num(vid); i++) {
> > -        idx = i * VIRTIO_QNUM;
> > -        rte_vhost_enable_guest_notification(vid, idx + VIRTIO_RXQ, 0);
> > -        rte_vhost_enable_guest_notification(vid, idx + VIRTIO_TXQ, 0);
> > -    }
> > -}
> > -
> > -/*
> >   * Fixes mapping for vhost-user tx queues. Must be called after each
> >   * enabling/disabling of queues and n_txq modifications.
> >   */
> > @@ -2359,73 +2280,6 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk
> *dev)
> >      free(enabled_queues);
> >  }
> >
> > -/*
> > - * A new virtio-net device is added to a vhost port.
> > - */
> > -static int
> > -new_device(int vid)
> > -{
> > -    struct netdev_dpdk *dev;
> > -    bool exists = false;
> > -    int newnode = 0;
> > -    char ifname[IF_NAME_SZ];
> > -
> > -    rte_vhost_get_ifname(vid, ifname, sizeof ifname);
> > -
> > -    ovs_mutex_lock(&dpdk_mutex);
> > -    /* Add device to the vhost port with the same name as that passed
> down. */
> > -    LIST_FOR_EACH(dev, list_node, &dpdk_list) {
> > -        ovs_mutex_lock(&dev->mutex);
> > -        if (strncmp(ifname, dev->vhost_id, IF_NAME_SZ) == 0) {
> > -            uint32_t qp_num = rte_vhost_get_queue_num(vid);
> > -
> > -            /* Get NUMA information */
> > -            newnode = rte_vhost_get_numa_node(vid);
> > -            if (newnode == -1) {
> > -#ifdef VHOST_NUMA
> > -                VLOG_INFO("Error getting NUMA info for vHost Device '%s'",
> > -                          ifname);
> > -#endif
> > -                newnode = dev->socket_id;
> > -            }
> > -
> > -            if (dev->requested_n_txq != qp_num
> > -                || dev->requested_n_rxq != qp_num
> > -                || dev->requested_socket_id != newnode) {
> > -                dev->requested_socket_id = newnode;
> > -                dev->requested_n_rxq = qp_num;
> > -                dev->requested_n_txq = qp_num;
> > -                netdev_request_reconfigure(&dev->up);
> > -            } else {
> > -                /* Reconfiguration not required. */
> > -                dev->vhost_reconfigured = true;
> > -            }
> > -
> > -            ovsrcu_index_set(&dev->vid, vid);
> > -            exists = true;
> > -
> > -            /* Disable notifications. */
> > -            set_irq_status(vid);
> > -            netdev_change_seq_changed(&dev->up);
> > -            ovs_mutex_unlock(&dev->mutex);
> > -            break;
> > -        }
> > -        ovs_mutex_unlock(&dev->mutex);
> > -    }
> > -    ovs_mutex_unlock(&dpdk_mutex);
> > -
> > -    if (!exists) {
> > -        VLOG_INFO("vHost Device '%s' can't be added - name not found",
> ifname);
> > -
> > -        return -1;
> > -    }
> > -
> > -    VLOG_INFO("vHost Device '%s' has been added on numa node %i",
> > -              ifname, newnode);
> > -
> > -    return 0;
> > -}
> > -
> >  /* Clears mapping for all available queues of vhost interface. */
> >  static void
> >  netdev_dpdk_txq_map_clear(struct netdev_dpdk *dev)
> > @@ -2438,133 +2292,12 @@ netdev_dpdk_txq_map_clear(struct
> netdev_dpdk *dev)
> >      }
> >  }
> >
> > -/*
> > - * Remove a virtio-net device from the specific vhost port.  Use dev-
> >remove
> > - * flag to stop any more packets from being sent or received to/from a VM
> and
> > - * ensure all currently queued packets have been sent/received before
> removing
> > - *  the device.
> > - */
> > -static void
> > -destroy_device(int vid)
> > -{
> > -    struct netdev_dpdk *dev;
> > -    bool exists = false;
> > -    char ifname[IF_NAME_SZ];
> > -
> > -    rte_vhost_get_ifname(vid, ifname, sizeof ifname);
> > -
> > -    ovs_mutex_lock(&dpdk_mutex);
> > -    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > -        if (netdev_dpdk_get_vid(dev) == vid) {
> > -
> > -            ovs_mutex_lock(&dev->mutex);
> > -            dev->vhost_reconfigured = false;
> > -            ovsrcu_index_set(&dev->vid, -1);
> > -            netdev_dpdk_txq_map_clear(dev);
> > -
> > -            netdev_change_seq_changed(&dev->up);
> > -            ovs_mutex_unlock(&dev->mutex);
> > -            exists = true;
> > -            break;
> > -        }
> > -    }
> > -
> > -    ovs_mutex_unlock(&dpdk_mutex);
> > -
> > -    if (exists) {
> > -        /*
> > -         * Wait for other threads to quiesce after setting the 'virtio_dev'
> > -         * to NULL, before returning.
> > -         */
> > -        ovsrcu_synchronize();
> > -        /*
> > -         * As call to ovsrcu_synchronize() will end the quiescent state,
> > -         * put thread back into quiescent state before returning.
> > -         */
> > -        ovsrcu_quiesce_start();
> > -        VLOG_INFO("vHost Device '%s' has been removed", ifname);
> > -    } else {
> > -        VLOG_INFO("vHost Device '%s' not found", ifname);
> > -    }
> > -}
> > -
> > -static int
> > -vring_state_changed(int vid, uint16_t queue_id, int enable)
> > -{
> > -    struct netdev_dpdk *dev;
> > -    bool exists = false;
> > -    int qid = queue_id / VIRTIO_QNUM;
> > -    char ifname[IF_NAME_SZ];
> > -
> > -    rte_vhost_get_ifname(vid, ifname, sizeof ifname);
> > -
> > -    if (queue_id % VIRTIO_QNUM == VIRTIO_TXQ) {
> > -        return 0;
> > -    }
> > -
> > -    ovs_mutex_lock(&dpdk_mutex);
> > -    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > -        ovs_mutex_lock(&dev->mutex);
> > -        if (strncmp(ifname, dev->vhost_id, IF_NAME_SZ) == 0) {
> > -            if (enable) {
> > -                dev->tx_q[qid].map = qid;
> > -            } else {
> > -                dev->tx_q[qid].map = OVS_VHOST_QUEUE_DISABLED;
> > -            }
> > -            netdev_dpdk_remap_txqs(dev);
> > -            exists = true;
> > -            ovs_mutex_unlock(&dev->mutex);
> > -            break;
> > -        }
> > -        ovs_mutex_unlock(&dev->mutex);
> > -    }
> > -    ovs_mutex_unlock(&dpdk_mutex);
> > -
> > -    if (exists) {
> > -        VLOG_INFO("State of queue %d ( tx_qid %d ) of vhost device '%s'"
> > -                  "changed to \'%s\'", queue_id, qid, ifname,
> > -                  (enable == 1) ? "enabled" : "disabled");
> > -    } else {
> > -        VLOG_INFO("vHost Device '%s' not found", ifname);
> > -        return -1;
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> > -int
> > -netdev_dpdk_get_vid(const struct netdev_dpdk *dev)
> > -{
> > -    return ovsrcu_index_get(&dev->vid);
> > -}
> > -
> >  struct ingress_policer *
> >  netdev_dpdk_get_ingress_policer(const struct netdev_dpdk *dev)
> >  {
> >      return ovsrcu_get(struct ingress_policer *, &dev->ingress_policer);
> >  }
> >
> > -/*
> > - * These callbacks allow virtio-net devices to be added to vhost ports when
> > - * configuration has been fully complete.
> > - */
> > -static const struct virtio_net_device_ops virtio_net_device_ops =
> > -{
> > -    .new_device =  new_device,
> > -    .destroy_device = destroy_device,
> > -    .vring_state_changed = vring_state_changed
> > -};
> > -
> > -static void *
> > -start_vhost_loop(void *dummy OVS_UNUSED)
> > -{
> > -     pthread_detach(pthread_self());
> > -     /* Put the vhost thread into quiescent state. */
> > -     ovsrcu_quiesce_start();
> > -     rte_vhost_driver_session_start();
> > -     return NULL;
> > -}
> > -
> >  static int
> >  netdev_dpdk_class_init(void)
> >  {
> > @@ -2587,20 +2320,10 @@ netdev_dpdk_class_init(void)
> >  static int
> >  netdev_dpdk_vhost_class_init(void)
> >  {
> > -    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> > -
> > -    /* This function can be called for different classes.  The initialization
> > -     * needs to be done only once */
> > -    if (ovsthread_once_start(&once)) {
> > -        rte_vhost_driver_callback_register(&virtio_net_device_ops);
> > -        rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
> > -                                  | 1ULL << VIRTIO_NET_F_HOST_TSO6
> > -                                  | 1ULL << VIRTIO_NET_F_CSUM
> > -                                  | 1ULL << VIRTIO_RING_F_INDIRECT_DESC);
> > -        ovs_thread_create("vhost_thread", start_vhost_loop, NULL);
> > -
> > -        ovsthread_once_done(&once);
> > -    }
> > +    rte_eth_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
> > +                                | 1ULL << VIRTIO_NET_F_HOST_TSO6
> > +                                | 1ULL << VIRTIO_NET_F_CSUM
> > +                                | 1ULL << VIRTIO_RING_F_INDIRECT_DESC);
> 
> I'd keep the ovsthread_once construct, especially if we're going to register
> the vhost id_pool here.

Got it - will include this change in v2.

> 
> >
> >      return 0;
> >  }
> > @@ -2702,7 +2425,17 @@ netdev_dpdk_ring_send(struct netdev
> *netdev, int qid,
> >          dp_packet_rss_invalidate(batch->packets[i]);
> >      }
> >
> > -    netdev_dpdk_send__(dev, qid, batch, may_steal, concurrent_txq);
> > +    if (OVS_UNLIKELY(concurrent_txq)) {
> > +        qid = qid % dev->up.n_txq;
> > +        rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +    }
> > +
> > +    netdev_dpdk_send__(dev, qid, batch, may_steal);
> > +
> > +    if (OVS_UNLIKELY(concurrent_txq)) {
> > +        rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > +    }
> > +
> >      return 0;
> >  }
> >
> > @@ -2997,10 +2730,6 @@ dpdk_vhost_reconfigure_helper(struct
> netdev_dpdk *dev)
> >              netdev_change_seq_changed(&dev->up);
> >          }
> >      }
> > -
> > -    if (netdev_dpdk_get_vid(dev) >= 0) {
> > -        dev->vhost_reconfigured = true;
> > -    }
> >  }
> >
> >  static int
> > @@ -3019,6 +2748,7 @@ netdev_dpdk_vhost_client_reconfigure(struct
> netdev *netdev)
> >  {
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >      int err = 0;
> > +    int sid = -1;
> >
> >      ovs_mutex_lock(&dev->mutex);
> >
> > @@ -3032,20 +2762,51 @@ netdev_dpdk_vhost_client_reconfigure(struct
> netdev *netdev)
> >      if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)
> >              && strlen(dev->vhost_id)) {
> >          /* Register client-mode device */
> > -        err = rte_vhost_driver_register(dev->vhost_id,
> > -                                        RTE_VHOST_USER_CLIENT);
> > -        if (err) {
> > -            VLOG_ERR("vhost-user device setup failure for device %s\n",
> > -                     dev->vhost_id);
> > -        } else {
> > -            /* Configuration successful */
> > +        err = dpdk_attach_vhost_pmd(dev, 1);
> > +
> > +        if (!err) {
> > +            sid = rte_eth_dev_socket_id(dev->port_id);
> > +            dev->socket_id = sid < 0 ? SOCKET0 : sid;
> >              dev->vhost_driver_flags |= RTE_VHOST_USER_CLIENT;
> > +
> > +            rte_eth_dev_stop(dev->port_id);
> > +            rte_free(dev->tx_q);
> > +            err = dpdk_eth_dev_init(dev);
> > +            dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> > +            if (!dev->tx_q) {
> > +                rte_eth_dev_detach(dev->port_id, dev->vhost_id);
> > +                if (dev->vhost_pmd_id >= 0) {
> > +                    id_pool_free_id(dpdk_get_vhost_id_pool(),
> > +                                    dev->vhost_pmd_id);
> > +                }
> > +                goto out;
> > +            } else {
> > +                rte_eth_dev_callback_register(dev->port_id,
> > +                                              RTE_ETH_EVENT_QUEUE_STATE,
> > +                                              vring_state_changed_callback,
> > +                                              NULL);
> > +                rte_eth_dev_callback_register(dev->port_id,
> > +                                              RTE_ETH_EVENT_INTR_LSC,
> > +                                              link_status_changed_callback,
> > +                                              NULL);
> > +            }
> > +
> > +            /* Enable TX queue 0 by default if it wasn't disabled. */
> > +            if (dev->tx_q[0].map == OVS_VHOST_QUEUE_MAP_UNKNOWN) {
> > +                dev->tx_q[0].map = 0;
> > +            }
> > +
> > +            netdev_dpdk_remap_txqs(dev);
> > +
> > +            netdev_change_seq_changed(netdev);
> > +
> 
> The error handling here is rather complicated.
> I think it'd be easier to goto out every time there's an error instead
> of continue in the else condition.
> Also the function should return the error value instead of 0.

Sure.

> 
> >              VLOG_INFO("vHost User device '%s' created in 'client' mode, "
> >                        "using client socket '%s'",
> >                        dev->up.name, dev->vhost_id);
> >          }
> >      }
> >
> > +out:
> >      ovs_mutex_unlock(&dev->mutex);
> >
> >      return 0;
> > @@ -3165,12 +2926,13 @@ static const struct netdev_class
> dpdk_vhost_class =
> >          NULL,
> >          NULL,
> >          netdev_dpdk_vhost_send,
> > -        netdev_dpdk_vhost_get_carrier,
> > -        netdev_dpdk_vhost_get_stats,
> > -        NULL,
> > -        NULL,
> > +        netdev_dpdk_get_carrier,
> > +        netdev_dpdk_get_stats,
> > +        netdev_dpdk_get_features,
> > +        netdev_dpdk_get_status,
> >          netdev_dpdk_vhost_reconfigure,
> > -        netdev_dpdk_vhost_rxq_recv);
> > +        netdev_dpdk_rxq_recv);
> > +
> >  static const struct netdev_class dpdk_vhost_client_class =
> >      NETDEV_DPDK_CLASS(
> >          "dpdkvhostuserclient",
> > @@ -3180,12 +2942,12 @@ static const struct netdev_class
> dpdk_vhost_client_class =
> >          netdev_dpdk_vhost_client_set_config,
> >          NULL,
> >          netdev_dpdk_vhost_send,
> > -        netdev_dpdk_vhost_get_carrier,
> > -        netdev_dpdk_vhost_get_stats,
> > -        NULL,
> > -        NULL,
> > +        netdev_dpdk_get_carrier,
> > +        netdev_dpdk_get_stats,
> > +        netdev_dpdk_get_features,
> > +        netdev_dpdk_get_status,
> >          netdev_dpdk_vhost_client_reconfigure,
> > -        netdev_dpdk_vhost_rxq_recv);
> > +        netdev_dpdk_vhost_client_rxq_recv);
> >
> >  void
> >  netdev_dpdk_register(void)
> > --
> > 2.4.3
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list