[ovs-dev] [PATCH v11 2/3] netdev-dpdk : Detailed packet drop statistics

Kevin Traynor ktraynor at redhat.com
Mon Nov 11 16:06:29 UTC 2019


On 11/11/2019 15:59, Ilya Maximets wrote:
> On 11.11.2019 16:55, Kevin Traynor wrote:
>> On 10/11/2019 23:20, Ilya Maximets wrote:
>>> On 29.10.2019 15:50, Sriram Vatala wrote:> @@ -2388,12 +2412,16 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>>>>           }
>>>>       } while (cnt && (retries++ < max_retries));
>>>>   
>>>> +    tx_failure = cnt;
>>>>       rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
>>>>   
>>>>       rte_spinlock_lock(&dev->stats_lock);
>>>>       netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts,
>>>>                                            cnt + dropped);
>>>> -    dev->tx_retries += MIN(retries, max_retries);
>>>> +    sw_stats->tx_retries += MIN(retries, max_retries);
>>>> +    sw_stats->tx_failure_drops += tx_failure;
>>>> +    sw_stats->tx_mtu_exceeded_drops += mtu_drops;
>>>> +    sw_stats->tx_qos_drops += qos_drops;
>>>
>>> Kevin pointed to this part of code in hope that we can move this to
>>> a separate function and reuse in his review for v9.  This code catches
>>> my eyes too.  I don't think that we can reuse this part, at least this
>>> will be not very efficient in current situation (clearing of the unused
>>> fields in a stats structure will be probably needed before calling such
>>> a common function, but ETH tx uses only half of the struct).
>>>
>>> But there is another thing here.  We already have special functions
>>> for vhost tx/rx counters.  And it looks strange when we're not using
>>> these functions to update tx/rx failure counters.
>>>
>>> Suggesting following incremental.
>>> Kevin, Sriram, please, share your thoughts.
>>>
>>
>> The incremental patch looks good, thanks. One additional thing is that
>> OVS_REQUIRES(dev->stats_lock) annotations can be used for the vhost
>> rx/tx update counter functions now (even if it seems unlikely someone
>> would miss doing that).
>>
>> @Sriram, see below or you can check similar OVS_REQUIRES usage elsewhere
>> in the file.
> 
> I'm not sure if clang annotations will work with rte_spinlock.
> DPDK doesn't have proper annotations for locking functions.
> 

Ah, good point, I didn't check the lock type. In that case nevermind,
patch+incremental LGTM as is.

Acked-by: Kevin Traynor <ktraynor at redhat.com>



thanks,
Kevin.



More information about the dev mailing list