Re: [ovs-dev] [PATCH RFC v6 2/2] netdev-dpdk: Add vHost User PMD

2016-10-28 Thread Loftus, Ciara 
[snip]

> > +
> > +static int
> >  netdev_dpdk_vhost_construct(struct netdev *netdev)
> >  {
> >  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > @@ -904,7 +1051,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);
> > @@ -917,18 +1064,26 @@ 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) {
> 
> I think, that callbacks are not registered at this point in case of failure.
> Anyway, IMHO, 'netdev_dpdk_init' should be responsible for unregistering.

Ok

> 
> > +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_eth_dev_detach(dev->port_id, dev->vhost_id);
> > +}
> >
> >  ovs_mutex_unlock(_mutex);
> >  return err;
> > @@ -940,7 +1095,7 @@ netdev_dpdk_vhost_client_construct(struct
> netdev *netdev)
> >  int err;
> >
> >  ovs_mutex_lock(_mutex);
> > -err = netdev_dpdk_init(netdev, -1, DPDK_DEV_VHOST);
> > +err = netdev_dpdk_init(netdev, -1, DPDK_DEV_VHOST_CLIENT);
> >  ovs_mutex_unlock(_mutex);
> >  return err;
> >  }
> > @@ -964,13 +1119,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(_mutex);
> > -ovs_mutex_lock(>mutex);
> > -
> >  rte_eth_dev_stop(dev->port_id);
> >  free(ovsrcu_get_protected(struct ingress_policer *,
> >>ingress_policer));
> > @@ -978,61 +1130,59 @@ netdev_dpdk_destruct(struct netdev *netdev)
> >  rte_free(dev->tx_q);
> >  ovs_list_remove(>list_node);
> >  dpdk_mp_put(dev->dpdk_mp);
> > -
> > -ovs_mutex_unlock(>mutex);
> > -ovs_mutex_unlock(_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.
> */
> > -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(_mutex);
> > +ovs_mutex_lock(>mutex);
> > +
> > +dpdk_destruct_helper(dev);
> > +
> > +ovs_mutex_unlock(>mutex);
> > +ovs_mutex_unlock(_mutex);
> >  }
> >
> >  static void
> >  netdev_dpdk_vhost_destruct(struct netdev *netdev)
> >  {
> >  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > -char *vhost_id;
> >
> >  ovs_mutex_lock(_mutex);
> >  ovs_mutex_lock(>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

Re: [ovs-dev] [PATCH RFC v6 2/2] netdev-dpdk: Add vHost User PMD

2016-10-28 Thread Ilya Maximets 
Still not a full review.
Comments inline.

Bet regards, Ilya Maximets.

On 28.10.2016 16:03, Ciara Loftus wrote:
> 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 
> 
> ---
> v6:
> * Unregister callbacks before detach
> 
> v5:
> * 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.
> 
> v4:
> * Use id-pool implementation for allocating vHost PMD IDs
> 
> v3:
> * 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|  10 +
>  lib/dpdk.h|   1 +
>  lib/netdev-dpdk.c | 977 
> +-
>  4 files changed, 388 insertions(+), 602 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 037bcda..5f84b0b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -137,6 +137,8 @@ v2.6.0 - 27 Sep 2016
>   * Remove dpdkvhostcuse port type.
>   * OVS client mode for vHost and vHost reconnect (Requires QEMU 2.7)
>   * 'dpdkvhostuserclient' port type.
> + * vHost PMD integration brings vhost-user ports under control of the
> +   rte_ether DPDK API.
> - Increase number of registers to 16.
> - ovs-benchmark: This utility has been removed due to lack of use and
>   bitrot.
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 49a589a..831257a 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,8 @@ dpdk_init__(const struct smap *ovs_other_config)
>  }
>  #endif
>  
> +vhost_driver_ids = id_pool_create(0, RTE_MAX_ETHPORTS - 1);
> +
>  /* Finally, register the dpdk classes */
>  netdev_dpdk_register();
>  }
> @@ -428,6 +432,12 @@ dpdk_get_vhost_sock_dir(void)
>  return vhost_sock_dir;
>  }
>  
> +struct id_pool *
> +dpdk_get_vhost_id_pool(void)
> +{
> +return vhost_driver_ids;
> +}
> +
>  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 7c1523e..0b1af1d 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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 = 20ULL };
>  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 */
> -

[ovs-dev] [PATCH RFC v6 2/2] netdev-dpdk: Add vHost User PMD

2016-10-28 Thread Ciara Loftus 
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 

---
v6:
* Unregister callbacks before detach

v5:
* 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.

v4:
* Use id-pool implementation for allocating vHost PMD IDs

v3:
* 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|  10 +
 lib/dpdk.h|   1 +
 lib/netdev-dpdk.c | 977 +-
 4 files changed, 388 insertions(+), 602 deletions(-)

diff --git a/NEWS b/NEWS
index 037bcda..5f84b0b 100644
--- a/NEWS
+++ b/NEWS
@@ -137,6 +137,8 @@ v2.6.0 - 27 Sep 2016
  * Remove dpdkvhostcuse port type.
  * OVS client mode for vHost and vHost reconnect (Requires QEMU 2.7)
  * 'dpdkvhostuserclient' port type.
+ * vHost PMD integration brings vhost-user ports under control of the
+   rte_ether DPDK API.
- Increase number of registers to 16.
- ovs-benchmark: This utility has been removed due to lack of use and
  bitrot.
diff --git a/lib/dpdk.c b/lib/dpdk.c
index 49a589a..831257a 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,8 @@ dpdk_init__(const struct smap *ovs_other_config)
 }
 #endif
 
+vhost_driver_ids = id_pool_create(0, RTE_MAX_ETHPORTS - 1);
+
 /* Finally, register the dpdk classes */
 netdev_dpdk_register();
 }
@@ -428,6 +432,12 @@ dpdk_get_vhost_sock_dir(void)
 return vhost_sock_dir;
 }
 
+struct id_pool *
+dpdk_get_vhost_id_pool(void)
+{
+return vhost_driver_ids;
+}
+
 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 7c1523e..0b1af1d 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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 = 20ULL };
 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);
 
@@