Re: [PATCH net-next v3 2/2] net: stmmac: use pcpu 64 bit statistics where necessary

[Simon Horman]

On Tue, Jun 20, 2023 at 12:52:20AM +0800, Jisheng Zhang wrote:

...

Hi Jisheng Zhang,

some minor feedback from my side, as it seems there will be a v4 anyway.

> @@ -535,23 +548,37 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
>  {
>  	u32 tx_cnt = priv->plat->tx_queues_to_use;
>  	u32 rx_cnt = priv->plat->rx_queues_to_use;
> -	int q, stat;
> +	unsigned int start;
> +	int q, stat, cpu;
>  	char *p;
> -
> -	for (q = 0; q < tx_cnt; q++) {
> -		p = (char *)priv + offsetof(struct stmmac_priv,
> -					    xstats.txq_stats[q].tx_pkt_n);
> -		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
> -			*data++ = (*(unsigned long *)p);
> -			p += sizeof(unsigned long);
> +	u64 *pos;

Please use reverse xmas tree - longest line to shortest - for local
variable declarations in new Networking code.

...

> @@ -563,7 +590,8 @@ static void stmmac_get_ethtool_stats(struct net_device *dev,
>  	u32 rx_queues_count = priv->plat->rx_queues_to_use;
>  	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>  	unsigned long count;
> -	int i, j = 0, ret;
> +	unsigned int start;
> +	int i, j = 0, pos, ret, cpu;

Ditto.

...

> @@ -606,6 +633,22 @@ static void stmmac_get_ethtool_stats(struct net_device *dev,
>  		data[j++] = (stmmac_gstrings_stats[i].sizeof_stat ==
>  			     sizeof(u64)) ? (*(u64 *)p) : (*(u32 *)p);
>  	}
> +	pos = j;
> +	for_each_possible_cpu(cpu) {
> +		struct stmmac_pcpu_stats *stats, snapshot;
> +
> +		stats = per_cpu_ptr(priv->xstats.pstats, cpu);
> +		j = pos;
> +		do {
> +			start = u64_stats_fetch_begin(&stats->syncp);
> +			snapshot = *stats;
> +		} while (u64_stats_fetch_retry(&stats->syncp, start));
> +
> +		for (i = 0; i < STMMAC_PCPU_STATS_LEN; i++) {
> +			char *p = (char *)&snapshot + stmmac_gstrings_pcpu_stats[i].stat_offset;

Blank line here please.

> +			data[j++] += *(u64 *)p;
> +		}
> +	}
>  	stmmac_get_per_qstats(priv, &data[j]);
>  }
>  

...

> @@ -695,6 +738,11 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>  				ETH_GSTRING_LEN);
>  			p += ETH_GSTRING_LEN;
>  		}
> +		for (i = 0; i < STMMAC_PCPU_STATS_LEN; i++) {
> +			memcpy(p, stmmac_gstrings_pcpu_stats[i].stat_string,
> +				ETH_GSTRING_LEN);

The indentation of the line above isn't quite right: it should align
with the inside of the opening parentheses on the previous line.

			memcpy(p, stmmac_gstrings_pcpu_stats[i].stat_string,
			       ETH_GSTRING_LEN);

> +			p += ETH_GSTRING_LEN;
> +		}
>  		stmmac_get_qstats_string(priv, p);
>  		break;
>  	case ETH_SS_TEST:

...

> @@ -5015,8 +5042,10 @@ static struct stmmac_xdp_buff *xsk_buff_to_stmmac_ctx(struct xdp_buff *xdp)
>  
>  static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  {
> +	struct stmmac_pcpu_stats *stats = this_cpu_ptr(priv->xstats.pstats);
>  	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
>  	unsigned int count = 0, error = 0, len = 0;
> +	u32 rx_errors = 0, rx_dropped = 0;
>  	int dirty = stmmac_rx_dirty(priv, queue);
>  	unsigned int next_entry = rx_q->cur_rx;
>  	unsigned int desc_size;

Reverse xmas tree here too.

...