diff mbox series

net: stmmac: fix ethtool per-queue statistics

Message ID 20240105181024.14418-1-petr@tesarici.cz (mailing list archive)
State New, archived
Headers show
Series net: stmmac: fix ethtool per-queue statistics | expand

Commit Message

Petr Tesařík Jan. 5, 2024, 6:10 p.m. UTC
Fix per-queue statistics for devices with more than one queue.

The output data pointer is currently reset in each loop iteration,
effectively summing all queue statistics in the first four u64 values.

The summary values are not even labeled correctly. For example, if eth0 has
2 queues, ethtool -S eth0 shows:

     q0_tx_pkt_n: 374 (actually tx_pkt_n over all queues)
     q0_tx_irq_n: 23  (actually tx_normal_irq_n over all queues)
     q1_tx_pkt_n: 462 (actually rx_pkt_n over all queues)
     q1_tx_irq_n: 446 (actually rx_normal_irq_n over all queues)
     q0_rx_pkt_n: 0
     q0_rx_irq_n: 0
     q1_rx_pkt_n: 0
     q1_rx_irq_n: 0

While touching this code, change the pointer type to u64 and get rid of the
weird pointer arithmetic.

Signed-off-by: Petr Tesarik <petr@tesarici.cz>
Fixes: 133466c3bbe1 ("net: stmmac: use per-queue 64 bit statistics where necessary")
---
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 23 ++++++-------------
 1 file changed, 7 insertions(+), 16 deletions(-)

Comments

Petr Tesařík Jan. 5, 2024, 6:24 p.m. UTC | #1
On Fri,  5 Jan 2024 19:10:24 +0100
Petr Tesarik <petr@tesarici.cz> wrote:

> Fix per-queue statistics for devices with more than one queue.
> 
> The output data pointer is currently reset in each loop iteration,
> effectively summing all queue statistics in the first four u64 values.
> 
> The summary values are not even labeled correctly. For example, if eth0 has
> 2 queues, ethtool -S eth0 shows:
> 
>      q0_tx_pkt_n: 374 (actually tx_pkt_n over all queues)
>      q0_tx_irq_n: 23  (actually tx_normal_irq_n over all queues)
>      q1_tx_pkt_n: 462 (actually rx_pkt_n over all queues)
>      q1_tx_irq_n: 446 (actually rx_normal_irq_n over all queues)
>      q0_rx_pkt_n: 0
>      q0_rx_irq_n: 0
>      q1_rx_pkt_n: 0
>      q1_rx_irq_n: 0
> 
> While touching this code, change the pointer type to u64 and get rid of the
> weird pointer arithmetic.

Just to make sure, this fix has nothing to do with my previous stmmac
fix. It's just a fix for something I noticed while working on the other
patch.

I hope this fix can be merged fast, so I can base my other patch on it.

Petr T

> Signed-off-by: Petr Tesarik <petr@tesarici.cz>
> Fixes: 133466c3bbe1 ("net: stmmac: use per-queue 64 bit statistics where necessary")
> ---
>  .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 23 ++++++-------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index f628411ae4ae..023876fc4da7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -543,43 +543,34 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
>  	u32 rx_cnt = priv->plat->rx_queues_to_use;
>  	unsigned int start;
>  	int q, stat;
> -	u64 *pos;
> -	char *p;
> +	u64 *p;
>  
> -	pos = data;
>  	for (q = 0; q < tx_cnt; q++) {
>  		struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[q];
>  		struct stmmac_txq_stats snapshot;
>  
> -		data = pos;
>  		do {
>  			start = u64_stats_fetch_begin(&txq_stats->syncp);
>  			snapshot = *txq_stats;
>  		} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
>  
> -		p = (char *)&snapshot + offsetof(struct stmmac_txq_stats, tx_pkt_n);
> -		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
> -			*data++ += (*(u64 *)p);
> -			p += sizeof(u64);
> -		}
> +		p = &snapshot.tx_pkt_n;
> +		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++)
> +			*data++ = *p++;
>  	}
>  
> -	pos = data;
>  	for (q = 0; q < rx_cnt; q++) {
>  		struct stmmac_rxq_stats *rxq_stats = &priv->xstats.rxq_stats[q];
>  		struct stmmac_rxq_stats snapshot;
>  
> -		data = pos;
>  		do {
>  			start = u64_stats_fetch_begin(&rxq_stats->syncp);
>  			snapshot = *rxq_stats;
>  		} while (u64_stats_fetch_retry(&rxq_stats->syncp, start));
>  
> -		p = (char *)&snapshot + offsetof(struct stmmac_rxq_stats, rx_pkt_n);
> -		for (stat = 0; stat < STMMAC_RXQ_STATS; stat++) {
> -			*data++ += (*(u64 *)p);
> -			p += sizeof(u64);
> -		}
> +		p = &snapshot.rx_pkt_n;
> +		for (stat = 0; stat < STMMAC_RXQ_STATS; stat++)
> +			*data++ = *p++;
>  	}
>  }
>
Andrew Lunn Jan. 5, 2024, 7:54 p.m. UTC | #2
On Fri, Jan 05, 2024 at 07:10:24PM +0100, Petr Tesarik wrote:
> Fix per-queue statistics for devices with more than one queue.
> 
> The output data pointer is currently reset in each loop iteration,
> effectively summing all queue statistics in the first four u64 values.
> 
> The summary values are not even labeled correctly. For example, if eth0 has
> 2 queues, ethtool -S eth0 shows:
> 
>      q0_tx_pkt_n: 374 (actually tx_pkt_n over all queues)
>      q0_tx_irq_n: 23  (actually tx_normal_irq_n over all queues)
>      q1_tx_pkt_n: 462 (actually rx_pkt_n over all queues)
>      q1_tx_irq_n: 446 (actually rx_normal_irq_n over all queues)
>      q0_rx_pkt_n: 0
>      q0_rx_irq_n: 0
>      q1_rx_pkt_n: 0
>      q1_rx_irq_n: 0
> 
> While touching this code, change the pointer type to u64 and get rid of the
> weird pointer arithmetic.
> 
> Signed-off-by: Petr Tesarik <petr@tesarici.cz>
> Fixes: 133466c3bbe1 ("net: stmmac: use per-queue 64 bit statistics where necessary")

