diff mbox series

[ovs-dev,v2,2/2] netdev-dpdk: Refactor vhost custom stats for extensibility.

Message ID 20190819111849.5824-3-i.maximets@samsung.com
State Accepted
Headers show
Series netdev-dpdk: Stats fix and refactoring. | expand

Commit Message

Ilya Maximets Aug. 19, 2019, 11:18 a.m. UTC 
vHost interfaces currently has only one custom statistic, but there
might be others in the near future. This refactoring makes the code
work in the same way as it done for dpdk and afxdp stats to keep the
common style over the different code places and makes it easily
extensible for the new stats addition.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/netdev-dpdk.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

David Marchand Aug. 20, 2019, 9:43 a.m. UTC | #1 
On Mon, Aug 19, 2019 at 1:19 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> vHost interfaces currently has only one custom statistic, but there
> might be others in the near future. This refactoring makes the code
> work in the same way as it done for dpdk and afxdp stats to keep the
> common style over the different code places and makes it easily
> extensible for the new stats addition.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  lib/netdev-dpdk.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 52ecf7576..bc20d6843 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -110,12 +110,6 @@ BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)
>  BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF))
>                    % MP_CACHE_SZ == 0);
>
> -/* Size of vHost custom stats. */
> -#define VHOST_CUSTOM_STATS_SIZE          1
> -
> -/* Names of vHost custom stats. */
> -#define VHOST_STAT_TX_RETRIES            "tx_retries"
> -
>  #define SOCKET0              0
>
>  /* Default size of Physical NIC RXQ */
> @@ -2827,17 +2821,31 @@ netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
>                                     struct netdev_custom_stats *custom_stats)
>  {
>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +    int i;
>
> -    ovs_mutex_lock(&dev->mutex);
> +#define VHOST_CSTATS \
> +    VHOST_CSTAT(tx_retries)
>
> -    custom_stats->size = VHOST_CUSTOM_STATS_SIZE;
> +#define VHOST_CSTAT(NAME) + 1
> +    custom_stats->size = VHOST_CSTATS;
> +#undef VHOST_CSTAT
>      custom_stats->counters = xcalloc(custom_stats->size,
>                                       sizeof *custom_stats->counters);
> -    ovs_strlcpy(custom_stats->counters[0].name, VHOST_STAT_TX_RETRIES,
> +    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);
> -    custom_stats->counters[0].value = dev->tx_retries;
> +    i = 0;
> +#define VHOST_CSTAT(NAME) \
> +    custom_stats->counters[i++].value = dev->NAME;
> +    VHOST_CSTATS;
> +#undef VHOST_CSTAT
>      rte_spinlock_unlock(&dev->stats_lock);
>
>      ovs_mutex_unlock(&dev->mutex);
> --
> 2.17.1
>

Reviewed-by: David Marchand <david.marchand@redhat.com>



--
David Marchand
Kevin Traynor Aug. 23, 2019, 1:16 p.m. UTC | #2 
On 19/08/2019 12:18, Ilya Maximets wrote:
> vHost interfaces currently has only one custom statistic, but there
> might be others in the near future. This refactoring makes the code
> work in the same way as it done for dpdk and afxdp stats to keep the
> common style over the different code places and makes it easily
> extensible for the new stats addition.
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  lib/netdev-dpdk.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 52ecf7576..bc20d6843 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -110,12 +110,6 @@ BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)
>  BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF))
>                    % MP_CACHE_SZ == 0);
>  
> -/* Size of vHost custom stats. */
> -#define VHOST_CUSTOM_STATS_SIZE          1
> -
> -/* Names of vHost custom stats. */
> -#define VHOST_STAT_TX_RETRIES            "tx_retries"
> -
>  #define SOCKET0              0
>  
>  /* Default size of Physical NIC RXQ */
> @@ -2827,17 +2821,31 @@ netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
>                                     struct netdev_custom_stats *custom_stats)
>  {
>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +    int i;
>  
> -    ovs_mutex_lock(&dev->mutex);
> +#define VHOST_CSTATS \
> +    VHOST_CSTAT(tx_retries)
>  
> -    custom_stats->size = VHOST_CUSTOM_STATS_SIZE;
> +#define VHOST_CSTAT(NAME) + 1
> +    custom_stats->size = VHOST_CSTATS;
> +#undef VHOST_CSTAT
>      custom_stats->counters = xcalloc(custom_stats->size,
>                                       sizeof *custom_stats->counters);
> -    ovs_strlcpy(custom_stats->counters[0].name, VHOST_STAT_TX_RETRIES,
> +    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);
> -    custom_stats->counters[0].value = dev->tx_retries;
> +    i = 0;
> +#define VHOST_CSTAT(NAME) \
> +    custom_stats->counters[i++].value = dev->NAME;

That would fit on one line, but maybe you want it to look consistent
with the previous #define.

Also, I wonder would having a blank line after the #define's make them
easier to distinguish?

Either way, Acked-by: Kevin Traynor <ktraynor@redhat.com>

> +    VHOST_CSTATS;
> +#undef VHOST_CSTAT
>      rte_spinlock_unlock(&dev->stats_lock);
>  
>      ovs_mutex_unlock(&dev->mutex);
>
Ilya Maximets Aug. 23, 2019, 2:05 p.m. UTC | #3 
On 23.08.2019 16:16, Kevin Traynor wrote:
> On 19/08/2019 12:18, Ilya Maximets wrote:
>> vHost interfaces currently has only one custom statistic, but there
>> might be others in the near future. This refactoring makes the code
>> work in the same way as it done for dpdk and afxdp stats to keep the
>> common style over the different code places and makes it easily
>> extensible for the new stats addition.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>  lib/netdev-dpdk.c | 28 ++++++++++++++++++----------
>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 52ecf7576..bc20d6843 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -110,12 +110,6 @@ BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)
>>  BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF))
>>                    % MP_CACHE_SZ == 0);
>>  
>> -/* Size of vHost custom stats. */
>> -#define VHOST_CUSTOM_STATS_SIZE          1
>> -
>> -/* Names of vHost custom stats. */
>> -#define VHOST_STAT_TX_RETRIES            "tx_retries"
>> -
>>  #define SOCKET0              0
>>  
>>  /* Default size of Physical NIC RXQ */
>> @@ -2827,17 +2821,31 @@ netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
>>                                     struct netdev_custom_stats *custom_stats)
>>  {
>>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> +    int i;
>>  
>> -    ovs_mutex_lock(&dev->mutex);
>> +#define VHOST_CSTATS \
>> +    VHOST_CSTAT(tx_retries)
>>  
>> -    custom_stats->size = VHOST_CUSTOM_STATS_SIZE;
>> +#define VHOST_CSTAT(NAME) + 1
>> +    custom_stats->size = VHOST_CSTATS;
>> +#undef VHOST_CSTAT
>>      custom_stats->counters = xcalloc(custom_stats->size,
>>                                       sizeof *custom_stats->counters);
>> -    ovs_strlcpy(custom_stats->counters[0].name, VHOST_STAT_TX_RETRIES,
>> +    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);
>> -    custom_stats->counters[0].value = dev->tx_retries;
>> +    i = 0;
>> +#define VHOST_CSTAT(NAME) \
>> +    custom_stats->counters[i++].value = dev->NAME;
> 
> That would fit on one line, but maybe you want it to look consistent
> with the previous #define.

I wanted the code to have same indentation level to look like the same code
flow for better understanding.

> 
> Also, I wonder would having a blank line after the #define's make them
> easier to distinguish?

#define's are tightly coupled with their usage by VHOST_CSTATS.
IMHO, separating them by blank lines makes the code harder to understand.
I'd prefer keeping them as a solid define-undef block.

> 
> Either way, Acked-by: Kevin Traynor <ktraynor@redhat.com>
> 
>> +    VHOST_CSTATS;
>> +#undef VHOST_CSTAT
>>      rte_spinlock_unlock(&dev->stats_lock);
>>  
>>      ovs_mutex_unlock(&dev->mutex);
>>
> 
> 
>
diff mbox series

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 52ecf7576..bc20d6843 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -110,12 +110,6 @@  BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)
 BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF))
                   % MP_CACHE_SZ == 0);
 
