[PATCH v3 net-next 0/2] net: mvneta: add support for page_pool_get_stats

[PATCH v3 net-next 0/2] net: mvneta: add support for page_pool_get_stats

[Lorenzo Bianconi]

Introduce page_pool stats ethtool APIs in order to avoid driver duplicated
code.

Changes since v2:
- remove enum list of page_pool stats in page_pool.h
- remove leftover change in mvneta.c for ethtool_stats array allocation

Changes since v1:
- move stats accounting to page_pool code
- move stats string management to page_pool code

Lorenzo Bianconi (2):
  net: page_pool: introduce ethtool stats
  net: mvneta: add support for page_pool_get_stats

 drivers/net/ethernet/marvell/Kconfig  |  1 +
 drivers/net/ethernet/marvell/mvneta.c | 20 ++++++++-
 include/net/page_pool.h               |  4 ++
 net/core/page_pool.c                  | 64 ++++++++++++++++++++++++++-
 4 files changed, 87 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Lorenzo Bianconi]

Introduce page_pool APIs to report stats through ethtool and reduce
duplicated code in each driver.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/net/page_pool.h |  4 +++
 net/core/page_pool.c    | 64 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index ea5fb70e5101..94b2d666db03 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -117,6 +117,10 @@ struct page_pool_stats {
 	struct page_pool_recycle_stats recycle_stats;
 };
 
+int page_pool_ethtool_stats_get_count(void);
+u8 *page_pool_ethtool_stats_get_strings(u8 *data);
+u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats);
+
 /*
  * Drivers that wish to harvest page pool stats and report them to users
  * (perhaps via ethtool, debugfs, or another mechanism) can allocate a
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 4af55d28ffa3..e82b94040ed1 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -18,6 +18,7 @@
 #include <linux/page-flags.h>
 #include <linux/mm.h> /* for __put_page() */
 #include <linux/poison.h>
+#include <linux/ethtool.h>
 
 #include <trace/events/page_pool.h>
 
@@ -42,6 +43,20 @@
 		this_cpu_add(s->__stat, val);						\
 	} while (0)
 
+static const char pp_stats[][ETH_GSTRING_LEN] = {
+	"rx_pp_alloc_fast",
+	"rx_pp_alloc_slow",
+	"rx_pp_alloc_slow_ho",
+	"rx_pp_alloc_empty",
+	"rx_pp_alloc_refill",
+	"rx_pp_alloc_waive",
+	"rx_pp_recycle_cached",
+	"rx_pp_recycle_cache_full",
+	"rx_pp_recycle_ring",
+	"rx_pp_recycle_ring_full",
+	"rx_pp_recycle_released_ref",
+};
+
 bool page_pool_get_stats(struct page_pool *pool,
 			 struct page_pool_stats *stats)
 {
@@ -50,7 +65,13 @@ bool page_pool_get_stats(struct page_pool *pool,
 	if (!stats)
 		return false;
 
-	memcpy(&stats->alloc_stats, &pool->alloc_stats, sizeof(pool->alloc_stats));
+	/* The caller is responsible to initialize stats. */
+	stats->alloc_stats.fast += pool->alloc_stats.fast;
+	stats->alloc_stats.slow += pool->alloc_stats.slow;
+	stats->alloc_stats.slow_high_order += pool->alloc_stats.slow_high_order;
+	stats->alloc_stats.empty += pool->alloc_stats.empty;
+	stats->alloc_stats.refill += pool->alloc_stats.refill;
+	stats->alloc_stats.waive += pool->alloc_stats.waive;
 
 	for_each_possible_cpu(cpu) {
 		const struct page_pool_recycle_stats *pcpu =
@@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
 	return true;
 }
 EXPORT_SYMBOL(page_pool_get_stats);
+
+u8 *page_pool_ethtool_stats_get_strings(u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
+		memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
+		data += ETH_GSTRING_LEN;
+	}
+
+	return data;
+}
+EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings);
+
+int page_pool_ethtool_stats_get_count(void)
+{
+	return ARRAY_SIZE(pp_stats);
+}
+EXPORT_SYMBOL(page_pool_ethtool_stats_get_count);
+
+u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
+		*data++ = stats->alloc_stats.fast;
+		*data++ = stats->alloc_stats.slow;
+		*data++ = stats->alloc_stats.slow_high_order;
+		*data++ = stats->alloc_stats.empty;
+		*data++ = stats->alloc_stats.refill;
+		*data++ = stats->alloc_stats.waive;
+		*data++ = stats->recycle_stats.cached;
+		*data++ = stats->recycle_stats.cache_full;
+		*data++ = stats->recycle_stats.ring;
+		*data++ = stats->recycle_stats.ring_full;
+		*data++ = stats->recycle_stats.released_refcnt;
+	}
+
+	return data;
+}
+EXPORT_SYMBOL(page_pool_ethtool_stats_get);
 #else
 #define alloc_stat_inc(pool, __stat)
 #define recycle_stat_inc(pool, __stat)
