>On 26.06.2017 00:52, Bodireddy, Bhanuprakash wrote:
>>>> +
>>>> +/* Flush the txq if there are any packets available.
>>>> + * dynamic_txqs/concurrent_txq is disabled for vHost User ports as
>>>> + * 'OVS_VHOST_MAX_QUEUE_NUM' txqs are preallocated.
>>>> + */
>>>
>>> This comment is completely untrue. You may ignore 'concurrent_txq'
>>> because you *must* lock the queue in any case because of dynamic txq
>>> remapping inside netdev-dpdk. You must take the spinlock for the
>>> 'dev-
>>>> tx_q[qid % netdev->n_txq].map' before sending packets.
>>
>> Thanks for catching this and the lock should be taken before flushing the
>queue. Below is how the new logic with spinlocks.
>>
>> /* Flush the txq if there are any packets available. */ static int
>> netdev_dpdk_vhost_txq_flush(struct netdev *netdev, int qid,
>> bool concurrent_txq OVS_UNUSED) {
>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> struct dpdk_tx_queue *txq;
>>
>> qid = dev->tx_q[qid % netdev->n_txq].map;
>>
>> txq = &dev->tx_q[qid];
>> if (OVS_LIKELY(txq->vhost_pkt_cnt)) {
>> rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
>> netdev_dpdk_vhost_tx_burst(dev, qid);
>> rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
>> }
>>
>> return 0;
>> }
>>
>>>
>>> In current implementation you're able to call send and flush
>>> simultaneously for the same queue from different threads because
>>> 'flush' doesn't care about queue remapping.
>>>
>>> See '__netdev_dpdk_vhost_send' and 'netdev_dpdk_remap_txqs' for
>detail.
>>>
>>> Additionally, flushing logic will be broken in case of txq remapping
>>> because you may have different underneath queue each time you trying
>>> to send of flush.
>>
>> I remember you raised this point earlier. To handle this case,
>> 'last_used_qid'
>was introduced in tx_port.
>> With this we can track any change in qid and make sure the packets are
>flushed in the old queue.
>> This logic is in patch 3/6 of this series.
>
>I'm talking about txq remapping inside netdev-dpdk not about XPS.
>You're trying to flush the 'qid = tx_q[...].map' but the 'map'
>value can be changed at any time because of enabling/disabling vrings inside
>guest. Refer the 'vring_state_changed()' callback that triggers
>'netdev_dpdk_remap_txqs' which I already mentioned.
>It's actually the reason why we're using unconditional locking for vhost-user
>ports ignoring 'concurrent_txq' value.
I spent some time looking at the above mentioned functions and see that 'qid =
tx_q[...].map' is updated by 'vhost_thread' as part of callback function. This
gets triggered only when the queues are enabled/disabled in the guest. I was
using 'ethtool -L' in the guest for testing this.
As you rightly mentioned the flushing logic breaks and the packets may
potentially get stuck in the queue. During my testing I found this corner case
that poses a problem with the patch (steps in order).
- Multiqueue is enabled on the guest with 2 rx,tx queues.
- In PVP case, Traffic is sent to VM and the packets are sent on the 'queue 1'
of the vHostUser port.
- PMD thread keeps queueing packets on the vhostuser port queue. [using
netdev_dpdk_vhost_send()]
- Meanwhile user changes queue configuration using 'ethtool -L ens3 combined
1', enables only one queue now and disabling the other.
- Callback functions gets triggered and disables the queue 1.
- 'tx_q[...].map' is updated to '0' [queue 0] - default queue.
- Meantime PMD thread reads the map and finds that tx_q[..].map == '0' and
flushes the packets in the queue0.
This is the problem as the packets enqueued earlier on queue1 were not flushed.
How about the below fix?
- Before disabling the queues (qid = OVS_VHOST_QUEUE_DISABLED), flush the
packets in the queue from vring_state_changed().
--------------------------vring_state_changed()------------------------------------
if (strncmp(ifname, dev->vhost_id, IF_NAME_SZ) == 0) {
if (enable) {
dev->tx_q[qid].map = qid;
} else {
/* If the queue is disabled in the guest, the corresponding qid
* map should be set to OVS_VHOST_QUEUE_DISABLED(-2).
*
* The packets that were queued in 'qid' can be potentially
* stuck and should be flushed before it is disabled.
*/
netdev_dpdk_vhost_txq_flush(&dev->up, dev->tx_q[qid].map, 0);
dev->tx_q[qid].map = OVS_VHOST_QUEUE_DISABLED;
}
netdev_dpdk_remap_txqs(dev);
exists = true;
ovs_mutex_unlock(&dev->mutex);
break;
}
ovs_mutex_unlock(&dev->mutex);
----------------------------------------------------------------------------------------------
Also update the txq_flush function to return immediately if queue is disabled
(qid == OVS_VHOST_QUEUE_DISABLED)
-----------------------------netdev_dpdk_vhost_txq_flush()-----------------------------
netdev_dpdk_vhost_txq_flush(struct netdev *netdev, int qid,
bool concurrent_txq OVS_UNUSED)
{
struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
struct dpdk_tx_queue *txq;
qid = dev->tx_q[qid % netdev->n_txq].map;
/* The qid may be disabled in the guest and has been set to
* OVS_VHOST_QUEUE_DISABLED.
*/
if (OVS_UNLIKELY(qid < 0)) {
return 0;
}
txq = &dev->tx_q[qid];
if (OVS_LIKELY(txq->vhost_pkt_cnt)) {
rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
netdev_dpdk_vhost_tx_burst(dev, qid);
rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
}
return 0;
}
--------------------------------------------------------------------------------------------------
Regards,
Bhanuprakash.
>
>>> Have you ever tested this with multiqueue vhost?
>>> With disabling/enabling queues inside the guest?
>>
>> I did basic sanity testing with vhost multiqueue to verify throughput and to
>check if flush logic is working at low rate(1 pkt each is sent to VM) on
>multiple
>VMs.
>> When you say queue 'enable/disable' in the guest are you referring to using
>'ethtool -L <inface> combined <channel no>' ?
>> If this is the case I did do this by configuring 5 rxqs(DPDK and vhost User)
>and changed the channel nos and verified with testpmd app again for flushing
>scenarios. I didn't testing with kernel forwarding here.
>>
>> Regards,
>> Bhanuprakash.
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
