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

[Eric Dumazet]

On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
<aleksander.lobakin@xxxxxxxxx> wrote:
>
> 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?
>

The __cold attribute is only applied to the helper _allocating_ the
memory, once.

Not on the functions actually incrementing the stats.

There are situations where they can be called thousands/millions of
times per second (incast flood).
If this situation happens, the _allocation_ still happens once.



> > 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