> -----Original Message-----
> From: Loftus, Ciara
> Sent: Tuesday, May 10, 2016 10:22 AM
> To: Traynor, Kevin <kevin.tray...@intel.com>
> Cc: dev@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.lof...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
