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

Sriram Vatala sriram.v at altencalsoftlabs.com
Tue Oct 15 15:32:46 UTC 2019


-----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?
>
>>>
>>>> @@ -2801,8 +2806,6 @@ netdev_dpdk_get_custom_stats(const struct netdev 
>>>> *netdev,
>>>>           } else {
>>>>               VLOG_WARN("Cannot get XSTATS values for port: 
>>>> "DPDK_PORT_ID_FMT,
>>>>                         dev->port_id);
>>>> -            custom_stats->counters = NULL;
>>>> -            custom_stats->size = 0;
>>>>               /* Let's clear statistics cache, so it will be
>>>>                * reconfigured */
>>>>               netdev_dpdk_clear_xstats(dev); @@ -2817,39 +2820,47
>>>> @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
>>>>   }
>>>>
>>>>   static int
>>>> -netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
>>>> -                                   struct netdev_custom_stats 
>>>> *custom_stats)
>>>> +netdev_dpdk_get_sw_custom_stats(const struct netdev *netdev,
>>>> +                                struct netdev_custom_stats
>>>> +*custom_stats)
>>>>   {
>>>>       struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>> -    int i;
>>>> +    int i, n;
>>>>
>>>> -#define VHOST_CSTATS \
>>>> -    VHOST_CSTAT(tx_retries)
>>>> +#define SW_CSTATS \
>>>> +    SW_CSTAT(tx_retries)
>>>>
>>>> -#define VHOST_CSTAT(NAME) + 1
>>>> -    custom_stats->size = VHOST_CSTATS;
>>>> -#undef VHOST_CSTAT
>>>> +#define SW_CSTAT(NAME) + 1
>>>> +    custom_stats->size = SW_CSTATS; #undef SW_CSTAT
>>>>       custom_stats->counters = xcalloc(custom_stats->size,
>>>>                                        sizeof *custom_stats->counters);
>>>> -    i = 0;
>>>> -#define VHOST_CSTAT(NAME) \
>>>> -    ovs_strlcpy(custom_stats->counters[i++].name, #NAME, \
>>>> -                NETDEV_CUSTOM_STATS_NAME_SIZE);
>>>> -    VHOST_CSTATS;
>>>> -#undef VHOST_CSTAT
>>>>
>>>>       ovs_mutex_lock(&dev->mutex);
>>>>
>>>>       rte_spinlock_lock(&dev->stats_lock);
>>>>       i = 0;
>>>> -#define VHOST_CSTAT(NAME) \
>>>> +#define SW_CSTAT(NAME) \
>>>>       custom_stats->counters[i++].value = dev->NAME;
>>>> -    VHOST_CSTATS;
>>>> -#undef VHOST_CSTAT
>>>> +    SW_CSTATS;
>>>> +#undef SW_CSTAT
>>>>       rte_spinlock_unlock(&dev->stats_lock);
>>>>
>>>>       ovs_mutex_unlock(&dev->mutex);
>>>>
>>>> +    i = 0;
>>>> +    n = 0;
>>>> +#define SW_CSTAT(NAME) \
>>>> +    if (custom_stats->counters[i].value != UINT64_MAX) 
>>>> {                   \
>>>> +        ovs_strlcpy(custom_stats->counters[n].name, #NAME, 
>>>> \
>>>> +                    NETDEV_CUSTOM_STATS_NAME_SIZE); 
>>>> \
>>>> +        custom_stats->counters[n].value = 
>>>> custom_stats->counters[i].value; \
>>>> +        n++; 
>>>> \
>>>> +    } 
>>>> \
>>>> +    i++;
>>>> +    SW_CSTATS;
>>>> +#undef SW_CSTAT
>>>> +
>>>> +    custom_stats->size = n;
>>>>       return 0;
>>>>   }
>>>>
>>>> @@ -4433,7 +4444,7 @@ static const struct netdev_class dpdk_vhost_class = 
>>>> {
>>>>       .send = netdev_dpdk_vhost_send,
>>>>       .get_carrier = netdev_dpdk_vhost_get_carrier,
>>>>       .get_stats = netdev_dpdk_vhost_get_stats,
>>>> -    .get_custom_stats = netdev_dpdk_vhost_get_custom_stats,
>>>> +    .get_custom_stats = netdev_dpdk_get_sw_custom_stats,
>>>>       .get_status = netdev_dpdk_vhost_user_get_status,
>>>>       .reconfigure = netdev_dpdk_vhost_reconfigure,
>>>>       .rxq_recv = netdev_dpdk_vhost_rxq_recv, @@ -4449,7 +4460,7 @@
>>>> static const struct netdev_class dpdk_vhost_client_class = {
>>>>       .send = netdev_dpdk_vhost_send,
>>>>       .get_carrier = netdev_dpdk_vhost_get_carrier,
>>>>       .get_stats = netdev_dpdk_vhost_get_stats,
>>>> -    .get_custom_stats = netdev_dpdk_vhost_get_custom_stats,
>>>> +    .get_custom_stats = netdev_dpdk_get_sw_custom_stats,
>>>>       .get_status = netdev_dpdk_vhost_user_get_status,
>>>>       .reconfigure = netdev_dpdk_vhost_client_reconfigure,
>>>>       .rxq_recv = netdev_dpdk_vhost_rxq_recv,
>>>>
>>>
>>


More information about the dev mailing list