Message-ID: <CAAXyoMNi3W-5VHWgr8L-gmbR2x1e3p=vsr6CdjKV8XfzLG8JsQ@mail.gmail.com>
Date: Thu, 29 Jan 2026 06:34:22 +0800
From: David Yang <mmyangfl@...il.com>
To: "Arinzon, David" <darinzon@...zon.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Allen, Neil" <shayagr@...zon.com>, 
	"Kiyanovski, Arthur" <akiyano@...zon.com>, "Bshara, Saeed" <saeedb@...zon.com>, 
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net" <davem@...emloft.net>, 
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>, 
	"pabeni@...hat.com" <pabeni@...hat.com>, "ast@...nel.org" <ast@...nel.org>, 
	"daniel@...earbox.net" <daniel@...earbox.net>, "hawk@...nel.org" <hawk@...nel.org>, 
	"john.fastabend@...il.com" <john.fastabend@...il.com>, "sdf@...ichev.me" <sdf@...ichev.me>, 
	"Bernstein, Amit" <amitbern@...zon.com>, "horms@...nel.org" <horms@...nel.org>, 
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>, "leitao@...ian.org" <leitao@...ian.org>, 
	"Enju, Kohei" <enjuk@...zon.co.jp>, "ahmed.zaki@...el.com" <ahmed.zaki@...el.com>, 
	"mingo@...nel.org" <mingo@...nel.org>, "tglx@...nel.org" <tglx@...nel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [RFC net-next] net: ena: Use u64_stats_t with u64_stats_sync properly

On Wed, Jan 28, 2026 at 9:21 PM Arinzon, David <darinzon@...zon.com> wrote:
>
> > Hi David,
> >
> >
> > Thank you for submitting the patch.
> >
> >
> > > On 64bit arches, struct u64_stats_sync is empty and provides no help
> > > against load/store tearing. Convert to u64_stats_t to ensure atomic
> > > operations.
> > >
> >
> >
> > We would like to run some quick performance tests internally before signing off.
> >
>
> We have conducted initial internal performance testing and observed no measurable impact from
> the proposed changes to the driver statistics.
>
> > > Signed-off-by: David Yang <mmyangfl@...il.com <mailto:mmyangfl@...il.com>>
> > > ---
> > > RFC Comment:
> > >
> > > The write side of u64_stats should ensure mutual exclusion, however I couldn't
> > > find the synchronization mechanism in use (for example, ena_up / ena_io_poll /
> > > ena_start_xmit). Should this be considered an issue?
> >
> >
> > We reviewed other drivers and it looks like none add extra synchronization on the write side.
> > Adding extra synchronization would degrade datapath performance to fix extremely rare missed counts.
> >
> >
> > You've submitted similar patches for other drivers but only commented on write-side
> > synchronization for ENA. Is there something specific to ENA that differs from other drivers?
>
> We're still waiting for your response to our previous question: You've submitted similar patches
> for other drivers but only commented on write-side synchronization for ENA.
> Is there something specific to ENA that differs from other drivers?
>

Hi, the series is suspended for the moment, as discuessed at
https://lore.kernel.org/all/CANn89iK9xfvQ275f+PPht8mM6K49mUq-T9D-4UUxUkTncM4tRw@mail.gmail.com/

u64_stats says: write side must ensure mutual exclusion (more
specifically, updates to the syncp), or one seqcount update could be
lost, thus blocking readers forever. I added a new u64_stats update
block in ena_fw_reset_device(), but I could not find the lock that
protects adapter->syncp, at first glance. It would be totally fine, if
all calls to u64_stats_update_begin() are actually guarded by RTNL or
something else, including all indirect uses from ena_increase_stat().

Note the issue affects 32-bit arches only, u64_stats_sync is no-op on
64-bit arches.

> > > @@ -440,42 +440,42 @@ struct ena_admin_eni_stats {
> > >     /* The number of packets shaped due to inbound aggregate BW
> > >      * allowance being exceeded
> > >      */
> > > -   u64 bw_in_allowance_exceeded;
> > > +   u64_stats_t bw_in_allowance_exceeded;
> > >
> > >     /* The number of packets shaped due to outbound aggregate BW
> > >      * allowance being exceeded
> > >      */
> > > -   u64 bw_out_allowance_exceeded;
> > > +   u64_stats_t bw_out_allowance_exceeded;
> > >
> > >     /* The number of packets shaped due to PPS allowance being exceeded */
> > > -   u64 pps_allowance_exceeded;
> > > +   u64_stats_t pps_allowance_exceeded;
> > >
> > >     /* The number of packets shaped due to connection tracking
> > >      * allowance being exceeded and leading to failure in establishment
> > >      * of new connections
> > >      */
> > > -   u64 conntrack_allowance_exceeded;
> > > +   u64_stats_t conntrack_allowance_exceeded;
> > >
> > >     /* The number of packets shaped due to linklocal packet rate
> > >      * allowance being exceeded
> > >      */
> > > -   u64 linklocal_allowance_exceeded;
> > > +   u64_stats_t linklocal_allowance_exceeded;
> > >  };
>
> Regarding the modifications to ena_admin_defs.h: these structures define the hardware-software
> interface between the host driver and the ENA device.
> The fields are specified as native u64 types to ensure consistent interpretation between the driver
> and device across different architectures.
> We're concerned that converting these interface definitions to u64_stats_t could potentially
> affect the structure layout or introduce subtle compatibility issues with the device.
> Since these definitions represent a contract with the hardware, we'd prefer to keep them
> unchanged while accepting the driver-internal statistics changes.
>
> Would you be willing to split the patch to exclude the ena_admin_defs.h changes and any
> code that touches these hardware interface fields?
>

Sorry, I'm not familiar with the details with the device. If they are
managed by the device, load/store tearing should not be possible and I
think plain unguarded read is enough. It was unconditional use of
ena_safe_update_stat() in ena_metrics_stats() that made them
suspicious. If that is the case, refining ena_metrics_stats() and/or
adding a comment about it would be better.

Powered by blists - more mailing lists