2023-09-12 20:43:14

by Eric Dumazet

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

On Tue, Sep 12, 2023 at 8:03 PM Eric Dumazet <[email protected]> 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.

2023-09-13 08:27:17

by Yajun Deng

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

On 2023/9/13 02:05, Eric Dumazet wrote:
> On Tue, Sep 12, 2023 at 8:03 PM Eric Dumazet <[email protected]> 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.