>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

Reply via email to