netdev - Re: [PATCH] net/core: Export dev_core_stats_rx_dropped

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3e84a37-3218-0d52-e7ed-2d215fed58e3@intel.com>
Date: Tue, 12 Sep 2023 17:57:26 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: Yajun Deng <yajun.deng@...ux.dev>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <horms@...nel.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets

From: Eric Dumazet <edumazet@...gle.com>
Date: Tue, 12 Sep 2023 06:23:24 +0200

> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@...ux.dev> wrote:
>>
>> 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.

[...]

>>  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)
> 
> Please remove this inline attritbute. Consider using __cold instead.

__cold? O_o I thought the author's inlining it as it's a couple
locs/intstructions, while the compilers would most likely keep it
non-inlined as it's referenced 4 times. __cold will for sure keep it
standalone and place it in .text.cold, i.e. far away from the call sites.
I realize dev_core_stats_*() aren't called frequently, but why making
only one small helper cold rather than all of them then?

> 
>> +{
>> +       /* 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);
>> +}

[...]

Thanks,
Olek