Received-SPF: pass (google.com: domain of linux-kernel+bounces-17702-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240105091556.15516-1-petr@tesarici.cz>
In-Reply-To: <20240105091556.15516-1-petr@tesarici.cz>
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 5 Jan 2024 10:58:42 +0100
Message-ID: <CANn89iLuYZBersxq4aH-9Fg_ojD0fh=0xtdLbRdbMrup=nvrkA@mail.gmail.com>
Subject: Re: [PATCH] net: stmmac: protect statistics updates with a spinlock
To: Petr Tesarik <petr@tesarici.cz>
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 5, 2024 at 10:16=E2=80=AFAM Petr Tesarik <petr@tesarici.cz> wro=
te:
>
> 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 stru=
ct
> u64_stats_sync must ensure mutual exclusion, or one seqcount update could
> be lost on 32-bit platforms, thus blocking readers forever.
>
> Such lockups have been actually observed on 32-bit Arm after stmmac_xmit(=
)
> on one core raced with stmmac_napi_poll_tx() on another core.
>
> Signed-off-by: Petr Tesarik <petr@tesarici.cz>

This is going to add more costs to 64bit platforms ?

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

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

Alternative would use atomic64_t fields for the ones where there is no
mutual exclusion.

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)

This leaves the fields updated from hardware interrupt context ?