[dpdk-dev,v1,0/6] Extended xstats API in ethdev library to allow grouping of stats

Message ID 29874180.MbnUNaIQCm@xps (mailing list archive)
State Not Applicable, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply patch file failure

Commit Message

Thomas Monjalon May 1, 2017, 9:34 p.m. UTC
  27/04/2017 16:42, Michal Jastrzebski:
> From: Kuba Kozak <kubax.kozak@intel.com>
> 
> This patchset fixes following patches:
> 
>     commit ea85e7d711b6 ("ethdev: retrieve xstats by ID")
>     commit a954495245c4 ("ethdev: get xstats ID by name")
>     commit 1223608adb9b ("app/proc-info: support xstats by ID")
>     commit 25e38f09af9c ("net/e1000: support xstats by ID")
>     commit 923419333f5a ("net/ixgbe: support xstats by ID")
> 
> This patch addresses some API concerns with the xstat patchset
> applied in DPDK 17.05 RC2.
> 
> For clarity the first patch reverts all changes from the above
> commits.

It looks crazy to restart the feature from scratch, not showing the fixes.
But after a quick look, there was so many lines and functions to fix
that it is maybe a good idea.
At least, the history will be clear.

[...]
> Kuba Kozak (6):
>   ethdev: revert patches extending xstats API in ethdev
>   ethdev: retrieve xstats by ID
>   ethdev: get xstats ID by name
>   app/proc-info: support xstats by ID and by name
>   net/e1000: support xstats by ID
>   net/ixgbe: support xstats by ID

I am doing these small changes in your patches:
  

Comments

Thomas Monjalon May 1, 2017, 9:49 p.m. UTC | #1
01/05/2017 23:34, Thomas Monjalon:
> I am doing these small changes in your patches:

More explanations below

> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -395,7 +395,7 @@ static const struct eth_dev_ops eth_igb_ops = {
>         .link_update          = eth_igb_link_update,
>         .stats_get            = eth_igb_stats_get,
>         .xstats_get           = eth_igb_xstats_get,
> -       .xstats_get_by_id    = eth_igb_xstats_get_by_id,
> +       .xstats_get_by_id     = eth_igb_xstats_get_by_id,
>         .xstats_get_names_by_id = eth_igb_xstats_get_names_by_id,
>         .xstats_get_names     = eth_igb_xstats_get_names,
>         .stats_reset          = eth_igb_stats_reset,
> 
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -532,7 +532,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
>         .link_update          = ixgbe_dev_link_update,
>         .stats_get            = ixgbe_dev_stats_get,
>         .xstats_get           = ixgbe_dev_xstats_get,
> -       .xstats_get_by_id    = ixgbe_dev_xstats_get_by_id,
> +       .xstats_get_by_id     = ixgbe_dev_xstats_get_by_id,
>         .stats_reset          = ixgbe_dev_stats_reset,
>         .xstats_reset         = ixgbe_dev_xstats_reset,
>         .xstats_get_names     = ixgbe_dev_xstats_get_names,

It is just for correct alignment.

> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -188,7 +188,7 @@ rte_eth_dev_allocated(const char *name)
>  static uint8_t
>  rte_eth_dev_find_free_port(void)
>  {
> -       unsigned int i;
> +       unsigned i;
>  
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>                 if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)

It reverts a change you made in ixgbe patch.

> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -185,7 +185,6 @@ extern "C" {
>  #include "rte_ether.h"
>  #include "rte_eth_ctrl.h"
>  #include "rte_dev_info.h"
> -#include "rte_compat.h"
>  
>  struct rte_mbuf;
>  

This include is not needed in this new version of xstats.

I will remove also the changes to release notes (API section),
as the API is not modified. They are just additional functions.
  

Patch

--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -395,7 +395,7 @@  static const struct eth_dev_ops eth_igb_ops = {
        .link_update          = eth_igb_link_update,
        .stats_get            = eth_igb_stats_get,
        .xstats_get           = eth_igb_xstats_get,
-       .xstats_get_by_id    = eth_igb_xstats_get_by_id,
+       .xstats_get_by_id     = eth_igb_xstats_get_by_id,
        .xstats_get_names_by_id = eth_igb_xstats_get_names_by_id,
        .xstats_get_names     = eth_igb_xstats_get_names,
        .stats_reset          = eth_igb_stats_reset,

--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -532,7 +532,7 @@  static const struct eth_dev_ops ixgbe_eth_dev_ops = {
        .link_update          = ixgbe_dev_link_update,
        .stats_get            = ixgbe_dev_stats_get,
        .xstats_get           = ixgbe_dev_xstats_get,
-       .xstats_get_by_id    = ixgbe_dev_xstats_get_by_id,
+       .xstats_get_by_id     = ixgbe_dev_xstats_get_by_id,
        .stats_reset          = ixgbe_dev_stats_reset,
        .xstats_reset         = ixgbe_dev_xstats_reset,
        .xstats_get_names     = ixgbe_dev_xstats_get_names,

--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -188,7 +188,7 @@  rte_eth_dev_allocated(const char *name)
 static uint8_t
 rte_eth_dev_find_free_port(void)
 {
-       unsigned int i;
+       unsigned i;
 
        for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
                if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)

--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -185,7 +185,6 @@  extern "C" {
 #include "rte_ether.h"
 #include "rte_eth_ctrl.h"
 #include "rte_dev_info.h"
-#include "rte_compat.h"
 
 struct rte_mbuf;