On Fri, 12 Feb 2016 09:00:48 +0300 Ilya Maximets <i.maxim...@samsung.com> wrote:
> Hi, Flavio. > > Comment inlined. > > On 12.02.2016 07:44, Flavio Leitner wrote: > > > > Hi Ilya, > > > > On Thu, 11 Feb 2016 16:04:12 +0300 > > Ilya Maximets <i.maxim...@samsung.com> wrote: > > > >> Currently virtio driver in guest operating system have to be configured > >> to use exactly same number of queues. If number of queues will be less, > >> some packets will get stuck in queues unused by guest and will not be > >> received. > >> > >> Fix that by using new 'vring_state_changed' callback, which is > >> available for vhost-user since DPDK 2.2. > >> Implementation uses additional mapping from configured tx queues to > >> enabled by virtio driver. This requires mandatory locking of TX queues > >> in __netdev_dpdk_vhost_send(), but this locking was almost always anyway > >> because of calling set_multiq with n_txq = 'ovs_numa_get_n_cores() + 1'. > >> > >> Fixes: 4573fbd38fa1 ("netdev-dpdk: Add vhost-user multiqueue support") > >> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com> > >> --- > >> lib/netdev-dpdk.c | 108 > >> +++++++++++++++++++++++++++++++++++++++++++++++------- > >> 1 file changed, 95 insertions(+), 13 deletions(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index e4f789b..f04f2bd 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -173,6 +173,8 @@ 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]; > >> }; > >> @@ -559,6 +561,13 @@ netdev_dpdk_alloc_txq(struct netdev_dpdk *netdev, > >> unsigned int n_txqs) > >> unsigned i; > >> > >> netdev->tx_q = dpdk_rte_mzalloc(n_txqs * sizeof *netdev->tx_q); > >> + > >> + if (netdev->type == DPDK_DEV_VHOST) { > >> + for (i = 0; i < n_txqs; i++) { > >> + netdev->tx_q[i].map = -1; > >> + } > >> + } > >> + > > > > There is the same loop down below which initializes other > > queue variables, so the above could be included in the latter loop: > > > > for (i = 0; i < n_txqs; i++) { > > int numa_id = ovs_numa_get_numa_id(i); > > > > if (!netdev->txq_needs_locking) { > > netdev->tx_q[i].flush_tx = netdev->socket_id == numa_id; > > } else { > > netdev->tx_q[i].flush_tx = true; > > } > > > > /* initialize map for vhost devices */ > > netdev->tx_q[i].map = -1; > > rte_spinlock_init(&netdev->tx_q[i].tx_lock); > > } > > > > It seems cleaner to me, but I have no strong opinion here. > > Yes, I think you're right. I'll send v2 with this fix. > Thanks for review. > > > > > I had a plan to actually change how many TX queues we are allocating > > which then would allow to not use locking at all. The reason for having > > n_txq = 'ovs_numa_get_n_cores() + 1' is to make sure that regardless the > > core that the PMD is running, we would have an unique TX queue. Since > > you proposed the patch to have sequential queue ids, we wouldn't need to > > allocate that many queues anymore. > > To make send lockless we need number of queues not less than number of > PMD threads. But this number is still may be bigger than number of actual > queues in device. I think, it's better to make dpdk_send for vhost thread > safe. It doesn't matter how many TX queues we have in the device. If you have a single stream, only one queue is used and that's it. The idea was to enable 1:1 queue to PMD, so we wouldn't have a situation where two threads send traffic over the same queue. -- fbl _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev