Re: [PATCH net-next v7] net/core: Introduce netdev_core_stats_inc()

[Yajun Deng <yajun.deng@linux.dev>]

On 2023/10/8 14:45, Eric Dumazet wrote:
> On Sat, Oct 7, 2023 at 8:34 AM Yajun Deng <yajun.deng@linux.dev> wrote:
>>
>> On 2023/10/7 13:29, Eric Dumazet wrote:
>>> On Sat, Oct 7, 2023 at 7:06 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 why this skb is dropped, but most callers didn't increase
>>>> one of rx_dropped, tx_dropped, rx_nohandler and rx_otherhost_dropped.
>>>>
>>> ...
>>>
>>>> +
>>>> +void netdev_core_stats_inc(struct net_device *dev, u32 offset)
>>>> +{
>>>> +       /* This READ_ONCE() pairs with the write in netdev_core_stats_alloc() */
>>>> +       struct net_device_core_stats __percpu *p = READ_ONCE(dev->core_stats);
>>>> +       unsigned long *field;
>>>> +
>>>> +       if (unlikely(!p))
>>>> +               p = netdev_core_stats_alloc(dev);
>>>> +
>>>> +       if (p) {
>>>> +               field = (unsigned long *)((void *)this_cpu_ptr(p) + offset);
>>>> +               WRITE_ONCE(*field, READ_ONCE(*field) + 1);
>>> This is broken...
>>>
>>> As I explained earlier, dev_core_stats_xxxx(dev) can be called from
>>> many different contexts:
>>>
>>> 1) process contexts, where preemption and migration are allowed.
>>> 2) interrupt contexts.
>>>
>>> Adding WRITE_ONCE()/READ_ONCE() is not solving potential races.
>>>
>>> I _think_ I already gave you how to deal with this ?
>>
>> Yes, I replied in v6.
>>
>> https://lore.kernel.org/all/e25b5f3c-bd97-56f0-de86-b93a3172870d@linux.dev/
>>
>>> Please try instead:
>>>
>>> +void netdev_core_stats_inc(struct net_device *dev, u32 offset)
>>> +{
>>> +       /* This READ_ONCE() pairs with the write in netdev_core_stats_alloc() */
>>> +       struct net_device_core_stats __percpu *p = READ_ONCE(dev->core_stats);
>>> +       unsigned long __percpu *field;
>>> +
>>> +       if (unlikely(!p)) {
>>> +               p = netdev_core_stats_alloc(dev);
>>> +               if (!p)
>>> +                       return;
>>> +       }
>>> +       field = (__force unsigned long __percpu *)((__force void *)p + offset);
>>> +       this_cpu_inc(*field);
>>> +}
>>
>> This wouldn't trace anything even the rx_dropped is in increasing. It
>> needs to add an extra operation, such as:
> I honestly do not know what you are talking about.
>
> Have you even tried to change your patch to use
>
> field = (__force unsigned long __percpu *)((__force void *)p + offset);
> this_cpu_inc(*field);


Yes, I tested this code. But the following couldn't show anything even 
if the rx_dropped is increasing.

'sudo python3 /usr/share/bcc/tools/trace netdev_core_stats_inc'

It needs to add anything else. The above command will show correctly.

>
> Instead of the clearly buggy code you had instead :
>
>      field = (unsigned long *)((void *)this_cpu_ptr(p) + offset);
>       WRITE_ONCE(*field, READ_ONCE(*field) + 1);
>
> If your v7 submission was ok for tracing what you wanted,
> I fail to see why a v8 with 3 lines changed would not work.


Me too.

If I add a pr_info in your code, the kprobe will be ok.