[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cc269865-d3c7-f8e6-9a61-25794f5ae220@iogearbox.net>
Date: Mon, 13 Nov 2023 14:05:36 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Simon Horman <horms@...nel.org>
Cc: martin.lau@...nel.org, kuba@...nel.org, razor@...ckwall.org,
sdf@...gle.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH bpf v2 2/8] net: Move {l,t,d}stats allocation to core and
convert veth & vrf
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@...nel.org>
>> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
>> Cc: David Ahern <dsahern@...nel.org>
>
> 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
Powered by blists - more mailing lists