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

2019-10-15 Thread Kevin Traynor 
On 15/10/2019 16:42, Ilya Maximets wrote:
> On 15.10.2019 17:32, Sriram Vatala wrote:
>> -Original Message-
>> From: Kevin Traynor 
>> Sent: 15 October 2019 19:21
>> To: Ilya Maximets ; Sriram Vatala
>> ; 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 
>>
>> 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 
>> Signed-off-by: Ilya Maximets 
>> Signed-off-by: Sriram Vatala 
>> ---
>>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(>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 
>>
>> @Ilya Maximets  with your approval, I can fix this with the suggested

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

2019-10-15 Thread Ilya Maximets 

On 15.10.2019 17:32, Sriram Vatala wrote:

-Original Message-
From: Kevin Traynor 
Sent: 15 October 2019 19:21
To: Ilya Maximets ; Sriram Vatala
; 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 

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 
Signed-off-by: Ilya Maximets 
Signed-off-by: Sriram Vatala 
---
   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(>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 

@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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2019-10-15 Thread Sriram Vatala via dev 
-Original Message-
From: Kevin Traynor 
Sent: 15 October 2019 19:21
To: Ilya Maximets ; Sriram Vatala 
; 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 

 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 
 Signed-off-by: Ilya Maximets 
 Signed-off-by: Sriram Vatala 
 ---
   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(>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 

@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;

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

2019-10-15 Thread Kevin Traynor 
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 

 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 
 Signed-off-by: Ilya Maximets 
 Signed-off-by: Sriram Vatala 
 ---
   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(>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 

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

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

2019-10-15 Thread Ilya Maximets 

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 

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 
Signed-off-by: Ilya Maximets 
Signed-off-by: Sriram Vatala 
---
  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(>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,

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

2019-10-15 Thread Kevin Traynor 
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 
>>
>> 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 
>> Signed-off-by: Ilya Maximets 
>> Signed-off-by: Sriram Vatala 
>> ---
>>  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(>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
>>  
>>

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

2019-10-15 Thread Kevin Traynor 
On 21/09/2019 03:40, Sriram Vatala wrote:
> From: Ilya Maximets 
> 
> 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 
> Signed-off-by: Ilya Maximets 
> Signed-off-by: Sriram Vatala 
> ---
>  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(>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(>mutex);
>  
>  rte_spinlock_lock(>stats_lock);
>  i = 0;
> -#define VHOST_CSTAT(NAME) \
> +#define SW_CSTAT(NAME) \
>  custom_stats->counters[i++].value = dev->NAME;
> -VHOST_CSTATS;
> -#undef