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

Message ID 20190921024008.11355-1-sriram.v@altencalsoftlabs.com
State Changes Requested
Delegated to: Ilya Maximets
Headers show
Series
  • [ovs-dev,v9,1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
Related show

Commit Message

Anil Kumar Koli via dev Sept. 21, 2019, 2:40 a.m. UTC
From: Ilya Maximets <i.maximets@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.

Cc: Sriram Vatala <sriram.v@altencalsoftlabs.com>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Signed-off-by: Sriram Vatala <sriram.v@altencalsoftlabs.com>
---
 lib/netdev-dpdk.c | 67 +++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

Comments

Kevin Traynor Oct. 15, 2019, 1:33 p.m. UTC | #1
On 21/09/2019 03:40, Sriram Vatala wrote:
> From: Ilya Maximets <i.maximets@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@altencalsoftlabs.com>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> Signed-off-by: Sriram Vatala <sriram.v@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.

> @@ -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,
>
Kevin Traynor Oct. 15, 2019, 1:35 p.m. UTC | #2
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@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@altencalsoftlabs.com>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> Signed-off-by: Sriram Vatala <sriram.v@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.
> 
>> @@ -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,
>>
>
Ilya Maximets Oct. 15, 2019, 1:43 p.m. UTC | #3
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@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@altencalsoftlabs.com>
>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>> Signed-off-by: Sriram Vatala <sriram.v@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.

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];
}


>>
>>> @@ -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,
>>>
>>
>
Kevin Traynor Oct. 15, 2019, 1:51 p.m. UTC | #4
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@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@altencalsoftlabs.com>
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> Signed-off-by: Sriram Vatala <sriram.v@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.
> 
> 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@redhat.com>

> 
>>>
>>>> @@ -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,
>>>>
>>>
>>
Anil Kumar Koli via dev Oct. 15, 2019, 3:32 p.m. UTC | #5
-----Original Message-----
From: Kevin Traynor <ktraynor@redhat.com>
Sent: 15 October 2019 19:21
To: Ilya Maximets <i.maximets@ovn.org>; Sriram Vatala 
<sriram.v@altencalsoftlabs.com>; ovs-dev@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@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@altencalsoftlabs.com>
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> Signed-off-by: Sriram Vatala <sriram.v@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@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,
>>>>
>>>
>>
Ilya Maximets Oct. 15, 2019, 3:42 p.m. UTC | #6
On 15.10.2019 17:32, Sriram Vatala wrote:
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: 15 October 2019 19:21
> To: Ilya Maximets <i.maximets@ovn.org>; Sriram Vatala
> <sriram.v@altencalsoftlabs.com>; ovs-dev@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@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@altencalsoftlabs.com>
>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>> Signed-off-by: Sriram Vatala <sriram.v@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@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.

Best regards, Ilya Maximets.
Kevin Traynor Oct. 15, 2019, 3:58 p.m. UTC | #7
On 15/10/2019 16:42, Ilya Maximets wrote:
> On 15.10.2019 17:32, Sriram Vatala wrote:
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: 15 October 2019 19:21
>> To: Ilya Maximets <i.maximets@ovn.org>; Sriram Vatala
>> <sriram.v@altencalsoftlabs.com>; ovs-dev@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@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@altencalsoftlabs.com>
>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>> Signed-off-by: Sriram Vatala <sriram.v@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@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.
>

Patch
diff mbox series

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]),
@@ -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,