Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets

[Alexander Lobakin]

From: Yajun Deng <yajun.deng@xxxxxxxxx>
Date: Mon, 11 Sep 2023 16:20:16 +0800

> Although there is a kfree_skb_reason() helper function that can be used
> to find the reason for dropped packets, but most callers didn't increase
> one of rx_dropped, tx_dropped, rx_nohandler and rx_otherhost_dropped.

[...]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index ccff2b6ef958..32ba730405b4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10475,7 +10475,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
>  }
>  EXPORT_SYMBOL(netdev_stats_to_stats64);
>  
> -struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device *dev)
> +static struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device *dev)
>  {
>  	struct net_device_core_stats __percpu *p;
>  
> @@ -10488,7 +10488,33 @@ struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device
>  	/* This READ_ONCE() pairs with the cmpxchg() above */
>  	return READ_ONCE(dev->core_stats);
>  }
> -EXPORT_SYMBOL(netdev_core_stats_alloc);
> +
> +static inline struct net_device_core_stats __percpu *dev_core_stats(struct net_device *dev)
> +{
> +	/* This READ_ONCE() pairs with the write in netdev_core_stats_alloc() */
> +	struct net_device_core_stats __percpu *p = READ_ONCE(dev->core_stats);
> +
> +	if (likely(p))
> +		return p;
> +
> +	return netdev_core_stats_alloc(dev);
> +}
> +
> +#define DEV_CORE_STATS_INC(FIELD)				\
> +void dev_core_stats_##FIELD##_inc(struct net_device *dev)	\
> +{								\
> +	struct net_device_core_stats __percpu *p;		\
> +								\
> +	p = dev_core_stats(dev);				\
> +	if (p)							\
> +		this_cpu_inc(p->FIELD);				\
> +}								\
> +EXPORT_SYMBOL(dev_core_stats_##FIELD##_inc)
> +
> +DEV_CORE_STATS_INC(rx_dropped);
> +DEV_CORE_STATS_INC(tx_dropped);
> +DEV_CORE_STATS_INC(rx_nohandler);
> +DEV_CORE_STATS_INC(rx_otherhost_dropped);

I realize you need to have an external function to be able to trace it,
but why don't you make it just 1 function instead of 4+ (will only be
increasing)?

Define 1 function

void dev_core_stats_inc(struct net_device *dev, u32 offset)
{
	struct net_device_core_stats __percpu *p;

	p = dev_core_stats(dev);
	if (p)
		this_cpu_inc(*(unsigned long *)(void *)p + offset);
}
EXPORT_SYMBOL_GPL(dev_core_stats_inc); // Why not GPL BTW?

And then build inlines:

#define DEV_CORE_STATS_INC(FIELD)				\
static inline void						\
dev_core_stats_##FIELD##_inc(struct net_device *dev)		\
{								\
	dev_core_stats_inc(dev,					\
		offsetof(struct net_device_core_stats, FIELD));	\
}

DEV_CORE_STATS_INC(rx_dropped);
...

OR even just make them macros

#define __DEV_CORE_STATS_INC(dev, field)			\
	dev_core_stats_inc(dev,					\
		offsetof(struct net_device_core_stats, field))

#define dev_core_stats_rx_dropped_inc(dev)			\
	__DEV_CORE_STATS_INC(dev, rx_dropped)
...

Just don't copy that awful Thunderbird's line wrap and don't assume this
code builds and works and that is something finished/polished.

You'll be able to trace functions and you'll be able to understand which
counter has been incremented by checking the second argument, i.e. the
field offset (IIRC tracing shows you arguments).
And that way you wouldn't geometrically increase the number of symbol
exports and deal with its consequences.

>  
>  /**
>   *	dev_get_stats	- get network device statistics

Thanks,
Olek