Received-SPF: pass (google.com: domain of linux-kernel+bounces-17760-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 5 Jan 2024 11:34:02 +0100
From: Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To: Eric Dumazet <edumazet@google.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu
 <joabreu@synopsys.com>, "David S. Miller" <davem@davemloft.net>, Jakub
 Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Maxime
 Coquelin <mcoquelin.stm32@gmail.com>, Chen-Yu Tsai <wens@csie.org>, Jernej
 Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>,
 "open list:STMMAC ETHERNET DRIVER" <netdev@vger.kernel.org>, "moderated
 list:ARM/STM32 ARCHITECTURE" <linux-stm32@st-md-mailman.stormreply.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-arm-kernel@lists.infradead.org>, open list
 <linux-kernel@vger.kernel.org>, "open list:ARM/Allwinner sunXi SoC support"
 <linux-sunxi@lists.linux.dev>
Subject: Re: [PATCH] net: stmmac: protect statistics updates with a spinlock
Message-ID: <20240105113402.0f5f1232@meshulam.tesarici.cz>
In-Reply-To: <CANn89iLuYZBersxq4aH-9Fg_ojD0fh=0xtdLbRdbMrup=nvrkA@mail.gmail.com>
References: <20240105091556.15516-1-petr@tesarici.cz>
	<CANn89iLuYZBersxq4aH-9Fg_ojD0fh=0xtdLbRdbMrup=nvrkA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, 5 Jan 2024 10:58:42 +0100
Eric Dumazet <edumazet@google.com> wrote:

> On Fri, Jan 5, 2024 at 10:16=E2=80=AFAM Petr Tesarik <petr@tesarici.cz> w=
rote:
> >
> > Add a spinlock to fix race conditions while updating Tx/Rx statistics.
> >
> > As explained by a comment in <linux/u64_stats_sync.h>, write side of st=
ruct
> > u64_stats_sync must ensure mutual exclusion, or one seqcount update cou=
ld
> > be lost on 32-bit platforms, thus blocking readers forever.
> >
> > Such lockups have been actually observed on 32-bit Arm after stmmac_xmi=
t()
> > on one core raced with stmmac_napi_poll_tx() on another core.
> >
> > Signed-off-by: Petr Tesarik <petr@tesarici.cz> =20
>=20
> This is going to add more costs to 64bit platforms ?

Yes, it adds a (hopefully not too contended) spinlock and in most
places an interrupt disable/enable pair.

FWIW the race condition is also present on 64-bit platforms, resulting
in inaccurate statistic counters. I can understand if you consider it a
mild annoyance, not worth fixing.

> It seems to me that the same syncp can be used from two different
> threads : hard irq and napi poller...

Yes, that's exactly the scenario that locks up my system.

> At this point, I do not see why you keep linux/u64_stats_sync.h if you
> decide to go for a spinlock...

The spinlock does not havce to be taken on the reader side, so the
seqcounter still adds some value.

> Alternative would use atomic64_t fields for the ones where there is no
> mutual exclusion.
>=20
> RX : napi poll is definitely safe (protected by an atomic bit)
> TX : each TX queue is also safe (protected by an atomic exclusion for
> non LLTX drivers)
>=20
> This leaves the fields updated from hardware interrupt context ?

I'm afraid I don't have enough network-stack-foo to follow here.

My issue on 32 bit is that stmmac_xmit() may be called directly from
process context while another core runs the TX napi on the same channel
(in interrupt context). I didn't observe any race on the RX path, but I
believe it's possible with NAPI busy polling.

In any case, I don't see the connection with LLTX. Maybe you want to
say that the TX queue is safe for stmmac (because it is a non-LLTX
driver), but might not be safe for LLTX drivers?

Petr T