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

Traynor, Kevin kevin.traynor at intel.com
Fri May 13 13:13:54 UTC 2016


> -----Original Message-----
> From: Loftus, Ciara
> Sent: Tuesday, May 10, 2016 10:22 AM
> To: Traynor, Kevin <kevin.traynor at intel.com>
> Cc: dev at openvswitch.org
> Subject: RE: [ovs-dev] [PATCH] netdev-dpdk: Add vHost User PMD
> 
> > On 21/04/2016 13:20, Ciara Loftus wrote:
> > > DPDK 16.04 introduces the vHost PMD which allows 'dpdkvhostuser'
> ports
> > > to be controlled by the librte_ether API, like physical 'dpdk'
> ports.
> > > The commit integrates this functionality into OVS, and refactors
> some
> > > of the existing vhost code such that it is vhost-cuse specific.
> > > Similarly, there is now some overlap between dpdk and vhost-user
> port
> > > code.
> > >
> > > Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>
> > > ---
> > >   INSTALL.DPDK.md   |  12 ++
> > >   NEWS              |   2 +
> > >   lib/netdev-dpdk.c | 515 +++++++++++++++++++++++++---------------
> ------
> > --------
> >
> > Hi Ciara, there's a lot of churn in this file. It might be worth
> > considering to see if it could be split through a few commits
> commits to
> > help reviewers. e.g. new features like adding get_features,
> get_status
> > for vhost could be a separate patch at least.
> 
> I've split into 3:
> - remove watchdog
> - add pmd
> - add get_stats & get_features

Great - this helps review a lot

> 
> Couldn't quite find a way to split it up more.
> 
> >
> > >   3 files changed, 254 insertions(+), 275 deletions(-)
> > >   mode change 100644 => 100755 lib/netdev-dpdk.c
> >
> > file permission change.
> 
> Woops. Fixed in v2.
> 
> >
> > >
> > > diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
> > > index 7f76df8..5006812 100644
> > > --- a/INSTALL.DPDK.md
> > > +++ b/INSTALL.DPDK.md
> > > @@ -945,6 +945,18 @@ Restrictions:
> > >       increased to the desired number of queues. Both DPDK and OVS
> must
> > be
> > >       recompiled for this change to take effect.
> > >
> > > +  DPDK 'eth' type ports:
> > > +  - dpdk, dpdkr and dpdkvhostuser ports are 'eth' type ports in
> the context
> > of
> > > +    DPDK as they are all managed by the rte_ether API. This means
> that
> > they
> > > +    adhere to the DPDK configuration option
> CONFIG_RTE_MAX_ETHPORTS
> > which by
> > > +    default is set to 32. This means by default the combined
> total number of
> > > +    dpdk, dpdkr and dpdkvhostuser ports allowable in OVS with
> DPDK is 32.
> > This
> > > +    value can be changed if desired by modifying the
> configuration file in
> > > +    DPDK, or by overriding the default value on the command line
> when
> > building
> > > +    DPDK. eg.
> > > +
> > > +        `make install CONFIG_RTE_MAX_ETHPORTS=64`
> >
> > format is not registering right for this in my md viewer.
> 
> It's looking ok on mine. What doesn't look right?

The ``'s are showing in atom.io - could be just atom or schema related.

> 
> >
> > > +
> > >   Bug Reporting:
> > >   --------------
> > >
> > > diff --git a/NEWS b/NEWS
> > > index ea7f3a1..4dc0201 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -26,6 +26,8 @@ Post-v2.5.0
> > >          assignment.
> > >        * Type of log messages from PMD threads changed from INFO
> to DBG.
> > >        * QoS functionality with sample egress-policer
> implementation.
> > > +     * vHost PMD integration brings vhost-user ports under
> control of the
> > > +       rte_ether DPDK API.
> > >      - ovs-benchmark: This utility has been removed due to lack of
> use and
> > >        bitrot.
> > >      - ovs-appctl:
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > > old mode 100644
> > > new mode 100755
> > > index 208c5f5..4fccd63
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -56,6 +56,7 @@
> > >   #include "rte_mbuf.h"
> > >   #include "rte_meter.h"
> > >   #include "rte_virtio_net.h"
> > > +#include "rte_eth_vhost.h"
> >
> > nit: generally these go in alphabetical order.
> 
> Ok
> 
> >
> > >
> > >   VLOG_DEFINE_THIS_MODULE(dpdk);
> > >   static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> > > @@ -109,6 +110,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
> > ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
> > >
> > >   static char *cuse_dev_name = NULL;    /* Character device
> > cuse_dev_name. */
> > >   static char *vhost_sock_dir = NULL;   /* Location of vhost-user
> sockets */
> > > +/* Array that tracks the used & unused vHost user driver IDs */
> > > +static unsigned int vhost_user_drv_ids[RTE_MAX_ETHPORTS];
> > >
> > >   /*
> > >    * Maximum amount of time in micro seconds to try and enqueue to
> > vhost.
> > > @@ -143,7 +146,8 @@ enum { DRAIN_TSC = 200000ULL };
> > >
> > >   enum dpdk_dev_type {
> > >       DPDK_DEV_ETH = 0,
> > > -    DPDK_DEV_VHOST = 1,
> > > +    DPDK_DEV_VHOST_USER = 1,
> > > +    DPDK_DEV_VHOST_CUSE = 2,
> > >   };
> > >
> > >   static int rte_eal_init_ret = ENODEV;
> > > @@ -275,8 +279,6 @@ struct dpdk_tx_queue {
> > >                                       * from concurrent access.
> It is used only
> > >                                       * if the queue is shared
> among different
> > >                                       * pmd threads (see
> 'txq_needs_locking'). */
> > > -    int map;                       /* Mapping of configured
> vhost-user queues
> > > -                                    * to enabled by guest. */
> > >       uint64_t tsc;
> > >       struct rte_mbuf *burst_pkts[MAX_TX_QUEUE_LEN];
> > >   };
> > > @@ -329,12 +331,22 @@ struct netdev_dpdk {
> > >       int real_n_rxq;
> > >       bool txq_needs_locking;
> > >
> > > -    /* virtio-net structure for vhost device */
> > > +    /* Spinlock for vhost cuse transmission. Other DPDK devices
> use
> > spinlocks
> > > +     * in dpdk_tx_queue */
> > > +    rte_spinlock_t vhost_cuse_tx_lock;
> >
> > Why can't we continue to use the lock in dpdk_tx_queue? rather than
> > adding a cuse specific lock.
> 
> This logic is taken from netdev-dpdk pre-multiqueue. I assumed it was
> the safest way to go since it's tried and tested.

The queue already has a lock, so I don't see the need to introduce
a new one. Anyway, it could be a moot point if people think cuse can be
removed.



More information about the dev mailing list