[Intel-wired-lan] The ice driver may rarely return incorrect statistics counter values

[Intel-wired-lan] The ice driver may rarely return incorrect statistics counter values

[Masakazu Asama]

[-- Attachment #1.1: Type: text/plain, Size: 2241 bytes --]

We have observed a very rare issue in Intel E810 environments where
SNMP-retrieved TX/RX counter values are sometimes nearly twice the actual
values.

Upon investigation, we identified a problem in the process that updates the
transmit and receive ring statistics in the ice driver. This issue occurs
when the counter update process is executed simultaneously on different CPU
cores.

I have attached a patch to fix this issue.

This patch is intended for Linux kernel 5.15 on Ubuntu 22.04, as my
environment is Ubuntu 22.04.

In my test environment, applying this patch prevents the issue from
occurring.

The function ice_update_vsi_ring_stats takes a pointer to a struct ice_vsi
as an argument. This structure is allocated on the heap and shared across
all CPU cores. The function resets the counter values to zero and then
accumulates the values from each ring of the NIC.

However, since struct ice_vsi is shared across all CPU cores, the following
race condition can occur when ice_update_vsi_ring_stats is executed
simultaneously on different CPUs:

1. Multiple CPU cores reset the counter values in struct ice_vsi to zero at
the same time.

2. Each CPU core independently increments the counter values.

As a result, the counter values may be updated to a higher-than-actual
value.

The attached patch modifies the implementation to store the counter values
on the stack, initialize them to zero, increment them with the values from
each ring, and finally update struct ice_vsi. By avoiding the use of shared
data for intermediate calculations, this fix prevents the issue.

In my environment, multiple Intel E810 NICs are bonded together.

I use Zabbix to graph the RX/TX counters of the bonding interface. However,
due to the way bonding ignores decreases in the counters of slave
interfaces, this issue makes the statistics completely unreliable.

Graphs generated from the slave interfaces may appear normal because, even
if the counter temporarily increases, it is corrected in the next
observation.

When I reported this issue to the Ubuntu bug tracking system, I was told to
get it merged upstream first.

I would like this issue to be fixed, but what should I do to get it
accepted?

Any advice would be greatly appreciated.

[-- Attachment #1.2: Type: text/html, Size: 3021 bytes --]

[-- Attachment #2: ice_update_vsi_ring_stats.patch --]
[-- Type: application/octet-stream, Size: 3410 bytes --]

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index b4c6965a8..38ba46235 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5746,10 +5746,10 @@ ice_fetch_u64_stats_per_ring(struct ice_ring *ring, u64 *pkts, u64 *bytes)
  * @count: number of rings
  */
 static void
-ice_update_vsi_tx_ring_stats(struct ice_vsi *vsi, struct ice_ring **rings,
-			     u16 count)
+ice_update_vsi_tx_ring_stats(u64 *tx_packets, u64 *tx_bytes, u64 *tx_restart,
+		u64 *tx_busy, u64 *tx_linearize, struct ice_ring **rings,
+		u16 count)
 {
-	struct rtnl_link_stats64 *vsi_stats = &vsi->net_stats;
 	u16 i;
 
 	for (i = 0; i < count; i++) {
@@ -5758,11 +5758,11 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi *vsi, struct ice_ring **rings,
 
 		ring = READ_ONCE(rings[i]);
 		ice_fetch_u64_stats_per_ring(ring, &pkts, &bytes);
-		vsi_stats->tx_packets += pkts;
-		vsi_stats->tx_bytes += bytes;
-		vsi->tx_restart += ring->tx_stats.restart_q;
-		vsi->tx_busy += ring->tx_stats.tx_busy;
-		vsi->tx_linearize += ring->tx_stats.tx_linearize;
+		*tx_packets += pkts;
+		*tx_bytes += bytes;
+		*tx_restart += ring->tx_stats.restart_q;
+		*tx_busy += ring->tx_stats.tx_busy;
+		*tx_linearize += ring->tx_stats.tx_linearize;
 	}
 }
 
@@ -5776,41 +5776,54 @@ static void ice_update_vsi_ring_stats(struct ice_vsi *vsi)
 	u64 pkts, bytes;
 	int i;
 
-	/* reset netdev stats */
-	vsi_stats->tx_packets = 0;
-	vsi_stats->tx_bytes = 0;
-	vsi_stats->rx_packets = 0;
-	vsi_stats->rx_bytes = 0;
+	u64 tx_packets = 0;
+	u64 tx_bytes = 0;
+	u64 rx_packets = 0;
+	u64 rx_bytes = 0;
 
-	/* reset non-netdev (extended) stats */
-	vsi->tx_restart = 0;
-	vsi->tx_busy = 0;
-	vsi->tx_linearize = 0;
-	vsi->rx_buf_failed = 0;
-	vsi->rx_page_failed = 0;
+	u64 tx_restart = 0;
+	u64 tx_busy = 0;
+	u64 tx_linearize = 0;
+	u64 rx_buf_failed = 0;
+	u64 rx_page_failed = 0;
 
 	rcu_read_lock();
 
 	/* update Tx rings counters */
-	ice_update_vsi_tx_ring_stats(vsi, vsi->tx_rings, vsi->num_txq);
+	ice_update_vsi_tx_ring_stats(&tx_packets, &tx_bytes, &tx_restart,
+			&tx_busy, &tx_linearize, vsi->tx_rings, vsi->num_txq);
 
 	/* update Rx rings counters */
 	ice_for_each_rxq(vsi, i) {
 		struct ice_ring *ring = READ_ONCE(vsi->rx_rings[i]);
 
 		ice_fetch_u64_stats_per_ring(ring, &pkts, &bytes);
-		vsi_stats->rx_packets += pkts;
-		vsi_stats->rx_bytes += bytes;
-		vsi->rx_buf_failed += ring->rx_stats.alloc_buf_failed;
-		vsi->rx_page_failed += ring->rx_stats.alloc_page_failed;
+		rx_packets += pkts;
+		rx_bytes += bytes;
+		rx_buf_failed += ring->rx_stats.alloc_buf_failed;
+		rx_page_failed += ring->rx_stats.alloc_page_failed;
 	}
 
 	/* update XDP Tx rings counters */
 	if (ice_is_xdp_ena_vsi(vsi))
-		ice_update_vsi_tx_ring_stats(vsi, vsi->xdp_rings,
-					     vsi->num_xdp_txq);
+		ice_update_vsi_tx_ring_stats(&tx_packets, &tx_bytes,
+				&tx_restart, &tx_busy, &tx_linearize,
+				vsi->xdp_rings, vsi->num_xdp_txq);
 
 	rcu_read_unlock();
+
+	/* reset netdev stats */
+	vsi_stats->tx_packets = tx_packets;
+	vsi_stats->tx_bytes = tx_bytes;
+	vsi_stats->rx_packets = rx_packets;
+	vsi_stats->rx_bytes = rx_bytes;
+
+	/* reset non-netdev (extended) stats */
+	vsi->tx_restart = tx_restart;
+	vsi->tx_busy = tx_busy;
+	vsi->tx_linearize = tx_linearize;
+	vsi->rx_buf_failed = rx_buf_failed;
+	vsi->rx_page_failed = rx_page_failed;
 }
 
 /**

Re: [Intel-wired-lan] The ice driver may rarely return incorrect statistics counter values

[Przemek Kitszel]

On 2/21/25 04:12, Masakazu Asama wrote:
> We have observed a very rare issue in Intel E810 environments where 
> SNMP-retrieved TX/RX counter values are sometimes nearly twice the 
> actual values.
> 
> Upon investigation, we identified a problem in the process that updates 
> the transmit and receive ring statistics in the ice driver. This issue 
> occurs when the counter update process is executed simultaneously on 
> different CPU cores.
> 
> I have attached a patch to fix this issue.
> 
> This patch is intended for Linux kernel 5.15 on Ubuntu 22.04, as my 
> environment is Ubuntu 22.04.
> 
> In my test environment, applying this patch prevents the issue from 
> occurring.
> 
> The function ice_update_vsi_ring_stats takes a pointer to a struct 
> ice_vsi as an argument. This structure is allocated on the heap and 
> shared across all CPU cores. The function resets the counter values to 
> zero and then accumulates the values from each ring of the NIC.
> 
> However, since struct ice_vsi is shared across all CPU cores, the 
> following race condition can occur when ice_update_vsi_ring_stats is 
> executed simultaneously on different CPUs:
> 
> 1.Multiple CPU cores reset the counter values in struct ice_vsi to zero 
> at the same time.
> 
> 2.Each CPU core independently increments the counter values.
> 
> As a result, the counter values may be updated to a higher-than-actual 
> value.

We had observed other problems caused by the very same shared data, it
already was fixed as part of kernel 5.16 via
commit 1a0f25a52e08 ("ice: safer stats processing").
Sadly it was not backported to 5.15.

 From your proposed patch I could tell that the fix is not present on
your Ubuntu kernel.

The first step is to check if the linked patch fixes the issue at hand,
could you please give it a try?

> 
> The attached patch modifies the implementation to store the counter 
> values on the stack, initialize them to zero, increment them with the 
> values from each ring, and finally update struct ice_vsi. By avoiding 
> the use of shared data for intermediate calculations, this fix prevents 
> the issue.
> 
> In my environment, multiple Intel E810 NICs are bonded together.
> 
> I use Zabbix to graph the RX/TX counters of the bonding interface. 
> However, due to the way bonding ignores decreases in the counters of 
> slave interfaces, this issue makes the statistics completely unreliable.
> 
> Graphs generated from the slave interfaces may appear normal because, 
> even if the counter temporarily increases, it is corrected in the next 
> observation.
> 
> When I reported this issue to the Ubuntu bug tracking system, I was told 
> to get it merged upstream first.
> 
> I would like this issue to be fixed, but what should I do to get it 
> accepted?
> 
> Any advice would be greatly appreciated.

You hit the correct mailing list for the upstream process.

Process is a bit different depending on weather we will need to just
backport Jesse's patch or parts of yours. For backports you will reach
to stable@vger.kernel.org

One more question prior to adding more patches: does the issue reproduce
with the current kernel (6.13, or even better if net-next:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git )

Re: [Intel-wired-lan] The ice driver may rarely return incorrect statistics counter values

[Masakazu Asama]

[-- Attachment #1.1: Type: text/plain, Size: 4541 bytes --]

> The first step is to check if the linked patch fixes the issue at hand,
> could you please give it a try?

Sorry for the long delay.
It took some time to complete the verification, as the environment I used
for testing is not something I can access freely. I apologize for the
inconvenience.

I have confirmed that applying the patch 1a0f25a52e08 to the Ubuntu 22.04
kernel (5.15) resolves the issue.
I’ve attached graphs showing the transmit/receive statistics before and
after applying the patch.
The data before May 30th is from before the patch was applied, and the data
after May 30th is from after the patch.

However, since kernel 5.15 still uses struct ice_ring instead of struct
ice_tx_ring, I was not able to apply the patch as-is.
I had to make two small modifications to replace struct ice_tx_ring with
struct ice_ring.

As shown above, the patch 1a0f25a52e08 appears to be effective on 5.15 as
well, so I would greatly appreciate it if you could consider backporting it.

2025年2月24日(月) 20:21 Przemek Kitszel <przemyslaw.kitszel@intel.com>:

> On 2/21/25 04:12, Masakazu Asama wrote:
> > We have observed a very rare issue in Intel E810 environments where
> > SNMP-retrieved TX/RX counter values are sometimes nearly twice the
> > actual values.
> >
> > Upon investigation, we identified a problem in the process that updates
> > the transmit and receive ring statistics in the ice driver. This issue
> > occurs when the counter update process is executed simultaneously on
> > different CPU cores.
> >
> > I have attached a patch to fix this issue.
> >
> > This patch is intended for Linux kernel 5.15 on Ubuntu 22.04, as my
> > environment is Ubuntu 22.04.
> >
> > In my test environment, applying this patch prevents the issue from
> > occurring.
> >
> > The function ice_update_vsi_ring_stats takes a pointer to a struct
> > ice_vsi as an argument. This structure is allocated on the heap and
> > shared across all CPU cores. The function resets the counter values to
> > zero and then accumulates the values from each ring of the NIC.
> >
> > However, since struct ice_vsi is shared across all CPU cores, the
> > following race condition can occur when ice_update_vsi_ring_stats is
> > executed simultaneously on different CPUs:
> >
> > 1.Multiple CPU cores reset the counter values in struct ice_vsi to zero
> > at the same time.
> >
> > 2.Each CPU core independently increments the counter values.
> >
> > As a result, the counter values may be updated to a higher-than-actual
> > value.
>
> We had observed other problems caused by the very same shared data, it
> already was fixed as part of kernel 5.16 via
> commit 1a0f25a52e08 ("ice: safer stats processing").
> Sadly it was not backported to 5.15.
>
>  From your proposed patch I could tell that the fix is not present on
> your Ubuntu kernel.
>
> The first step is to check if the linked patch fixes the issue at hand,
> could you please give it a try?
>
> >
> > The attached patch modifies the implementation to store the counter
> > values on the stack, initialize them to zero, increment them with the
> > values from each ring, and finally update struct ice_vsi. By avoiding
> > the use of shared data for intermediate calculations, this fix prevents
> > the issue.
> >
> > In my environment, multiple Intel E810 NICs are bonded together.
> >
> > I use Zabbix to graph the RX/TX counters of the bonding interface.
> > However, due to the way bonding ignores decreases in the counters of
> > slave interfaces, this issue makes the statistics completely unreliable.
> >
> > Graphs generated from the slave interfaces may appear normal because,
> > even if the counter temporarily increases, it is corrected in the next
> > observation.
> >
> > When I reported this issue to the Ubuntu bug tracking system, I was told
> > to get it merged upstream first.
> >
> > I would like this issue to be fixed, but what should I do to get it
> > accepted?
> >
> > Any advice would be greatly appreciated.
>
> You hit the correct mailing list for the upstream process.
>
> Process is a bit different depending on weather we will need to just
> backport Jesse's patch or parts of yours. For backports you will reach
> to stable@vger.kernel.org
>
> One more question prior to adding more patches: does the issue reproduce
> with the current kernel (6.13, or even better if net-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git )
>
>

[-- Attachment #1.2: Type: text/html, Size: 5457 bytes --]

[-- Attachment #2: image.png --]
[-- Type: image/png, Size: 70158 bytes --]

Re: [Intel-wired-lan] The ice driver may rarely return incorrect statistics counter values

[Przemek Kitszel]

On 7/2/25 10:01, Masakazu Asama wrote:
>  > The first step is to check if the linked patch fixes the issue at hand,
>  > could you please give it a try?
> 
> Sorry for the long delay.
> It took some time to complete the verification, as the environment I 
> used for testing is not something I can access freely. I apologize for 
> the inconvenience.
> 
> I have confirmed that applying the patch 1a0f25a52e08 to the Ubuntu 
> 22.04 kernel (5.15) resolves the issue.
> I’ve attached graphs showing the transmit/receive statistics before and 
> after applying the patch.
> The data before May 30th is from before the patch was applied, and the 
> data after May 30th is from after the patch.

thank you! it's great that this resolves the issue

> 
> However, since kernel 5.15 still uses struct ice_ring instead of struct 
> ice_tx_ring, I was not able to apply the patch as-is.
> I had to make two small modifications to replace struct ice_tx_ring with 
> struct ice_ring.

for me it was just one place to change (but there was also conflict in
need of manual resolution, so that makes it "two small modifications")

> 
> As shown above, the patch 1a0f25a52e08 appears to be effective on 5.15 
> as well, so I would greatly appreciate it if you could consider 
> backporting it.

I will send this as a patch to stable kernel 5.15, CC'ing you;
I think that Canonical will pick it up that way too