Messages in this thread | | | From | Eric Dumazet <> | Date | Sun, 8 Oct 2023 08:45:00 +0200 | Subject | Re: [PATCH net-next v7] net/core: Introduce netdev_core_stats_inc() |
| |
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);
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.
| |