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

[Eric Dumazet]

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.
Forcing an inline makes no sense, this would duplicate the code four times,
for absolutely no gain.

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