-/* Size of vHost custom stats. */
-#define VHOST_CUSTOM_STATS_SIZE          1
-
-/* Names of vHost custom stats. */
-#define VHOST_STAT_TX_RETRIES            "tx_retries"
-
 #define SOCKET0              0
 
 /* Default size of Physical NIC RXQ */
@@ -2827,17 +2821,31 @@  netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
                                    struct netdev_custom_stats *custom_stats)
 {
     struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+    int i;
 
-    ovs_mutex_lock(&dev->mutex);
+#define VHOST_CSTATS \
+    VHOST_CSTAT(tx_retries)
 
-    custom_stats->size = VHOST_CUSTOM_STATS_SIZE;
+#define VHOST_CSTAT(NAME) + 1
+    custom_stats->size = VHOST_CSTATS;
+#undef VHOST_CSTAT
     custom_stats->counters = xcalloc(custom_stats->size,
                                      sizeof *custom_stats->counters);
-    ovs_strlcpy(custom_stats->counters[0].name, VHOST_STAT_TX_RETRIES,
+    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);
-    custom_stats->counters[0].value = dev->tx_retries;
+    i = 0;
+#define VHOST_CSTAT(NAME) \
+    custom_stats->counters[i++].value = dev->NAME;
+    VHOST_CSTATS;
+#undef VHOST_CSTAT
     rte_spinlock_unlock(&dev->stats_lock);
 
     ovs_mutex_unlock(&dev->mutex);