Re: [PATCH net-next v2] octeontx2-pf: persist netdev stats across routine operations

[Dragos Tatulea]

On 11.05.26 11:42, Anshumali Gaur wrote:
> Currently netdev driver is not retaining ethtool stats during
> interface up and down, as per kernel driver standard netdev driver
> must keep stats constant to avoid race conditions with user space
> trying to read them. Stats must persist across routine
> operations like bringing the interface down and up.
> 
> Signed-off-by: Anshumali Gaur <agaur@xxxxxxxxxxx>
> ---
> v2:
>   - Fix subject prefix to target net-next
>  .../marvell/octeontx2/nic/otx2_common.c       | 20 ++++++++++---------
>  .../marvell/octeontx2/nic/otx2_common.h       |  1 +
>  .../ethernet/marvell/octeontx2/nic/otx2_pf.c  | 18 +++++++++++++++++
>  3 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 971fcab1c248..0ffcc613a4b2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -139,19 +139,21 @@ void otx2_get_stats64(struct net_device *netdev,
>  		      struct rtnl_link_stats64 *stats)
>  {
>  	struct otx2_nic *pfvf = netdev_priv(netdev);
> -	struct otx2_dev_stats *dev_stats;
> +	struct otx2_dev_stats *dev_stats, *old_stats;
>  
>  	otx2_get_dev_stats(pfvf);
>  
>  	dev_stats = &pfvf->hw.dev_stats;
> -	stats->rx_bytes = dev_stats->rx_bytes;
> -	stats->rx_packets = dev_stats->rx_frames;
> -	stats->rx_dropped = dev_stats->rx_drops;
> -	stats->multicast = dev_stats->rx_mcast_frames;
> -
> -	stats->tx_bytes = dev_stats->tx_bytes;
> -	stats->tx_packets = dev_stats->tx_frames;
> -	stats->tx_dropped = dev_stats->tx_drops;
> +	old_stats = &pfvf->hw.old_stats;
> +
> +	stats->rx_bytes = old_stats->rx_bytes + dev_stats->rx_bytes;
> +	stats->rx_packets = old_stats->rx_frames + dev_stats->rx_frames;
> +	stats->rx_dropped = old_stats->rx_drops + dev_stats->rx_drops;
> +	stats->multicast = old_stats->rx_mcast_frames + dev_stats->rx_mcast_frames;
> +
> +	stats->tx_bytes = old_stats->tx_bytes + dev_stats->tx_bytes;
> +	stats->tx_packets = old_stats->tx_frames + dev_stats->tx_frames;
> +	stats->tx_dropped = old_stats->tx_drops + dev_stats->tx_drops;
I don't quite understand why the additional old_stats is required.
Why not accumulate dev_stats in place? Either directly in
otx2_get_dev_stats() or pass a local struct otx2_dev_stats var
and then accumulate that into dev_stats?

Thanks,
Dragos