On 09.08.2017 15:29, Bodireddy, Bhanuprakash wrote:
> Hi Ilya,
>>>
>>> +/* Flush tx queues.
>>> + * This is done periodically to empty the intermediate queue in case
>>> +of
>>> + * fewer packets (< INTERIM_QUEUE_BURST_THRESHOLD) buffered in the
>> queue.
>>> + */
>>> +static int
>>> +netdev_dpdk_txq_flush(struct netdev *netdev, int qid , bool
>>> +concurrent_txq) {
>>> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>> +    struct dpdk_tx_queue *txq = &dev->tx_q[qid];
>>> +
>>> +    if (OVS_LIKELY(txq->dpdk_pkt_cnt)) {
>>> +        if (OVS_UNLIKELY(concurrent_txq)) {
>>> +            qid = qid % dev->up.n_txq;
>>> +            rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
>>> +        }
>>> +
>>> +        netdev_dpdk_eth_tx_burst(dev, qid, txq->dpdk_burst_pkts,
>>> +                                 txq->dpdk_pkt_cnt);
>>
>> The queue used for send and the locked one are different because you're
>> remapping the qid before taking the spinlock.
> 
>> I suspect that we're always using right queue numbers in current
>> implementation of dpif-netdev, but I need to recheck to be sure.
> 
> I believe the case you are referring here is the XPS case ('dynamic_txqs' 
> true).
> When we have to flush the packets we retrieve the qid from the 
> 'cached_tx_port->last_used_qid'
>  that was initialized earlier by 'dpif_netdev_xps_get_tx_qid()'. The logic of 
> remapping the qid and 
> acquiring the spin lock in the above function is no different from current 
> logic in master. Can you 
> elaborate the specific case where this would break the functionality?

Maybe my initial words are not fully correct, but below example shows what I 
tried to say.

1. dpif-netdev calls netdev_dpdk_txq_flush() with qid == 10;
2. txq = &dev->tx_q[10];  // Remember that 'txq' points to queue #10
3. if (txq->dpdk_pkt_cnt) ? true // Is there packets to send to queue #10?
4. qid = 10 % dev->up.n_txq; // Lets assume that dev->up.n_txq == 7
       ---> qid = 10 % 7 = 3
5. rte_spinlock_lock(&dev->tx_q[3].tx_lock); // Locking queue #3
6. netdev_dpdk_eth_tx_burst(dev, 3, txq->dpdk_burst_pkts, ..);
   --> sending to queue #3 packets enqueued for queue #10 ('txq' still points 
to queue #10)
   At this point queue #10 is not locked, so 'txq->dpdk_burst_pkts' is not 
protected
   from modifications, which could lead to wrong mempool refilling or driver 
crash.
   Also, you're trying to send not right packets to the queue.

   I mentioned that it looks like above scenario is impossible right now and
   qid will always be the same after truncating, but the logic is wrong anyway.

> 
> Please note that  in 'dpif_netdev_xps_get_tx_qid'  the qid can change and so 
> we did flush the queue.  
> 
> - Bhanuprakash. 
> 
>> Anyway, logic of this function completely broken.
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to