On 12.02.2016 09:00, Ilya Maximets 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.
I meant, make rte_vhost_dequeue_burst/rte_vhost_enqueue_burst thread safe.
>
> Best regards, Ilya Maximets.
>
>>
>> Anyway, this patch is more important and it works for me.
>>
>> Acked-by: Flavio Leitner <f...@sysclose.org>
>>
>> This should go in 2.5 but I am not sure how much time we have left for
>> the actual release.
>>
>> Thanks,
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
