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

Ilya Maximets i.maximets at ovn.org
Mon Nov 11 16:11:25 UTC 2019


On 11.11.2019 17:06, Kevin Traynor wrote:
> 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. In this case, I think, there is no need to re-spin the series.
I'll just squash the incremental with this patch and give it another try.
If it'll work fine, I'll apply the series.

Best regards, Ilya Maximets.


More information about the dev mailing list