[ovs-dev] [PATCH RFC] netdev-dpdk: vhost-user: Fix sending packets to queues not enabled by guest.

[Ilya Maximets]

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.maximets at 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.maximets at 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.

Best regards, Ilya Maximets.

> 
> Anyway, this patch is more important and it works for me.
> 
> Acked-by: Flavio Leitner <fbl at sysclose.org>
> 
> This should go in 2.5 but I am not sure how much time we have left for
> the actual release.
> 
> Thanks,
>