Received-SPF: pass (google.com: domain of linux-kernel+bounces-200750-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240531080739.2608969-1-jiangyunshui@kylinos.cn>
 <41e4b0e3-ecc0-43ca-a6cd-4a6beb0ceb8f@datenfreihafen.org> <20240603165543.46c7d3b4@kernel.org>
In-Reply-To: <20240603165543.46c7d3b4@kernel.org>
From: Alexander Aring <aahringo@redhat.com>
Date: Tue, 4 Jun 2024 09:52:20 -0400
Message-ID: <CAK-6q+j7vBbeB5ZPdT6szgUzYhDiPyVuadLooOywOU7M0fpfzQ@mail.gmail.com>
Subject: Re: [PATCH] net: mac802154: Fix racy device stats updates by
 DEV_STATS_INC() and DEV_STATS_ADD()
To: Jakub Kicinski <kuba@kernel.org>
Cc: Stefan Schmidt <stefan@datenfreihafen.org>, Yunshui Jiang <jiangyunshui@kylinos.cn>, 
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org, 
	linux-wpan@vger.kernel.org, alex.aring@gmail.com, miquel.raynal@bootlin.com, 
	davem@davemloft.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, Jun 3, 2024 at 7:56=E2=80=AFPM Jakub Kicinski <kuba@kernel.org> wro=
te:
>
> On Mon, 3 Jun 2024 11:33:28 +0200 Stefan Schmidt wrote:
> > Hello.
> >
> > On 31.05.24 10:07, Yunshui Jiang wrote:
> > > mac802154 devices update their dev->stats fields locklessly. Therefor=
e
> > > these counters should be updated atomically. Adopt SMP safe DEV_STATS=
_INC()
> > > and DEV_STATS_ADD() to achieve this.
> > >
> > > Signed-off-by: Yunshui Jiang <jiangyunshui@kylinos.cn>
> > > ---
> > >   net/mac802154/tx.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> > > index 2a6f1ed763c9..6fbed5bb5c3e 100644
> > > --- a/net/mac802154/tx.c
> > > +++ b/net/mac802154/tx.c
> > > @@ -34,8 +34,8 @@ void ieee802154_xmit_sync_worker(struct work_struct=
 *work)
> > >     if (res)
> > >             goto err_tx;
> > >
> > > -   dev->stats.tx_packets++;
> > > -   dev->stats.tx_bytes +=3D skb->len;
> > > +   DEV_STATS_INC(dev, tx_packets);
> > > +   DEV_STATS_ADD(dev, tx_bytes, skb->len);
> > >
> > >     ieee802154_xmit_complete(&local->hw, skb, false);
> > >
> > > @@ -90,8 +90,8 @@ ieee802154_tx(struct ieee802154_local *local, struc=
t sk_buff *skb)
> > >             if (ret)
> > >                     goto err_wake_netif_queue;
> > >
> > > -           dev->stats.tx_packets++;
> > > -           dev->stats.tx_bytes +=3D len;
> > > +           DEV_STATS_INC(dev, tx_packets);
> > > +           DEV_STATS_ADD(dev, tx_bytes, len);
> > >     } else {
> > >             local->tx_skb =3D skb;
> > >             queue_work(local->workqueue, &local->sync_tx_work);
> >
> > This patch has been applied to the wpan tree and will be
> > part of the next pull request to net. Thanks!
>
> Hi! I haven't looked in detail, but FWIW
>
> $ git grep LLTX net/mac802154/
> $
>
> and similar patch from this author has been rejected:
>
> https://lore.kernel.org/all/CANn89iLPYoOjMxNjBVHY7GwPFBGuxwRoM9gZZ-fWUUYF=
YjM1Uw@mail.gmail.com/

In the case of ieee802154_tx() yes the tx lock is held so it's like
what the mentioned link says. The workqueue is an ordered workqueue
and you either have the driver async xmit option (the preferred
option) or the driver sync xmit callback on a per driver (implies per
interface) basis.

I also don't see why there is currently a problem with the current
non-atomic way.

- Alex