On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
<[email protected]> wrote:
>
> From: Eric Dumazet <[email protected]>
> Date: Tue, 12 Sep 2023 18:04:44 +0200
>
> > On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> > <[email protected]> wrote:
> >>
> >> From: Eric Dumazet <[email protected]>
> >> Date: Tue, 12 Sep 2023 06:23:24 +0200
> >>
> >>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <[email protected]> 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
From: Eric Dumazet <[email protected]>
Date: Tue, 12 Sep 2023 19:28:50 +0200
> On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
> <[email protected]> wrote:
>>
>> From: Eric Dumazet <[email protected]>
>> Date: Tue, 12 Sep 2023 18:04:44 +0200
>>
>>> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
>>> <[email protected]> wrote:
>>>>
>>>> From: Eric Dumazet <[email protected]>
>>>> Date: Tue, 12 Sep 2023 06:23:24 +0200
>>>>
>>>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <[email protected]> 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.
Then it should be applied to netdev_core_stats_alloc(), not
dev_core_stats(). The latter only dereferences the already existing
pointer or calls the former, which actually does the allocation.
That's why I don't get why make one if/else non-inline or even cold.
>
> 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.
Correct, but dev_core_stats() will be called the same millions of times
per second, see above. It's called unconditionally each increment.
So seems like I got the idea of .cold correctly, but you were referring
to the wrong function.
>
>
>
>>> 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
Thanks,
Olek
On Tue, Sep 12, 2023 at 7:44 PM Alexander Lobakin
<[email protected]> wrote:
>
> From: Eric Dumazet <[email protected]>
> Date: Tue, 12 Sep 2023 19:28:50 +0200
>
> > On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
> > <[email protected]> wrote:
> >>
> >> From: Eric Dumazet <[email protected]>
> >> Date: Tue, 12 Sep 2023 18:04:44 +0200
> >>
> >>> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> >>> <[email protected]> wrote:
> >>>>
> >>>> From: Eric Dumazet <[email protected]>
> >>>> Date: Tue, 12 Sep 2023 06:23:24 +0200
> >>>>
> >>>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <[email protected]> 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.
>
> Then it should be applied to netdev_core_stats_alloc(), not
> dev_core_stats(). The latter only dereferences the already existing
> pointer or calls the former, which actually does the allocation.
> That's why I don't get why make one if/else non-inline or even cold.
Sure, this was what was suggested (perhaps not _very_ precisely, but
the general idea was pretty clear).
v2 seems ok, right ?
It seems we are all on the same page.
+static __cold struct net_device_core_stats __percpu
*dev_core_stats(struct net_device *dev)
+{
+ /* 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);
+}
+
+#define DEV_CORE_STATS_INC(FIELD) \
+void dev_core_stats_##FIELD##_inc(struct net_device *dev) \
+{ \
+ struct net_device_core_stats __percpu *p; \
+ \
+ p = dev_core_stats(dev); \
+ if (p) \
+ this_cpu_inc(p->FIELD); \
+} \
+EXPORT_SYMBOL(dev_core_stats_##FIELD##_inc)