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 <khore...@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.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
