include/linux/netdevice.h | 32 +++++--------------------------- net/core/dev.c | 30 ++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 29 deletions(-)
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.
For the users, people are more concerned about why the dropped in ifconfig
is increasing. So we can export dev_core_stats_rx_dropped_inc sets,
which users would trace them know why rx_dropped is increasing.
Export dev_core_stats_{rx_dropped, tx_dropped, rx_nohandler,
rx_otherhost_dropped}_inc for trace. Also, move dev_core_stats()
and netdev_core_stats_alloc() in dev.c, because they are not called
externally.
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
include/linux/netdevice.h | 32 +++++---------------------------
net/core/dev.c | 30 ++++++++++++++++++++++++++++--
2 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0896aaa91dd7..879b01c85ba4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3954,6 +3954,11 @@ int dev_forward_skb_nomtu(struct net_device *dev, struct sk_buff *skb);
bool is_skb_forwardable(const struct net_device *dev,
const struct sk_buff *skb);
+void dev_core_stats_rx_dropped_inc(struct net_device *dev);
+void dev_core_stats_tx_dropped_inc(struct net_device *dev);
+void dev_core_stats_rx_nohandler_inc(struct net_device *dev);
+void dev_core_stats_rx_otherhost_dropped_inc(struct net_device *dev);
+
static __always_inline bool __is_skb_forwardable(const struct net_device *dev,
const struct sk_buff *skb,
const bool check_mtu)
@@ -3980,33 +3985,6 @@ static __always_inline bool __is_skb_forwardable(const struct net_device *dev,
return false;
}
-struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device *dev);
-
-static inline 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) \
-static inline 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); \
-}
-DEV_CORE_STATS_INC(rx_dropped)
-DEV_CORE_STATS_INC(tx_dropped)
-DEV_CORE_STATS_INC(rx_nohandler)
-DEV_CORE_STATS_INC(rx_otherhost_dropped)
-
static __always_inline int ____dev_forward_skb(struct net_device *dev,
struct sk_buff *skb,
const bool check_mtu)
diff --git a/net/core/dev.c b/net/core/dev.c
index ccff2b6ef958..32ba730405b4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10475,7 +10475,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
}
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)
+{
+ /* 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)
+
+DEV_CORE_STATS_INC(rx_dropped);
+DEV_CORE_STATS_INC(tx_dropped);
+DEV_CORE_STATS_INC(rx_nohandler);
+DEV_CORE_STATS_INC(rx_otherhost_dropped);
/**
* dev_get_stats - get network device statistics
--
2.25.1From: Yajun Deng <yajun.deng@linux.dev>
Date: Mon, 11 Sep 2023 16:20:16 +0800
> 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.
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ccff2b6ef958..32ba730405b4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10475,7 +10475,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
> }
> 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)
> +{
> + /* 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)
> +
> +DEV_CORE_STATS_INC(rx_dropped);
> +DEV_CORE_STATS_INC(tx_dropped);
> +DEV_CORE_STATS_INC(rx_nohandler);
> +DEV_CORE_STATS_INC(rx_otherhost_dropped);
I realize you need to have an external function to be able to trace it,
but why don't you make it just 1 function instead of 4+ (will only be
increasing)?
Define 1 function
void dev_core_stats_inc(struct net_device *dev, u32 offset)
{
struct net_device_core_stats __percpu *p;
p = dev_core_stats(dev);
if (p)
this_cpu_inc(*(unsigned long *)(void *)p + offset);
}
EXPORT_SYMBOL_GPL(dev_core_stats_inc); // Why not GPL BTW?
And then build inlines:
#define DEV_CORE_STATS_INC(FIELD) \
static inline void \
dev_core_stats_##FIELD##_inc(struct net_device *dev) \
{ \
dev_core_stats_inc(dev, \
offsetof(struct net_device_core_stats, FIELD)); \
}
DEV_CORE_STATS_INC(rx_dropped);
...
OR even just make them macros
#define __DEV_CORE_STATS_INC(dev, field) \
dev_core_stats_inc(dev, \
offsetof(struct net_device_core_stats, field))
#define dev_core_stats_rx_dropped_inc(dev) \
__DEV_CORE_STATS_INC(dev, rx_dropped)
...
Just don't copy that awful Thunderbird's line wrap and don't assume this
code builds and works and that is something finished/polished.
You'll be able to trace functions and you'll be able to understand which
counter has been incremented by checking the second argument, i.e. the
field offset (IIRC tracing shows you arguments).
And that way you wouldn't geometrically increase the number of symbol
exports and deal with its consequences.
>
> /**
> * dev_get_stats - get network device statistics
Thanks,
Olek
On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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.
>
> For the users, people are more concerned about why the dropped in ifconfig
> is increasing. So we can export dev_core_stats_rx_dropped_inc sets,
> which users would trace them know why rx_dropped is increasing.
>
> Export dev_core_stats_{rx_dropped, tx_dropped, rx_nohandler,
> rx_otherhost_dropped}_inc for trace. Also, move dev_core_stats()
> and netdev_core_stats_alloc() in dev.c, because they are not called
> externally.
>
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
Okay, but it seems you forgot to say which tree was targeted by this patch.
Documentation/process/maintainer-netdev.rst
I would guess net-next, but patch authors are supposed to be explicit.
> ---
> include/linux/netdevice.h | 32 +++++---------------------------
> net/core/dev.c | 30 ++++++++++++++++++++++++++++--
> 2 files changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0896aaa91dd7..879b01c85ba4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3954,6 +3954,11 @@ int dev_forward_skb_nomtu(struct net_device *dev, struct sk_buff *skb);
> bool is_skb_forwardable(const struct net_device *dev,
> const struct sk_buff *skb);
>
> +void dev_core_stats_rx_dropped_inc(struct net_device *dev);
> +void dev_core_stats_tx_dropped_inc(struct net_device *dev);
> +void dev_core_stats_rx_nohandler_inc(struct net_device *dev);
> +void dev_core_stats_rx_otherhost_dropped_inc(struct net_device *dev);
> +
> static __always_inline bool __is_skb_forwardable(const struct net_device *dev,
> const struct sk_buff *skb,
> const bool check_mtu)
> @@ -3980,33 +3985,6 @@ static __always_inline bool __is_skb_forwardable(const struct net_device *dev,
> return false;
> }
>
> -struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device *dev);
> -
> -static inline 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) \
> -static inline 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); \
> -}
> -DEV_CORE_STATS_INC(rx_dropped)
> -DEV_CORE_STATS_INC(tx_dropped)
> -DEV_CORE_STATS_INC(rx_nohandler)
> -DEV_CORE_STATS_INC(rx_otherhost_dropped)
> -
> static __always_inline int ____dev_forward_skb(struct net_device *dev,
> struct sk_buff *skb,
> const bool check_mtu)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ccff2b6ef958..32ba730405b4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10475,7 +10475,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
> }
> 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.
> +{
> + /* 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)
> +
> +DEV_CORE_STATS_INC(rx_dropped);
> +DEV_CORE_STATS_INC(tx_dropped);
> +DEV_CORE_STATS_INC(rx_nohandler);
> +DEV_CORE_STATS_INC(rx_otherhost_dropped);
#undef DEV_CORE_STATS_INC
>
> /**
> * dev_get_stats - get network device statistics
> --
> 2.25.1
>
From: Eric Dumazet <edumazet@google.com>
Date: Tue, 12 Sep 2023 06:23:24 +0200
> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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
On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Tue, 12 Sep 2023 06:23:24 +0200
>
> > On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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 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
From: Eric Dumazet <edumazet@google.com>
Date: Tue, 12 Sep 2023 18:04:44 +0200
> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Tue, 12 Sep 2023 06:23:24 +0200
>>
>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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 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
On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Tue, 12 Sep 2023 18:04:44 +0200
>
> > On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> > <aleksander.lobakin@intel.com> wrote:
> >>
> >> From: Eric Dumazet <edumazet@google.com>
> >> Date: Tue, 12 Sep 2023 06:23:24 +0200
> >>
> >>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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 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 <edumazet@google.com>
Date: Tue, 12 Sep 2023 19:28:50 +0200
> On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Tue, 12 Sep 2023 18:04:44 +0200
>>
>>> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
>>> <aleksander.lobakin@intel.com> wrote:
>>>>
>>>> From: Eric Dumazet <edumazet@google.com>
>>>> Date: Tue, 12 Sep 2023 06:23:24 +0200
>>>>
>>>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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 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
<aleksander.lobakin@intel.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Tue, 12 Sep 2023 19:28:50 +0200
>
> > On Tue, Sep 12, 2023 at 7:16 PM Alexander Lobakin
> > <aleksander.lobakin@intel.com> wrote:
> >>
> >> From: Eric Dumazet <edumazet@google.com>
> >> Date: Tue, 12 Sep 2023 18:04:44 +0200
> >>
> >>> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> >>> <aleksander.lobakin@intel.com> wrote:
> >>>>
> >>>> From: Eric Dumazet <edumazet@google.com>
> >>>> Date: Tue, 12 Sep 2023 06:23:24 +0200
> >>>>
> >>>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.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 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)
On Tue, Sep 12, 2023 at 8:03 PM Eric Dumazet <edumazet@google.com> wrote:
> 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)
Oh well, I just read the patch, and it seems wrong indeed.
netdev_core_stats_alloc() is the one that can be cold.
On 2023/9/13 02:05, Eric Dumazet wrote:
> On Tue, Sep 12, 2023 at 8:03 PM Eric Dumazet <edumazet@google.com> wrote:
>
>> 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)
> Oh well, I just read the patch, and it seems wrong indeed.
>
> netdev_core_stats_alloc() is the one that can be cold.
Okay, I would add __cold to netdev_core_stats_alloc() in v3.
Olek suggest that define a new dev_core_stats_inc() function.
I hope to see the suggestion in another reply.
On Mon, 11 Sep 2023 16:20:16 +0800 Yajun Deng <yajun.deng@linux.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. > > For the users, people are more concerned about why the dropped in > ifconfig is increasing. So we can export > dev_core_stats_rx_dropped_inc sets, which users would trace them know > why rx_dropped is increasing. ifconfig has been frozen for over 10 years, and is deprecated so there is no point in catering to legacy api's. There are better API's such as ethtool and netlink that can provide more info.
September 12, 2023 at 12:15 AM, "Stephen Hemminger" <stephen@networkplumber.org> wrote: > > On Mon, 11 Sep 2023 16:20:16 +0800 > Yajun Deng <yajun.deng@linux.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. > > > > For the users, people are more concerned about why the dropped in > > ifconfig is increasing. So we can export > > dev_core_stats_rx_dropped_inc sets, which users would trace them know > > why rx_dropped is increasing. > > > > ifconfig has been frozen for over 10 years, and is deprecated so there > is no point in catering to legacy api's. There are better API's such as > ethtool and netlink that can provide more info. > Yes, ifconfig is deprecated. but the dropped in ifconfig and ip is the same. We're more concerned about the reason for dropped packets. ip, ethtool and netlink couldn't show the reason.
© 2016 - 2025 Red Hat, Inc.