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

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

[David Yang]

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.

Signed-off-by: David Yang <mmyangfl@gmail.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?
 .../net/ethernet/amazon/ena/ena_admin_defs.h  | 18 ++--
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 27 +++---
 drivers/net/ethernet/amazon/ena/ena_netdev.c  | 50 ++++++----
 drivers/net/ethernet/amazon/ena/ena_netdev.h  | 92 +++++++++----------
 drivers/net/ethernet/amazon/ena/ena_xdp.c     |  2 +-
 drivers/net/ethernet/amazon/ena/ena_xdp.h     |  2 +-
 6 files changed, 101 insertions(+), 90 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index 898ecd96b96a..d8a494e44f3b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -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;
 };
 
 struct ena_admin_ena_srd_stats {
 	/* Number of packets transmitted over ENA SRD */
-	u64 ena_srd_tx_pkts;
+	u64_stats_t ena_srd_tx_pkts;
 
 	/* Number of packets transmitted or could have been
 	 * transmitted over ENA SRD
 	 */
-	u64 ena_srd_eligible_tx_pkts;
+	u64_stats_t ena_srd_eligible_tx_pkts;
 
 	/* Number of packets received over ENA SRD */
-	u64 ena_srd_rx_pkts;
+	u64_stats_t ena_srd_rx_pkts;
 
 	/* Percentage of the ENA SRD resources that is in use */
-	u64 ena_srd_resource_utilization;
+	u64_stats_t ena_srd_resource_utilization;
 };
 
 /* ENA SRD Statistics Command */
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 2455d6dddc26..ff26b99bfc2f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -27,12 +27,12 @@ struct ena_hw_metrics {
 
 #define ENA_STAT_ENTRY(stat, stat_type) { \
 	.name = #stat, \
-	.stat_offset = offsetof(struct ena_stats_##stat_type, stat) / sizeof(u64) \
+	.stat_offset = offsetof(struct ena_stats_##stat_type, stat) / sizeof(u64_stats_t) \
 }
 
 #define ENA_STAT_HW_ENTRY(stat, stat_type) { \
 	.name = #stat, \
-	.stat_offset = offsetof(struct ena_admin_##stat_type, stat) / sizeof(u64) \
+	.stat_offset = offsetof(struct ena_admin_##stat_type, stat) / sizeof(u64_stats_t) \
 }
 
 #define ENA_STAT_RX_ENTRY(stat) \
@@ -154,14 +154,14 @@ static const struct ena_stats ena_stats_ena_com_strings[] = {
 #define ENA_STATS_ARRAY_ENA_SRD		ARRAY_SIZE(ena_srd_info_strings)
 #define ENA_METRICS_ARRAY_ENI		ARRAY_SIZE(ena_hw_stats_strings)
 
-static void ena_safe_update_stat(u64 *src, u64 *dst,
+static void ena_safe_update_stat(u64_stats_t *src, u64 *dst,
 				 struct u64_stats_sync *syncp)
 {
 	unsigned int start;
 
 	do {
 		start = u64_stats_fetch_begin(syncp);
-		*(dst) = *src;
+		*dst = u64_stats_read(src);
 	} while (u64_stats_fetch_retry(syncp, start));
 }
 
@@ -169,7 +169,7 @@ static void ena_metrics_stats(struct ena_adapter *adapter, u64 **data)
 {
 	struct ena_com_dev *dev = adapter->ena_dev;
 	const struct ena_stats *ena_stats;
-	u64 *ptr;
+	u64_stats_t *ptr;
 	int i;
 
 	if (ena_com_get_cap(dev, ENA_ADMIN_CUSTOMER_METRICS)) {
@@ -191,7 +191,7 @@ static void ena_metrics_stats(struct ena_adapter *adapter, u64 **data)
 		for (i = 0; i < ENA_STATS_ARRAY_ENI; i++) {
 			ena_stats = &ena_stats_eni_strings[i];
 
-			ptr = (u64 *)&adapter->eni_stats +
+			ptr = (u64_stats_t *)&adapter->eni_stats +
 				ena_stats->stat_offset;
 
 			ena_safe_update_stat(ptr, (*data)++, &adapter->syncp);
@@ -201,14 +201,14 @@ static void ena_metrics_stats(struct ena_adapter *adapter, u64 **data)
 	if (ena_com_get_cap(dev, ENA_ADMIN_ENA_SRD_INFO)) {
 		ena_com_get_ena_srd_info(dev, &adapter->ena_srd_info);
 		/* Get ENA SRD mode */
-		ptr = (u64 *)&adapter->ena_srd_info;
+		ptr = (u64_stats_t *)&adapter->ena_srd_info;
 		ena_safe_update_stat(ptr, (*data)++, &adapter->syncp);
 		for (i = 1; i < ENA_STATS_ARRAY_ENA_SRD; i++) {
 			ena_stats = &ena_srd_info_strings[i];
 			/* Wrapped within an outer struct - need to accommodate an
 			 * additional offset of the ENA SRD mode that was already processed
 			 */
-			ptr = (u64 *)&adapter->ena_srd_info +
+			ptr = (u64_stats_t *)&adapter->ena_srd_info +
 				ena_stats->stat_offset + 1;
 
 			ena_safe_update_stat(ptr, (*data)++, &adapter->syncp);
@@ -221,7 +221,7 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 	const struct ena_stats *ena_stats;
 	struct ena_ring *ring;
 
-	u64 *ptr;
+	u64_stats_t *ptr;
 	int i, j;
 
 	for (i = 0; i < adapter->num_io_queues + adapter->xdp_num_queues; i++) {
@@ -231,7 +231,8 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
 			ena_stats = &ena_stats_tx_strings[j];
 
-			ptr = (u64 *)&ring->tx_stats + ena_stats->stat_offset;
+			ptr = (u64_stats_t *)&ring->tx_stats +
+				ena_stats->stat_offset;
 
 			ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
 		}
@@ -243,7 +244,7 @@ static void ena_queue_stats(struct ena_adapter *adapter, u64 **data)
 			for (j = 0; j < ENA_STATS_ARRAY_RX; j++) {
 				ena_stats = &ena_stats_rx_strings[j];
 
-				ptr = (u64 *)&ring->rx_stats +
+				ptr = (u64_stats_t *)&ring->rx_stats +
 					ena_stats->stat_offset;
 
 				ena_safe_update_stat(ptr, (*data)++, &ring->syncp);
@@ -273,13 +274,13 @@ static void ena_get_stats(struct ena_adapter *adapter,
 			  bool hw_stats_needed)
 {
 	const struct ena_stats *ena_stats;
-	u64 *ptr;
+	u64_stats_t *ptr;
 	int i;
 
 	for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) {
 		ena_stats = &ena_stats_global_strings[i];
 
-		ptr = (u64 *)&adapter->dev_stats + ena_stats->stat_offset;
+		ptr = (u64_stats_t *)&adapter->dev_stats + ena_stats->stat_offset;
 
 		ena_safe_update_stat(ptr, data++, &adapter->syncp);
 	}
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 92d149d4f091..5f1ec7008c48 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -149,8 +149,8 @@ int ena_xmit_common(struct ena_adapter *adapter,
 	}
 
 	u64_stats_update_begin(&ring->syncp);
-	ring->tx_stats.cnt++;
-	ring->tx_stats.bytes += bytes;
+	u64_stats_inc(&ring->tx_stats.cnt);
+	u64_stats_add(&ring->tx_stats.bytes, bytes);
 	u64_stats_update_end(&ring->syncp);
 
 	tx_info->tx_descs = nb_hw_desc;
@@ -1294,9 +1294,9 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
 	work_done = budget - res_budget;
 	rx_ring->per_napi_packets += work_done;
 	u64_stats_update_begin(&rx_ring->syncp);
-	rx_ring->rx_stats.bytes += total_len;
-	rx_ring->rx_stats.cnt += work_done;
-	rx_ring->rx_stats.rx_copybreak_pkt += rx_copybreak_pkt;
+	u64_stats_add(&rx_ring->rx_stats.bytes, total_len);
+	u64_stats_add(&rx_ring->rx_stats.cnt, work_done);
+	u64_stats_add(&rx_ring->rx_stats.rx_copybreak_pkt, rx_copybreak_pkt);
 	u64_stats_update_end(&rx_ring->syncp);
 
 	rx_ring->next_to_clean = next_to_clean;
@@ -1349,15 +1349,21 @@ static void ena_adjust_adaptive_rx_intr_moderation(struct ena_napi *ena_napi)
 {
 	struct dim_sample dim_sample;
 	struct ena_ring *rx_ring = ena_napi->rx_ring;
+	u64 packets, bytes;
+	unsigned int start;
 
 	if (!rx_ring->per_napi_packets)
 		return;
 
 	rx_ring->non_empty_napi_events++;
 
-	dim_update_sample(rx_ring->non_empty_napi_events,
-			  rx_ring->rx_stats.cnt,
-			  rx_ring->rx_stats.bytes,
+	do {
+		start = u64_stats_fetch_begin(&rx_ring->syncp);
+		packets = u64_stats_read(&rx_ring->rx_stats.cnt);
+		bytes = u64_stats_read(&rx_ring->rx_stats.bytes);
+	} while (u64_stats_fetch_retry(&rx_ring->syncp, start));
+
+	dim_update_sample(rx_ring->non_empty_napi_events, packets, bytes,
 			  &dim_sample);
 
 	net_dim(&ena_napi->dim, &dim_sample);
@@ -1496,8 +1502,8 @@ static int ena_io_poll(struct napi_struct *napi, int budget)
 	}
 
 	u64_stats_update_begin(&tx_ring->syncp);
-	tx_ring->tx_stats.napi_comp += napi_comp_call;
-	tx_ring->tx_stats.tx_poll++;
+	u64_stats_add(&tx_ring->tx_stats.napi_comp, napi_comp_call);
+	u64_stats_inc(&tx_ring->tx_stats.tx_poll);
 	u64_stats_update_end(&tx_ring->syncp);
 
 	tx_ring->tx_stats.last_napi_jiffies = jiffies;
@@ -2824,8 +2830,8 @@ static void ena_get_stats64(struct net_device *netdev,
 
 		do {
 			start = u64_stats_fetch_begin(&tx_ring->syncp);
-			packets = tx_ring->tx_stats.cnt;
-			bytes = tx_ring->tx_stats.bytes;
+			packets = u64_stats_read(&tx_ring->tx_stats.cnt);
+			bytes = u64_stats_read(&tx_ring->tx_stats.bytes);
 		} while (u64_stats_fetch_retry(&tx_ring->syncp, start));
 
 		stats->tx_packets += packets;
@@ -2839,9 +2845,9 @@ static void ena_get_stats64(struct net_device *netdev,
 
 		do {
 			start = u64_stats_fetch_begin(&rx_ring->syncp);
-			packets = rx_ring->rx_stats.cnt;
-			bytes = rx_ring->rx_stats.bytes;
-			xdp_rx_drops = rx_ring->rx_stats.xdp_drop;
+			packets = u64_stats_read(&rx_ring->rx_stats.cnt);
+			bytes = u64_stats_read(&rx_ring->rx_stats.bytes);
+			xdp_rx_drops = u64_stats_read(&rx_ring->rx_stats.xdp_drop);
 		} while (u64_stats_fetch_retry(&rx_ring->syncp, start));
 
 		stats->rx_packets += packets;
@@ -2851,8 +2857,8 @@ static void ena_get_stats64(struct net_device *netdev,
 
 	do {
 		start = u64_stats_fetch_begin(&adapter->syncp);
-		rx_drops = adapter->dev_stats.rx_drops;
-		tx_drops = adapter->dev_stats.tx_drops;
+		rx_drops = u64_stats_read(&adapter->dev_stats.rx_drops);
+		tx_drops = u64_stats_read(&adapter->dev_stats.tx_drops);
 	} while (u64_stats_fetch_retry(&adapter->syncp, start));
 
 	stats->rx_dropped = rx_drops + total_xdp_rx_drops;
@@ -3380,7 +3386,11 @@ static void ena_fw_reset_device(struct work_struct *work)
 	if (likely(test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))) {
 		rc |= ena_destroy_device(adapter, false);
 		rc |= ena_restore_device(adapter);
-		adapter->dev_stats.reset_fail += !!rc;
+		if (rc) {
+			u64_stats_update_begin(&adapter->syncp);
+			u64_stats_inc(&adapter->dev_stats.reset_fail);
+			u64_stats_update_end(&adapter->syncp);
+		}
 
 		dev_err(&adapter->pdev->dev, "Device reset completed successfully\n");
 	}
@@ -4329,8 +4339,8 @@ static void ena_keep_alive_wd(void *adapter_data,
 	/* These stats are accumulated by the device, so the counters indicate
 	 * all drops since last reset.
 	 */
-	adapter->dev_stats.rx_drops = rx_drops;
-	adapter->dev_stats.tx_drops = tx_drops;
+	u64_stats_set(&adapter->dev_stats.rx_drops, rx_drops);
+	u64_stats_set(&adapter->dev_stats.tx_drops, tx_drops);
 	u64_stats_update_end(&adapter->syncp);
 }
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 006f9a3acea6..bf2e7c74d3eb 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -178,44 +178,44 @@ struct ena_rx_buffer {
 } ____cacheline_aligned;
 
 struct ena_stats_tx {
-	u64 cnt;
-	u64 bytes;
-	u64 queue_stop;
-	u64 prepare_ctx_err;
-	u64 queue_wakeup;
-	u64 dma_mapping_err;
-	u64 linearize;
-	u64 linearize_failed;
-	u64 napi_comp;
-	u64 tx_poll;
-	u64 doorbells;
-	u64 bad_req_id;
-	u64 llq_buffer_copy;
-	u64 missed_tx;
-	u64 unmask_interrupt;
+	u64_stats_t cnt;
+	u64_stats_t bytes;
+	u64_stats_t queue_stop;
+	u64_stats_t prepare_ctx_err;
+	u64_stats_t queue_wakeup;
+	u64_stats_t dma_mapping_err;
+	u64_stats_t linearize;
+	u64_stats_t linearize_failed;
+	u64_stats_t napi_comp;
+	u64_stats_t tx_poll;
+	u64_stats_t doorbells;
+	u64_stats_t bad_req_id;
+	u64_stats_t llq_buffer_copy;
+	u64_stats_t missed_tx;
+	u64_stats_t unmask_interrupt;
 	u64 last_napi_jiffies;
 };
 
 struct ena_stats_rx {
-	u64 cnt;
-	u64 bytes;
-	u64 rx_copybreak_pkt;
-	u64 csum_good;
-	u64 refil_partial;
-	u64 csum_bad;
-	u64 page_alloc_fail;
-	u64 skb_alloc_fail;
-	u64 dma_mapping_err;
-	u64 bad_desc_num;
-	u64 bad_req_id;
-	u64 empty_rx_ring;
-	u64 csum_unchecked;
-	u64 xdp_aborted;
-	u64 xdp_drop;
-	u64 xdp_pass;
-	u64 xdp_tx;
-	u64 xdp_invalid;
-	u64 xdp_redirect;
+	u64_stats_t cnt;
+	u64_stats_t bytes;
+	u64_stats_t rx_copybreak_pkt;
+	u64_stats_t csum_good;
+	u64_stats_t refil_partial;
+	u64_stats_t csum_bad;
+	u64_stats_t page_alloc_fail;
+	u64_stats_t skb_alloc_fail;
+	u64_stats_t dma_mapping_err;
+	u64_stats_t bad_desc_num;
+	u64_stats_t bad_req_id;
+	u64_stats_t empty_rx_ring;
+	u64_stats_t csum_unchecked;
+	u64_stats_t xdp_aborted;
+	u64_stats_t xdp_drop;
+	u64_stats_t xdp_pass;
+	u64_stats_t xdp_tx;
+	u64_stats_t xdp_invalid;
+	u64_stats_t xdp_redirect;
 };
 
 struct ena_ring {
@@ -284,16 +284,16 @@ struct ena_ring {
 } ____cacheline_aligned;
 
 struct ena_stats_dev {
-	u64 tx_timeout;
-	u64 suspend;
-	u64 resume;
-	u64 wd_expired;
-	u64 interface_up;
-	u64 interface_down;
-	u64 admin_q_pause;
-	u64 rx_drops;
-	u64 tx_drops;
-	u64 reset_fail;
+	u64_stats_t tx_timeout;
+	u64_stats_t suspend;
+	u64_stats_t resume;
+	u64_stats_t wd_expired;
+	u64_stats_t interface_up;
+	u64_stats_t interface_down;
+	u64_stats_t admin_q_pause;
+	u64_stats_t rx_drops;
+	u64_stats_t tx_drops;
+	u64_stats_t reset_fail;
 };
 
 enum ena_flags_t {
@@ -430,11 +430,11 @@ int handle_invalid_req_id(struct ena_ring *ring, u16 req_id,
 			  struct ena_tx_buffer *tx_info, bool is_xdp);
 
 /* Increase a stat by cnt while holding syncp seqlock on 32bit machines */
-static inline void ena_increase_stat(u64 *statp, u64 cnt,
+static inline void ena_increase_stat(u64_stats_t *statp, u64 cnt,
 				     struct u64_stats_sync *syncp)
 {
 	u64_stats_update_begin(syncp);
-	(*statp) += cnt;
+	u64_stats_add(statp, cnt);
 	u64_stats_update_end(syncp);
 }
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_xdp.c b/drivers/net/ethernet/amazon/ena/ena_xdp.c
index 5b175e7e92a1..9271d1deb96f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_xdp.c
+++ b/drivers/net/ethernet/amazon/ena/ena_xdp.c
@@ -461,7 +461,7 @@ int ena_xdp_io_poll(struct napi_struct *napi, int budget)
 	}
 
 	u64_stats_update_begin(&tx_ring->syncp);
-	tx_ring->tx_stats.tx_poll++;
+	u64_stats_inc(&tx_ring->tx_stats.tx_poll);
 	u64_stats_update_end(&tx_ring->syncp);
 	tx_ring->tx_stats.last_napi_jiffies = jiffies;
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_xdp.h b/drivers/net/ethernet/amazon/ena/ena_xdp.h
index cfd82728486a..98756a065a39 100644
--- a/drivers/net/ethernet/amazon/ena/ena_xdp.h
+++ b/drivers/net/ethernet/amazon/ena/ena_xdp.h
@@ -85,7 +85,7 @@ static inline int ena_xdp_execute(struct ena_ring *rx_ring, struct xdp_buff *xdp
 	struct bpf_prog *xdp_prog;
 	struct ena_ring *xdp_ring;
 	struct xdp_frame *xdpf;
-	u64 *xdp_stat;
+	u64_stats_t *xdp_stat;
 
 	xdp_prog = READ_ONCE(rx_ring->xdp_bpf_prog);
 
-- 
2.51.0

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

[Arinzon, David]

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.

> Signed-off-by: David Yang <mmyangfl@gmail.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?

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

[Arinzon, David]

> 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@gmail.com <mailto:mmyangfl@gmail.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?

> > @@ -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?

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@amazon.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@gmail.com <mailto:mmyangfl@gmail.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.