>enable)
>>>> if (enable) {
>>>> dev->tx_q[qid].map = qid;
>>
>> Here flushing required too because we're possibly enabling previously
>remapped queue.
>>
>>>> } else {
>>>> + /* If the queue is disabled in the guest, the
>>>> corresponding qid
>>>> + * map shall be set to OVS_VHOST_QUEUE_DISABLED(-2).
>>>> + *
>>>> + * The packets that were queued in 'qid' could be
>>>> potentially
>>>> + * stuck and needs to be dropped.
>>>> + *
>>>> + * XXX: The queues may be already disabled in the guest so
>>>> + * flush function in this case only helps in updating
>>>> stats
>>>> + * and freeing memory.
>>>> + */
>>>> + netdev_dpdk_vhost_txq_flush(&dev->up, qid, 0);
>>>> dev->tx_q[qid].map = OVS_VHOST_QUEUE_DISABLED;
>>>> }
>>>> netdev_dpdk_remap_txqs(dev);
>>
>> 'netdev_dpdk_remap_txqs()', actually, is able to change mapping for
>> all the disabled in guest queues. So, we need to flush all of them
>> while remapping somewhere inside the function.
>> One other thing is that there is a race window between flush and
>> mapping update where another process able to enqueue more packets in
>> just flushed queue. The order of operations should be changed, or both
>> of them should be done under the same tx_lock. I think, it's required
>> to make tx_q[].map field atomic to fix the race condition, because
>> send function takes the 'map' and then locks the corresponding queue.
>> It wasn't an issue before, because packets in case of race was just
>> dropped on attempt to send to disabled queue, but with this patch
>> applied they will be enqueued to the intermediate queue and stuck there.
>
>Making 'map' atomic will not help. To solve the race we should make 'reading
>of map + enqueue' an atomic operation by some spinlock.
>Like this:
>
>vhost_send:
>----------------------------------------------------------------
> qid = qid % netdev->n_txq;
> rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
>
> mapped_qid = dev->tx_q[qid].map;
>
> if (qid != mapped_qid) {
> rte_spinlock_lock(&dev->tx_q[mapped_qid].tx_lock);
> }
>
> tx_enqueue(mapped_qid, pkts, cnt);
>
> if (qid != mapped_qid) {
> rte_spinlock_unlock(&dev->tx_q[mapped_qid].tx_lock);
> }
>
> rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
>----------------------------------------------------------------
>
>txq remapping inside 'netdev_dpdk_remap_txqs()' or
>'vring_state_changed()':
>----------------------------------------------------------------
> qid - queue we need to remap.
> new_qid - queue we need to remap to.
>
> rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
>
> mapped_qid = dev->tx_q[qid].map;
> if (qid != mapped_qid) {
> rte_spinlock_lock(&dev->tx_q[mapped_qid].tx_lock);
> }
>
> tx_flush(mapped_qid)
>
> if (qid != mapped_qid) {
> rte_spinlock_unlock(&dev->tx_q[mapped_qid].tx_lock);
> }
>
> dev->tx_q[qid].map = new_qid;
>
> rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
>----------------------------------------------------------------
>
>Above schema should work without races, but looks kind of ugly and requires
>taking of additional spinlock on each send.
>
>P.S. Sorry for talking with myself. Just want to share my thoughts.
Hi Ilya,
Thanks for reviewing the patches and providing inputs.
I went through your comments for this patch(2/5) and agree with the suggestions.
Meanwhile while go through the changes above and get back to you.
Bhanuprakash.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
