[Devel] [PATCH rh7] vznetstat: fix statistics output when getting stats for same device in parallel

[Pavel Tikhomirov]

On 28.08.2023 23:54, Konstantin Khorenko wrote:
> Before this patch .get_stats() for venetdev was not defended from
> racing, Documentation/networking/netdevices.txt states ndo_get_stats()
> is to be syncronized by dev_base_lock rwlock, but we do READ stats, so
> can go in parallel.
> 
> At the same time the algorithm of gathering per-cpu stats into a single
> value used per-device structure, so in case .get_stats() is called in
> parallel, stats could be corrupted.
> 
> Example, 2 cpu node:
> 
> 	cpu A					cpu B
> .get_stats()
> memset(dev->...->stats, 0)
> stats.rx_bytes += cpuA.rx_bytes
> 					.get_stats()
> 					memset(dev->...->stats, 0)
> stats.rx_bytes += cpuB.rx_bytes
> return stats
> 					stats.rx_bytes += cpuA.rx_bytes
> 					stats.rx_bytes += cpuB.rx_bytes
> 					return stats
> 
> => process running on cpu A will print rx_bytes only taken from cpu B
> (so lower than expected), and the process running on cpu B will print
> too high numbers because cpu B values will be added twice.
> 
> Let's rework the code to sumup per-cpu values to a local structure
> (on-stack).
> Extra 5 unsigned long variables seem to be fine to be put on stack.
> 
> https://jira.vzint.dev/browse/PSBM-150027
> 
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>   drivers/net/venetdev.c | 38 ++++++++++++++++++++++++--------------
>   include/linux/venet.h  | 10 +++++++++-
>   2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
> index 3ca4f8a49a5c..3ad67ee2f2b0 100644
> --- a/drivers/net/venetdev.c
> +++ b/drivers/net/venetdev.c
> @@ -568,25 +568,35 @@ static int venet_xmit(struct sk_buff *skb, struct net_device *dev)
>   	return 0;
>   }
>   
> -static struct net_device_stats *get_stats(struct net_device *dev)
> +static void venet_stats_one(struct venet_device_stats *result, struct net_device *dev)
>   {
> -	int i;
> -	struct venet_stats *stats;
> +	int cpu;
> +
> +	memset(result, 0, sizeof(struct venet_device_stats));
>   
> -	stats = (struct venet_stats *)dev->ml_priv;
> -	memset(&stats->stats, 0, sizeof(struct net_device_stats));
> -	for_each_possible_cpu(i) {
> +	for_each_possible_cpu(cpu) {
>   		struct net_device_stats *dev_stats;
>   
> -		dev_stats = venet_stats(dev, i);
> -		stats->stats.rx_bytes   += dev_stats->rx_bytes;
> -		stats->stats.tx_bytes   += dev_stats->tx_bytes;
> -		stats->stats.rx_packets += dev_stats->rx_packets;
> -		stats->stats.tx_packets += dev_stats->tx_packets;
> -		stats->stats.tx_dropped += dev_stats->tx_dropped;
> +		dev_stats = venet_stats(dev, cpu);
> +		result->rx_bytes += dev_stats->rx_bytes;
> +		result->tx_bytes += dev_stats->tx_bytes;
> +		result->rx_packets += dev_stats->rx_packets;
> +		result->tx_packets += dev_stats->tx_packets;
> +		result->tx_dropped += dev_stats->tx_dropped;
>   	}
> +}
>   
> -	return &stats->stats;
> +static void venet_get_stats64(struct net_device *dev,
> +			      struct rtnl_link_stats64 *total)
> +{
> +	struct venet_device_stats one;
> +
> +	venet_stats_one(&one, dev);

Having both venet_get_stats64 and venet_stats_one and adding struct 
venet_device_stats seems excess to me. Why can't we just fill 
rtnl_link_stats64 directly?

> +	total->rx_bytes = one.rx_bytes;
> +	total->tx_bytes = one.tx_bytes;
> +	total->rx_packets = one.rx_packets;
> +	total->tx_packets = one.tx_packets;
> +	total->tx_dropped = one.tx_dropped;
>   }
>   
>   /* Initialize the rest of the LOOPBACK device. */
> @@ -712,7 +722,7 @@ static const struct ethtool_ops venet_ethtool_ops = {
>   
>   static const struct net_device_ops venet_netdev_ops = {
>   	.ndo_start_xmit = venet_xmit,
> -	.ndo_get_stats = get_stats,
> +	.ndo_get_stats64 = venet_get_stats64,
>   	.ndo_open = venet_open,
>   	.ndo_stop = venet_close,
>   	.ndo_init = venet_init_dev,
> diff --git a/include/linux/venet.h b/include/linux/venet.h
> index 51e0abeb03d7..b54b0342a38a 100644
> --- a/include/linux/venet.h
> +++ b/include/linux/venet.h
> @@ -21,10 +21,18 @@
>   struct ve_struct;
>   struct venet_stat;
>   struct venet_stats {
> -	struct net_device_stats	stats;
>   	struct net_device_stats	*real_stats;
>   };
>   
> +struct venet_device_stats {
> +	unsigned long	rx_packets;
> +	unsigned long	tx_packets;
> +	unsigned long	rx_bytes;
> +	unsigned long	tx_bytes;
> +
> +	unsigned long	tx_dropped;
> +};
> +
>   struct ip_entry_struct
>   {
>   	struct ve_addr_struct	addr;

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.