Re: [RFC net-next] net: ena: Use u64_stats_t with u64_stats_sync properly

[David Yang]

On Wed, Jan 28, 2026 at 9:21 PM Arinzon, David <darinzon@xxxxxxxxxx> 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@xxxxxxxxx <mailto:mmyangfl@xxxxxxxxx>>
> > > ---
> > > 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@xxxxxxxxxxxxxx/

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.