-- 
2.35.1

[PATCH v3 net-next 2/2] net: mvneta: add support for page_pool_get_stats

[Lorenzo Bianconi]

Introduce support for the page_pool_get_stats API to mvneta driver
Report page_pool stats through ethtool.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/Kconfig  |  1 +
 drivers/net/ethernet/marvell/mvneta.c | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index fe0989c0fc25..1240cb2dc07f 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -62,6 +62,7 @@ config MVNETA
 	select MVMDIO
 	select PHYLINK
 	select PAGE_POOL
+	select PAGE_POOL_STATS
 	help
 	  This driver supports the network interface units in the
 	  Marvell ARMADA XP, ARMADA 370, ARMADA 38x and
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 934f6dd90992..f6a54c7f0c69 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4735,6 +4735,9 @@ static void mvneta_ethtool_get_strings(struct net_device *netdev, u32 sset,
 		for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
 			memcpy(data + i * ETH_GSTRING_LEN,
 			       mvneta_statistics[i].name, ETH_GSTRING_LEN);
+
+		data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
+		page_pool_ethtool_stats_get_strings(data);
 	}
 }
 
@@ -4847,6 +4850,17 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp)
 	}
 }
 
+static void mvneta_ethtool_pp_stats(struct mvneta_port *pp, u64 *data)
+{
+	struct page_pool_stats stats = {};
+	int i;
+
+	for (i = 0; i < rxq_number; i++)
+		page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
+
+	page_pool_ethtool_stats_get(data, &stats);
+}
+
 static void mvneta_ethtool_get_stats(struct net_device *dev,
 				     struct ethtool_stats *stats, u64 *data)
 {
@@ -4857,12 +4871,16 @@ static void mvneta_ethtool_get_stats(struct net_device *dev,
 
 	for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
 		*data++ = pp->ethtool_stats[i];
+
+	mvneta_ethtool_pp_stats(pp, data);
 }
 
 static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
 {
 	if (sset == ETH_SS_STATS)
-		return ARRAY_SIZE(mvneta_statistics);
+		return ARRAY_SIZE(mvneta_statistics) +
+		       page_pool_ethtool_stats_get_count();
+
 	return -EOPNOTSUPP;
 }
 
-- 
2.35.1

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Andrew Lunn]

> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index ea5fb70e5101..94b2d666db03 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -117,6 +117,10 @@ struct page_pool_stats {
>  	struct page_pool_recycle_stats recycle_stats;
>  };
>  
> +int page_pool_ethtool_stats_get_count(void);
> +u8 *page_pool_ethtool_stats_get_strings(u8 *data);
> +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats);
> +
>  /*
>   * Drivers that wish to harvest page pool stats and report them to users
>   * (perhaps via ethtool, debugfs, or another mechanism) can allocate a

You could also add stub function here for when the page pool
statistics are disabled. We can then avoid all the messy #ifdef in the
drivers.

> +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> +		*data++ = stats->alloc_stats.fast;
> +		*data++ = stats->alloc_stats.slow;
> +		*data++ = stats->alloc_stats.slow_high_order;
> +		*data++ = stats->alloc_stats.empty;
> +		*data++ = stats->alloc_stats.refill;
> +		*data++ = stats->alloc_stats.waive;
> +		*data++ = stats->recycle_stats.cached;
> +		*data++ = stats->recycle_stats.cache_full;
> +		*data++ = stats->recycle_stats.ring;
> +		*data++ = stats->recycle_stats.ring_full;
> +		*data++ = stats->recycle_stats.released_refcnt;
> +	}
> +
> +	return data;

What is the purpose of the loop?

     Andrew

Re: [PATCH v3 net-next 2/2] net: mvneta: add support for page_pool_get_stats

[Andrew Lunn]

On Sat, Apr 09, 2022 at 08:39:11PM +0200, Lorenzo Bianconi wrote:
> Introduce support for the page_pool_get_stats API to mvneta driver
> Report page_pool stats through ethtool.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Ilias Apalodimas]

Hi Lorenzo,

[...]

>
>         for_each_possible_cpu(cpu) {
>                 const struct page_pool_recycle_stats *pcpu =
> @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
>         return true;
>  }
>  EXPORT_SYMBOL(page_pool_get_stats);
> +
> +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> +               data += ETH_GSTRING_LEN;
> +       }
> +
> +       return data;

Is there a point returning data here or can we make this a void?

> +}
> +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings);
> +
> +int page_pool_ethtool_stats_get_count(void)
> +{
> +       return ARRAY_SIZE(pp_stats);
> +}
> +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count);
> +
> +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> +               *data++ = stats->alloc_stats.fast;
> +               *data++ = stats->alloc_stats.slow;
> +               *data++ = stats->alloc_stats.slow_high_order;
> +               *data++ = stats->alloc_stats.empty;
> +               *data++ = stats->alloc_stats.refill;
> +               *data++ = stats->alloc_stats.waive;
> +               *data++ = stats->recycle_stats.cached;
> +               *data++ = stats->recycle_stats.cache_full;
> +               *data++ = stats->recycle_stats.ring;
> +               *data++ = stats->recycle_stats.ring_full;
> +               *data++ = stats->recycle_stats.released_refcnt;
> +       }
> +
> +       return data;

Ditto

> +}
> +EXPORT_SYMBOL(page_pool_ethtool_stats_get);
>  #else
>  #define alloc_stat_inc(pool, __stat)
>  #define recycle_stat_inc(pool, __stat)
> --
> 2.35.1
>

Thanks
/Ilias

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index ea5fb70e5101..94b2d666db03 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -117,6 +117,10 @@ struct page_pool_stats {
> >  	struct page_pool_recycle_stats recycle_stats;
> >  };
> >  
> > +int page_pool_ethtool_stats_get_count(void);
> > +u8 *page_pool_ethtool_stats_get_strings(u8 *data);
> > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats);
> > +
> >  /*
> >   * Drivers that wish to harvest page pool stats and report them to users
> >   * (perhaps via ethtool, debugfs, or another mechanism) can allocate a
> 
> You could also add stub function here for when the page pool
> statistics are disabled. We can then avoid all the messy #ifdef in the
> drivers.
> 
> > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > +		*data++ = stats->alloc_stats.fast;
> > +		*data++ = stats->alloc_stats.slow;
> > +		*data++ = stats->alloc_stats.slow_high_order;
> > +		*data++ = stats->alloc_stats.empty;
> > +		*data++ = stats->alloc_stats.refill;
> > +		*data++ = stats->alloc_stats.waive;
> > +		*data++ = stats->recycle_stats.cached;
> > +		*data++ = stats->recycle_stats.cache_full;
> > +		*data++ = stats->recycle_stats.ring;
> > +		*data++ = stats->recycle_stats.ring_full;
> > +		*data++ = stats->recycle_stats.released_refcnt;
> > +	}
> > +
> > +	return data;
> 
> What is the purpose of the loop?

ops sorry, you are right. I will fix it.

Regards,
Lorenzo

> 
>      Andrew

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

> Hi Lorenzo,

Hi Ilias,

> 
> [...]
> 
> >
> >         for_each_possible_cpu(cpu) {
> >                 const struct page_pool_recycle_stats *pcpu =
> > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
> >         return true;
> >  }
> >  EXPORT_SYMBOL(page_pool_get_stats);
> > +
> > +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> > +               data += ETH_GSTRING_LEN;
> > +       }
> > +
> > +       return data;
> 
> Is there a point returning data here or can we make this a void?

it is to add the capability to add more strings in the driver code after
running page_pool_ethtool_stats_get_strings.

> 
> > +}
> > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings);
> > +
> > +int page_pool_ethtool_stats_get_count(void)
> > +{
> > +       return ARRAY_SIZE(pp_stats);
> > +}
> > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count);
> > +
> > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > +               *data++ = stats->alloc_stats.fast;
> > +               *data++ = stats->alloc_stats.slow;
> > +               *data++ = stats->alloc_stats.slow_high_order;
> > +               *data++ = stats->alloc_stats.empty;
> > +               *data++ = stats->alloc_stats.refill;
> > +               *data++ = stats->alloc_stats.waive;
> > +               *data++ = stats->recycle_stats.cached;
> > +               *data++ = stats->recycle_stats.cache_full;
> > +               *data++ = stats->recycle_stats.ring;
> > +               *data++ = stats->recycle_stats.ring_full;
> > +               *data++ = stats->recycle_stats.released_refcnt;
> > +       }
> > +
> > +       return data;
> 
> Ditto

same here.

Regards,
Lorenzo

> 
> > +}
> > +EXPORT_SYMBOL(page_pool_ethtool_stats_get);
> >  #else
> >  #define alloc_stat_inc(pool, __stat)
> >  #define recycle_stat_inc(pool, __stat)
> > --
> > 2.35.1
> >
> 
> Thanks
> /Ilias

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Ilias Apalodimas]

On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > Hi Lorenzo,
>
> Hi Ilias,
>
> >
> > [...]
> >
> > >
> > >         for_each_possible_cpu(cpu) {
> > >                 const struct page_pool_recycle_stats *pcpu =
> > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
> > >         return true;
> > >  }
> > >  EXPORT_SYMBOL(page_pool_get_stats);
> > > +
> > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> > > +               data += ETH_GSTRING_LEN;
> > > +       }
> > > +
> > > +       return data;
> >
> > Is there a point returning data here or can we make this a void?
>
> it is to add the capability to add more strings in the driver code after
> running page_pool_ethtool_stats_get_strings.

But the current driver isn't using it.  I don't have too much
experience with how drivers consume ethtool stats, but would it make
more sense to return a length instead of a pointer? Maybe Andrew has
an idea.

Thanks
/Ilias
>
> >
> > > +}
> > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings);
> > > +
> > > +int page_pool_ethtool_stats_get_count(void)
> > > +{
> > > +       return ARRAY_SIZE(pp_stats);
> > > +}
> > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count);
> > > +
> > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > +               *data++ = stats->alloc_stats.fast;
> > > +               *data++ = stats->alloc_stats.slow;
> > > +               *data++ = stats->alloc_stats.slow_high_order;
> > > +               *data++ = stats->alloc_stats.empty;
> > > +               *data++ = stats->alloc_stats.refill;
> > > +               *data++ = stats->alloc_stats.waive;
> > > +               *data++ = stats->recycle_stats.cached;
> > > +               *data++ = stats->recycle_stats.cache_full;
> > > +               *data++ = stats->recycle_stats.ring;
> > > +               *data++ = stats->recycle_stats.ring_full;
> > > +               *data++ = stats->recycle_stats.released_refcnt;
> > > +       }
> > > +
> > > +       return data;
> >
> > Ditto
>
> same here.
>
> Regards,
> Lorenzo
>
> >
> > > +}
> > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get);
> > >  #else
> > >  #define alloc_stat_inc(pool, __stat)
> > >  #define recycle_stat_inc(pool, __stat)
> > > --
> > > 2.35.1
> > >
> >
> > Thanks
> > /Ilias

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

On Apr 11, Ilias Apalodimas wrote:
> On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > > Hi Lorenzo,
> >
> > Hi Ilias,
> >
> > >
> > > [...]
> > >
> > > >
> > > >         for_each_possible_cpu(cpu) {
> > > >                 const struct page_pool_recycle_stats *pcpu =
> > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
> > > >         return true;
> > > >  }
> > > >  EXPORT_SYMBOL(page_pool_get_stats);
> > > > +
> > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > > +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> > > > +               data += ETH_GSTRING_LEN;
> > > > +       }
> > > > +
> > > > +       return data;
> > >
> > > Is there a point returning data here or can we make this a void?
> >
> > it is to add the capability to add more strings in the driver code after
> > running page_pool_ethtool_stats_get_strings.
> 
> But the current driver isn't using it.  I don't have too much
> experience with how drivers consume ethtool stats, but would it make
> more sense to return a length instead of a pointer? Maybe Andrew has
> an idea.

yes, but it is for future usage. Returning a pointer modified by a
local routine is a common approach in the kernel (not sure about ethtool)
and it seems straightforward to me but len is fine as well. Opinions?

Regards,
Lorenzo

> 
> Thanks
> /Ilias
> >
> > >
> > > > +}
> > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_strings);
> > > > +
> > > > +int page_pool_ethtool_stats_get_count(void)
> > > > +{
> > > > +       return ARRAY_SIZE(pp_stats);
> > > > +}
> > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get_count);
> > > > +
> > > > +u64 *page_pool_ethtool_stats_get(u64 *data, struct page_pool_stats *stats)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > > +               *data++ = stats->alloc_stats.fast;
> > > > +               *data++ = stats->alloc_stats.slow;
> > > > +               *data++ = stats->alloc_stats.slow_high_order;
> > > > +               *data++ = stats->alloc_stats.empty;
> > > > +               *data++ = stats->alloc_stats.refill;
> > > > +               *data++ = stats->alloc_stats.waive;
> > > > +               *data++ = stats->recycle_stats.cached;
> > > > +               *data++ = stats->recycle_stats.cache_full;
> > > > +               *data++ = stats->recycle_stats.ring;
> > > > +               *data++ = stats->recycle_stats.ring_full;
> > > > +               *data++ = stats->recycle_stats.released_refcnt;
> > > > +       }
> > > > +
> > > > +       return data;
> > >
> > > Ditto
> >
> > same here.
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > > +}
> > > > +EXPORT_SYMBOL(page_pool_ethtool_stats_get);
> > > >  #else
> > > >  #define alloc_stat_inc(pool, __stat)
> > > >  #define recycle_stat_inc(pool, __stat)
> > > > --
> > > > 2.35.1
> > > >
> > >
> > > Thanks
> > > /Ilias

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Andrew Lunn]

On Mon, Apr 11, 2022 at 03:34:21PM +0300, Ilias Apalodimas wrote:
> On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > > Hi Lorenzo,
> >
> > Hi Ilias,
> >
> > >
> > > [...]
> > >
> > > >
> > > >         for_each_possible_cpu(cpu) {
> > > >                 const struct page_pool_recycle_stats *pcpu =
> > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
> > > >         return true;
> > > >  }
> > > >  EXPORT_SYMBOL(page_pool_get_stats);
> > > > +
> > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > > +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> > > > +               data += ETH_GSTRING_LEN;
> > > > +       }
> > > > +
> > > > +       return data;
> > >
> > > Is there a point returning data here or can we make this a void?
> >
> > it is to add the capability to add more strings in the driver code after
> > running page_pool_ethtool_stats_get_strings.
> 
> But the current driver isn't using it.

It could be you need it for the mlx5 driver, which puts the TLS
counters after the page pool counters. Or you could just move them to
the end. I don't think the order of statistics are ABI, just the
strings themselves..

> I don't have too much
> experience with how drivers consume ethtool stats, but would it make
> more sense to return a length instead of a pointer? Maybe Andrew has
> an idea.

Either is acceptable. Even if you do make it a void, the driver can
use the stats_get_count() and do the maths. But a length or a pointer
is simpler.

   Andrew

Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

[Ilias Apalodimas]

Hi Andrew,

On Mon, 11 Apr 2022 at 15:49, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Apr 11, 2022 at 03:34:21PM +0300, Ilias Apalodimas wrote:
> > On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > >
> > > > Hi Lorenzo,
> > >
> > > Hi Ilias,
> > >
> > > >
> > > > [...]
> > > >
> > > > >
> > > > >         for_each_possible_cpu(cpu) {
> > > > >                 const struct page_pool_recycle_stats *pcpu =
> > > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
> > > > >         return true;
> > > > >  }
> > > > >  EXPORT_SYMBOL(page_pool_get_stats);
> > > > > +
> > > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> > > > > +{
> > > > > +       int i;
> > > > > +
> > > > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > > > +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> > > > > +               data += ETH_GSTRING_LEN;
> > > > > +       }
> > > > > +
> > > > > +       return data;
> > > >
> > > > Is there a point returning data here or can we make this a void?
> > >
> > > it is to add the capability to add more strings in the driver code after
> > > running page_pool_ethtool_stats_get_strings.
> >
> > But the current driver isn't using it.
>
> It could be you need it for the mlx5 driver, which puts the TLS
> counters after the page pool counters. Or you could just move them to
> the end. I don't think the order of statistics are ABI, just the
> strings themselves..
>
> > I don't have too much
> > experience with how drivers consume ethtool stats, but would it make
> > more sense to return a length instead of a pointer? Maybe Andrew has
> > an idea.
>
> Either is acceptable. Even if you do make it a void, the driver can
> use the stats_get_count() and do the maths. But a length or a pointer
> is simpler.

Thanks, I am fine with either as well.   I was just wondering why we
need it since the mvneta driver wasn't using it. Let's leave it as is

/Ilias
>
>    Andrew