[ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.

Kevin Traynor ktraynor at redhat.com
Tue Oct 15 15:58:21 UTC 2019


On 15/10/2019 16:42, Ilya Maximets wrote:
> On 15.10.2019 17:32, Sriram Vatala wrote:
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor at redhat.com>
>> Sent: 15 October 2019 19:21
>> To: Ilya Maximets <i.maximets at ovn.org>; Sriram Vatala
>> <sriram.v at altencalsoftlabs.com>; ovs-dev at openvswitch.org
>> Subject: Re: [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH
>> custom stats.
>>
>> On 15/10/2019 14:43, Ilya Maximets wrote:
>>> On 15.10.2019 15:35, Kevin Traynor wrote:
>>>> With corrected email for Ilya.
>>>>
>>>> On 15/10/2019 14:33, Kevin Traynor wrote:
>>>>> On 21/09/2019 03:40, Sriram Vatala wrote:
>>>>>> From: Ilya Maximets <i.maximets at samsung.com>
>>>>>>
>>>>>> This is yet another refactoring for upcoming detailed drop stats.
>>>>>> It allowes to use single function for all the software calculated
>>>>>> statistics in netdev-dpdk for both vhost and ETH ports.
>>>>>>
>>>>>> UINT64_MAX used as a marker for non-supported statistics in a same
>>>>>> way as it's done in bridge.c for common netdev stats.
>>>>>>
>>>>>
>>>>> Hi Ilya, one comment below,
>>>>>
>>>>> thanks,
>>>>> Kevin.
>>>>>
>>>>>> Cc: Sriram Vatala <sriram.v at altencalsoftlabs.com>
>>>>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>>>>>> Signed-off-by: Sriram Vatala <sriram.v at altencalsoftlabs.com>
>>>>>> ---
>>>>>>    lib/netdev-dpdk.c | 67 +++++++++++++++++++++++++++--------------------
>>>>>>    1 file changed, 39 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>>>>> bc20d6843..652b57e3b 100644
>>>>>> --- a/lib/netdev-dpdk.c
>>>>>> +++ b/lib/netdev-dpdk.c
>>>>>> @@ -471,6 +471,8 @@ struct netdev_rxq_dpdk {
>>>>>>    static void netdev_dpdk_destruct(struct netdev *netdev);
>>>>>>    static void netdev_dpdk_vhost_destruct(struct netdev *netdev);
>>>>>>
>>>>>> +static int netdev_dpdk_get_sw_custom_stats(const struct netdev *,
>>>>>> +                                           struct
>>>>>> +netdev_custom_stats *);
>>>>>>    static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev);
>>>>>>
>>>>>>    int netdev_dpdk_get_vid(const struct netdev_dpdk *dev); @@
>>>>>> -1171,7 +1173,7 @@ common_construct(struct netdev *netdev, dpdk_port_t
>>>>>> port_no,
>>>>>>        dev->rte_xstats_ids = NULL;
>>>>>>        dev->rte_xstats_ids_size = 0;
>>>>>>
>>>>>> -    dev->tx_retries = 0;
>>>>>> +    dev->tx_retries = (dev->type == DPDK_DEV_VHOST) ? 0 :
>>>>>> + UINT64_MAX;
>>>>>>
>>>>>>        return 0;
>>>>>>    }
>>>>>> @@ -2771,7 +2773,9 @@ netdev_dpdk_get_custom_stats(const struct
>>>>>> netdev *netdev,
>>>>>>
>>>>>>        uint32_t i;
>>>>>>        struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>>>> -    int rte_xstats_ret;
>>>>>> +    int rte_xstats_ret, sw_stats_size;
>>>>>> +
>>>>>> +    netdev_dpdk_get_sw_custom_stats(netdev, custom_stats);
>>>>>>
>>>>>>        ovs_mutex_lock(&dev->mutex);
>>>>>>
>>>>>> @@ -2786,12 +2790,13 @@ netdev_dpdk_get_custom_stats(const struct netdev
>>>>>> *netdev,
>>>>>>            if (rte_xstats_ret > 0 &&
>>>>>>                rte_xstats_ret <= dev->rte_xstats_ids_size) {
>>>>>>
>>>>>> -            custom_stats->size = rte_xstats_ret;
>>>>>> -            custom_stats->counters =
>>>>>> -                    (struct netdev_custom_counter *)
>>>>>> xcalloc(rte_xstats_ret,
>>>>>> -                            sizeof(struct netdev_custom_counter));
>>>>>> +            sw_stats_size = custom_stats->size;
>>>>>> +            custom_stats->size += rte_xstats_ret;
>>>>>> +            custom_stats->counters = xrealloc(custom_stats->counters,
>>>>>> +                                              custom_stats->size *
>>>>>> +                                              sizeof
>>>>>> + *custom_stats->counters);
>>>>>>
>>>>>> -            for (i = 0; i < rte_xstats_ret; i++) {
>>>>>> +            for (i = sw_stats_size; i < sw_stats_size +
>>>>>> + rte_xstats_ret; i++) {
>>>>>>                    ovs_strlcpy(custom_stats->counters[i].name,
>>>>>>                                netdev_dpdk_get_xstat_name(dev,
>>>>>>
>>>>>> dev->rte_xstats_ids[i]),
>>>>>
>>>>> I think you need to add another array index counter for
>>>>> ret_xstats_ids[] and values[] as they are still using i, but i is
>>>>> now starting with sw_stats_size and not 0 anymore.
>>>
>>> Good catch.
>>> I didn't actually test this code hoping that it'll be tested along
>>> with the second patch.
>>> Sorry I missed this.  I checked vhost port stats and missed to check dpdk
>>> port stats in my testing. It's my mistake.
>>> For this part we could just move the 'sw_stats_size' from the loop
>>> counter to the counters[i]. Like this:
>>>
>>> for (i = 0; i < rte_xstats_ret; i++) {
>>>       ovs_strlcpy(custom_stats->counters[sw_stats_size + i].name,
>>>                   netdev_dpdk_get_xstat_name(dev,
>>>                                              dev->rte_xstats_ids[i]),
>>>                   NETDEV_CUSTOM_STATS_NAME_SIZE);
>>>       custom_stats->counters[sw_stats_size + i].value = values[i]; }
>>>
>>
>> Yep, that would be good. With that fix, you can add
>> Acked-by: Kevin Traynor <ktraynor at redhat.com>
>>
>> @Ilya Maximets  with your approval, I can fix this with the suggested approach
>> and send the updated patch. What do you say?
> 
> It's OK. But, I think, that it might be better to wait for review comments
> for patch #2 first.
> 

I'll review patch #2 and send comments tomorrow. (btw, it doesn't apply
cleanly on master)

> Best regards, Ilya Maximets.
> 



More information about the dev mailing list