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
