Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231007050621.1706331-1-yajun.deng@linux.dev>
 <CANn89iL-zUw1FqjYRSC7BGB0hfQ5uKpJzUba3YFd--c=GdOoGg@mail.gmail.com> <917708b5-cb86-f233-e878-9233c4e6c707@linux.dev>
In-Reply-To: <917708b5-cb86-f233-e878-9233c4e6c707@linux.dev>
From:   Eric Dumazet <edumazet@google.com>
Date:   Sun, 8 Oct 2023 08:45:00 +0200
Message-ID: <CANn89i+navyRe8-AV=ehM3qFce2hmnOEKBqvK5Xnev7KTaS5Lg@mail.gmail.com>
Subject: Re: [PATCH net-next v7] net/core: Introduce netdev_core_stats_inc()
To:     Yajun Deng <yajun.deng@linux.dev>
Cc:     davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Oct 7, 2023 at 8:34=E2=80=AFAM Yajun Deng <yajun.deng@linux.dev> wr=
ote:
>
>
> On 2023/10/7 13:29, Eric Dumazet wrote:
> > On Sat, Oct 7, 2023 at 7:06=E2=80=AFAM Yajun Deng <yajun.deng@linux.dev=
> wrote:
> >> Although there is a kfree_skb_reason() helper function that can be use=
d to
> >> find the reason why this skb is dropped, but most callers didn't incre=
ase
> >> 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 =3D READ_ONCE(dev->co=
re_stats);
> >> +       unsigned long *field;
> >> +
> >> +       if (unlikely(!p))
> >> +               p =3D netdev_core_stats_alloc(dev);
> >> +
> >> +       if (p) {
> >> +               field =3D (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.de=
v/
>
> > 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_a=
lloc() */
> > +       struct net_device_core_stats __percpu *p =3D READ_ONCE(dev->cor=
e_stats);
> > +       unsigned long __percpu *field;
> > +
> > +       if (unlikely(!p)) {
> > +               p =3D netdev_core_stats_alloc(dev);
> > +               if (!p)
> > +                       return;
> > +       }
> > +       field =3D (__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 =3D (__force unsigned long __percpu *)((__force void *)p + offset);
this_cpu_inc(*field);

Instead of the clearly buggy code you had instead :

    field =3D (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.