Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets

[Alexander Lobakin]

From: Eric Dumazet <edumazet@xxxxxxxxxx>
Date: Tue, 12 Sep 2023 18:04:44 +0200

> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> <aleksander.lobakin@xxxxxxxxx> wrote:
>>
>> From: Eric Dumazet <edumazet@xxxxxxxxxx>
>> Date: Tue, 12 Sep 2023 06:23:24 +0200
>>
>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@xxxxxxxxx> 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 helper is used at least one time per netdevice lifetime.
> This is definitely cold.

But then each dev_stats_*_inc() (not cold) has to call it from a
completely different piece of .text far from their. I either don't
understand the idea or dunno. Why not make them cold as well then?

> Forcing an inline makes no sense, this would duplicate the code four times,
> for absolutely no gain.

I'd love to see bloat-o-meter numbers, I suspect we're talking about
20-30 bytes.

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

Thanks,
Olek