Re: [PATCH] net: airoha: fix MIB stats collection to be lossless

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Hi Lorenzo,
> 
> Thank you for the detailed review and suggestions!
> 
> > > +	/* TX - 64-bit H+L registers: hw accumulates the total, read directly. =
> > */
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_H(port->id));
> > > -	dev->stats.tx_ok_pkts += ((u64)val << 32);
> > > -	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
> > > -	dev->stats.tx_ok_pkts += val;
> > > +	dev->stats.tx_ok_pkts = (u64)val << 32;
> > 
> > I guess it is more readable to store REG_FE_GDM_TX_OK_PKT_CNT_L() read in v=
> > al
> > here. Something like:
> > 
> > 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
> > 	dev->stats.tx_ok_pkts += val;
> > 
> > This apply even to occurrence below
> Agreed. I'll store CNT_L read in val first to improve readability.
> 
> > > +	dev->stats.tx_ok_pkts += airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L=
> > (port->id));
> > > =20
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT_H(port->id));
> > > -	dev->stats.tx_ok_bytes += ((u64)val << 32);
> > > -	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT_L(port->id));
> > > -	dev->stats.tx_ok_bytes += val;
> > > +	dev->stats.tx_ok_bytes = (u64)val << 32;
> > > +	dev->stats.tx_ok_bytes += airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT=
> > _L(port->id));
> > > =20
> > > +	/* TX - 32-bit registers: accumulate delta to handle wrap-around. */
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_DROP_CNT(port->id));
> > > -	dev->stats.tx_drops += val;
> > > +	dev->stats.tx_drops += (u32)(val - dev->stats.hw_prev_stats.tx_drops);
> > > +	dev->stats.hw_prev_stats.tx_drops = val;
> > > =20
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_BC_CNT(port->id));
> > > -	dev->stats.tx_broadcast += val;
> > > +	dev->stats.tx_broadcast += (u32)(val - dev->stats.hw_prev_stats.tx_br=
> > oadcast);
> > > +	dev->stats.hw_prev_stats.tx_broadcast = val;
> > > =20
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_MC_CNT(port->id));
> > > -	dev->stats.tx_multicast += val;
> > > +	dev->stats.tx_multicast += (u32)(val - dev->stats.hw_prev_stats.tx_mu=
> > lticast);
> > > +	dev->stats.hw_prev_stats.tx_multicast = val;
> > > =20
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_RUNT_CNT(port->id));
> > > -	dev->stats.tx_len[i] += val;
> > > +	dev->stats.tx_len[i] += (u32)(val - dev->stats.hw_prev_stats.tx_len[i=
> > ]);
> > > +	dev->stats.hw_prev_stats.tx_len[i] = val;
> > > =20
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_H(port->id));
> > > -	dev->stats.tx_len[i] += ((u64)val << 32);
> > > -	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_L(port->id));
> > > -	dev->stats.tx_len[i++] += val;
> > > +	dev->stats.tx_len[i] += (u64)val << 32;
> >  
> > Since now we do not reset MIB counters, this is wrong, you can't use "+="
> 
> You are absolutely right, since MIB counters are no longer cleared, using "+=" for E64 counter would cause double counting each iteration. This was missed in the patch, specifically for the case where runt count(32 bit) and E64 counter (64 bit) need to be combined in the same counter. 
> 
> I'll fix this by using separate accumulator fields to "tx_runt_accum/rx_runt_accum" to track the runt deltas, then compute tx_len[i] as tx_len[i]= tx_runt_accum + E64_CNT (H+L).
> 
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_RUNT_CNT(port->id));
> > > -	dev->stats.rx_len[i] += val;
> > > +	dev->stats.rx_len[i] += (u32)(val - dev->stats.hw_prev_stats.rx_len[i=
> > ]);
> > > +	dev->stats.hw_prev_stats.rx_len[i] = val;
> > > =20
> > >  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_H(port->id));
> > > -	dev->stats.rx_len[i] += ((u64)val << 32);
> > > -	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_L(port->id));
> > > -	dev->stats.rx_len[i++] += val;
> > > +	dev->stats.rx_len[i] += (u64)val << 32;
> > 
> > same here.
> 
> Acked. The same approach above will be applied to rx_len[i]. 
> 
> > > +
> > > +	struct {
> > > +	/* Previous HW register values for 32-bit counter delta tracking.
> > > +	 * Storing the last seen value and accumulating (u32)(curr - prev)
> > > +	 * in 64-bit software counter & handles wrap-around transparently
> > > +	 * via unsigned arithmetic. These fields are never reported to
> > > +	 * userspace.
> > > +	 */
> > 
> > can you please align the comment here?
> 
> Will fix the comment alignment.
> 
> > 
> > > +		u32 tx_drops;
> > > +		u32 tx_broadcast;
> > > +		u32 tx_multicast;
> > > +		u32 tx_len[7];
> > > +		u32 rx_drops;
> > > +		u32 rx_broadcast;
> > > +		u32 rx_multicast;
> > > +		u32 rx_errors;
> > > +		u32 rx_crc_error;
> > > +		u32 rx_over_errors;
> > > +		u32 rx_fragment;
> > > +		u32 rx_jabber;
> > > +		u32 rx_len[7];
> > > +	} hw_prev_stats;
> > 
> > Maybe something like "prev_val32" ?
> > 
> > Will update the name of struct to hold prev counter from hw_pre_stats to prev_val32.
> 
> Good suggestion. However, since the struct hw_prev_stats now contains both u32 (previous register value) and u64 (runt accumulators) fields. I'll rename it to "prev_mib_state" to better reflect its dual purpose of storing previous register values for delta calculation and accumulators for combined counters. 

Maybe better mib_prev?

Since now we do not reset the MIB counters in airoha_update_hw_stats(), we can
get rid of the for loop there and just call airoha_dev_get_hw_stats() with the
provided dev pointer. Even better, just rename airoha_dev_get_hw_stats() in
airoha_update_hw_stats() and move the spinlock there. What do you think?

Regards,
Lorenzo

>   
> Regards,
> Aniket Negi
> 

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