[ovs-dev] [PATCH v4 2/5] netdev-dpdk: Add netdev_dpdk_vhost_txq_flush function.

Bodireddy, Bhanuprakash bhanuprakash.bodireddy at intel.com
Wed Aug 9 12:46:45 UTC 2017


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




More information about the dev mailing list