Hi Petr

There are a few process things you are missing. Please take a look at

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

You need to indicate which tree this is for.

Additionally, your Signed-off-by comes last.

Patches for stable should ideally be minimal. And obviously correct. See:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

So the bits about changing the pointer type and removing the weird
arithmetic might be better suited for net-next, not net.

	      Andrew
Petr Tesařík Jan. 5, 2024, 8:13 p.m. UTC | #3
On Fri, 5 Jan 2024 20:54:01 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Jan 05, 2024 at 07:10:24PM +0100, Petr Tesarik wrote:
> > Fix per-queue statistics for devices with more than one queue.
> > 
> > The output data pointer is currently reset in each loop iteration,
> > effectively summing all queue statistics in the first four u64 values.
> > 
> > The summary values are not even labeled correctly. For example, if eth0 has
> > 2 queues, ethtool -S eth0 shows:
> > 
> >      q0_tx_pkt_n: 374 (actually tx_pkt_n over all queues)
> >      q0_tx_irq_n: 23  (actually tx_normal_irq_n over all queues)
> >      q1_tx_pkt_n: 462 (actually rx_pkt_n over all queues)
> >      q1_tx_irq_n: 446 (actually rx_normal_irq_n over all queues)
> >      q0_rx_pkt_n: 0
> >      q0_rx_irq_n: 0
> >      q1_rx_pkt_n: 0
> >      q1_rx_irq_n: 0
> > 
> > While touching this code, change the pointer type to u64 and get rid of the
> > weird pointer arithmetic.
> > 
> > Signed-off-by: Petr Tesarik <petr@tesarici.cz>
> > Fixes: 133466c3bbe1 ("net: stmmac: use per-queue 64 bit statistics where necessary")  
> 
> Hi Petr
> 
> There are a few process things you are missing. Please take a look at
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> You need to indicate which tree this is for.

Ah. Thank you for the pointer. You see, this is my first netdev patch...

> Additionally, your Signed-off-by comes last.

Oh, I'm sorry for that.

> Patches for stable should ideally be minimal. And obviously correct. See:
> 
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> So the bits about changing the pointer type and removing the weird
> arithmetic might be better suited for net-next, not net.

Good. Given that I have meanwhile found out that I will have to change
this whole function to fix the lockup on 32-bit systems, let me just
discard this part, add "net" to the subject and cc stable.

Resending...

Petr T
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index f628411ae4ae..023876fc4da7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -543,43 +543,34 @@  static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
 	u32 rx_cnt = priv->plat->rx_queues_to_use;
 	unsigned int start;
 	int q, stat;
-	u64 *pos;
-	char *p;
+	u64 *p;
 
-	pos = data;
 	for (q = 0; q < tx_cnt; q++) {
 		struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[q];
 		struct stmmac_txq_stats snapshot;
 
-		data = pos;
 		do {
 			start = u64_stats_fetch_begin(&txq_stats->syncp);
 			snapshot = *txq_stats;
 		} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
 
-		p = (char *)&snapshot + offsetof(struct stmmac_txq_stats, tx_pkt_n);
-		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
-			*data++ += (*(u64 *)p);
-			p += sizeof(u64);
-		}
+		p = &snapshot.tx_pkt_n;
+		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++)
+			*data++ = *p++;
 	}
 
-	pos = data;
 	for (q = 0; q < rx_cnt; q++) {
 		struct stmmac_rxq_stats *rxq_stats = &priv->xstats.rxq_stats[q];
 		struct stmmac_rxq_stats snapshot;
 
-		data = pos;
 		do {
 			start = u64_stats_fetch_begin(&rxq_stats->syncp);
 			snapshot = *rxq_stats;
 		} while (u64_stats_fetch_retry(&rxq_stats->syncp, start));
 
-		p = (char *)&snapshot + offsetof(struct stmmac_rxq_stats, rx_pkt_n);
-		for (stat = 0; stat < STMMAC_RXQ_STATS; stat++) {
-			*data++ += (*(u64 *)p);
-			p += sizeof(u64);
-		}
+		p = &snapshot.rx_pkt_n;
+		for (stat = 0; stat < STMMAC_RXQ_STATS; stat++)
+			*data++ = *p++;
 	}
 }