On 11/13/23 11:03 AM, Simon Horman wrote:
On Sun, Nov 12, 2023 at 09:30:03PM +0100, Daniel Borkmann wrote:
Move {l,t,d}stats allocation to the core and let netdevs pick the stats
type they need. That way the driver doesn't have to bother with error
handling (allocation failure checking, making sure free happens in the
right spot, etc) - all happening in the core.
Co-developed-by: Jakub Kicinski <kuba@xxxxxxxxxx>
Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
Cc: David Ahern <dsahern@xxxxxxxxxx>
Hi Daniel,
sorry I was a bit to hasty in hitting the send button for my previous
message. I have a some more minor feedback on this series.
diff --git a/net/core/dev.c b/net/core/dev.c
index 0d548431f3fa..75db81496db5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10049,6 +10049,44 @@ void netif_tx_stop_all_queues(struct net_device *dev)
}
EXPORT_SYMBOL(netif_tx_stop_all_queues);
+static int netdev_do_alloc_pcpu_stats(struct net_device *dev)
+{
+ void __percpu *v;
+
+ switch (dev->pcpu_stat_type) {
+ case NETDEV_PCPU_STAT_NONE:
+ return 0;
+ case NETDEV_PCPU_STAT_LSTATS:
+ v = dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats);
+ break;
+ case NETDEV_PCPU_STAT_TSTATS:
+ v = dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
+ break;
+ case NETDEV_PCPU_STAT_DSTATS:
+ v = dev->dstats = netdev_alloc_pcpu_stats(struct pcpu_dstats);
+ break;
+ }
+
+ return v ? 0 : -ENOMEM;
Perhaps this cannot happen, but if none of the cases in the switch
statement are met, then v will be uninitialised here.
As flagged by Smatch.
Good point, I'll add a guard in case someone tries to set an invalid value
outside of the enum.
+}
+
...
@@ -10469,6 +10513,7 @@ void netdev_run_todo(void)
WARN_ON(rcu_access_pointer(dev->ip_ptr));
WARN_ON(rcu_access_pointer(dev->ip6_ptr));
+ netdev_do_free_pcpu_stats(dev);
if (dev->priv_destructor)
dev->priv_destructor(dev);
if (dev->needs_free_netdev)
nit: the hunk above seems unnecessary; one blank line is enough.
I'm not sure which one you mean?
Thanks,
Daniel
