[PATCH net 0/2] net: u64_stats fixups for 32bit.

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net 0/2] net: u64_stats fixups for 32bit.
@ 2022-08-25 11:36 Sebastian Andrzej Siewior
  2022-08-25 11:36 ` [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update Sebastian Andrzej Siewior
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-08-25 11:36 UTC (permalink / raw)
  To: netdev; +Cc: Jakub Kicinski, David S. Miller, Thomas Gleixner, Peter Zijlstra

Hi,

while looking at the u64-stats patch
	https://lore.kernel.org/all/20220817162703.728679-10-bigeasy@linutronix.de

I noticed that u64_stats_fetch_begin() is used. That suspicious thing
about it is that network processing, including stats update, is
performed in NAPI and so I would expect to see
u64_stats_fetch_begin_irq() in order to avoid updates from NAPI during
the read. This is only needed on 32bit-UP where the seqcount is not
used. This is address in 2/2. The remaining user take some kind of
precaution and may use u64_stats_fetch_begin().

I updated the previously mentioned patch to get rid of
u64_stats_fetch_begin_irq(). If this is not considered stable patch
worthy then it can be ignored and considred fixed by the other series
which removes the special 32bit cases.

The xrs700x driver reads and writes the counter from preemptible context
so the only missing piece here is at least disable preemption on the
writer side to avoid preemption while the writer is in progress. The
possible reader would spin then until the writer completes its write
critical section which is considered bad. This is addressed in 1/2 by
using u64_stats_update_begin_irqsave() and so disable interrupts during
the write critical section.
The other closet resemblance I found is mdio_bus.c::mdiobus_stats_acct()
where preemtion is disabled unconditionally. This is something I want to
avoid since it also affects 64bit.

Sebastian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update
  2022-08-25 11:36 [PATCH net 0/2] net: u64_stats fixups for 32bit Sebastian Andrzej Siewior
@ 2022-08-25 11:36 ` Sebastian Andrzej Siewior
  2022-08-25 11:51   ` George McCollister
  2022-08-25 11:36 ` [PATCH net 2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch Sebastian Andrzej Siewior
  2022-08-29 12:10 ` [PATCH net 0/2] net: u64_stats fixups for 32bit patchwork-bot+netdevbpf
  2 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-08-25 11:36 UTC (permalink / raw)
  To: netdev
  Cc: Jakub Kicinski, David S. Miller, Thomas Gleixner, Peter Zijlstra,
	Sebastian Andrzej Siewior, Andrew Lunn, Florian Fainelli,
	George McCollister, Vivien Didelot, Vladimir Oltean

xrs700x_read_port_counters() updates the stats from a worker using the
u64_stats_update_begin() version. This is okay on 32-UP since on the
reader side preemption is disabled.
On 32bit-SMP the writer can be preempted by the reader at which point
the reader will spin on the seqcount until writer continues and
completes the update.

Assigning the mib_mutex mutex to the underlying seqcount would ensure
proper synchronisation. The API for that on the u64_stats_init() side
isn't available. Since it is the only user, just use disable interrupts
during the update.

Use u64_stats_update_begin_irqsave() on the writer side to ensure an
uninterrupted update.

Fixes: ee00b24f32eb8 ("net: dsa: add Arrow SpeedChips XRS700x driver")
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: George McCollister <george.mccollister@gmail.com>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/dsa/xrs700x/xrs700x.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
index 3887ed33c5fe2..fa622639d6401 100644
--- a/drivers/net/dsa/xrs700x/xrs700x.c
+++ b/drivers/net/dsa/xrs700x/xrs700x.c
@@ -109,6 +109,7 @@ static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
 {
 	struct xrs700x_port *p = &priv->ports[port];
 	struct rtnl_link_stats64 stats;
+	unsigned long flags;
 	int i;
 
 	memset(&stats, 0, sizeof(stats));
@@ -138,9 +139,9 @@ static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
 	 */
 	stats.rx_packets += stats.multicast;
 
-	u64_stats_update_begin(&p->syncp);
+	flags = u64_stats_update_begin_irqsave(&p->syncp);
 	p->stats64 = stats;
-	u64_stats_update_end(&p->syncp);
+	u64_stats_update_end_irqrestore(&p->syncp, flags);
 
 	mutex_unlock(&p->mib_mutex);
 }
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update
  2022-08-25 11:36 ` [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update Sebastian Andrzej Siewior
@ 2022-08-25 11:51   ` George McCollister
  0 siblings, 0 replies; 6+ messages in thread
From: George McCollister @ 2022-08-25 11:51 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: netdev, Jakub Kicinski, David S. Miller, Thomas Gleixner,
	Peter Zijlstra, Andrew Lunn, Florian Fainelli, Vivien Didelot,
	Vladimir Oltean

On Thu, Aug 25, 2022 at 6:36 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> xrs700x_read_port_counters() updates the stats from a worker using the
> u64_stats_update_begin() version. This is okay on 32-UP since on the
> reader side preemption is disabled.
> On 32bit-SMP the writer can be preempted by the reader at which point
> the reader will spin on the seqcount until writer continues and
> completes the update.
>
> Assigning the mib_mutex mutex to the underlying seqcount would ensure
> proper synchronisation. The API for that on the u64_stats_init() side
> isn't available. Since it is the only user, just use disable interrupts
> during the update.
>
> Use u64_stats_update_begin_irqsave() on the writer side to ensure an
> uninterrupted update.
>
> Fixes: ee00b24f32eb8 ("net: dsa: add Arrow SpeedChips XRS700x driver")
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: George McCollister <george.mccollister@gmail.com>
> Cc: Vivien Didelot <vivien.didelot@gmail.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/dsa/xrs700x/xrs700x.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
> index 3887ed33c5fe2..fa622639d6401 100644
> --- a/drivers/net/dsa/xrs700x/xrs700x.c
> +++ b/drivers/net/dsa/xrs700x/xrs700x.c
> @@ -109,6 +109,7 @@ static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
>  {
>         struct xrs700x_port *p = &priv->ports[port];
>         struct rtnl_link_stats64 stats;
> +       unsigned long flags;
>         int i;
>
>         memset(&stats, 0, sizeof(stats));
> @@ -138,9 +139,9 @@ static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
>          */
>         stats.rx_packets += stats.multicast;
>
> -       u64_stats_update_begin(&p->syncp);
> +       flags = u64_stats_update_begin_irqsave(&p->syncp);
>         p->stats64 = stats;
> -       u64_stats_update_end(&p->syncp);
> +       u64_stats_update_end_irqrestore(&p->syncp, flags);
>
>         mutex_unlock(&p->mib_mutex);
>  }
> --
> 2.37.2
>

Acked-by: George McCollister <george.mccollister@gmail.com>

Thanks,
George

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net 2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch.
  2022-08-25 11:36 [PATCH net 0/2] net: u64_stats fixups for 32bit Sebastian Andrzej Siewior
  2022-08-25 11:36 ` [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update Sebastian Andrzej Siewior
@ 2022-08-25 11:36 ` Sebastian Andrzej Siewior
  2022-08-29  9:55   ` Simon Horman
  2022-08-29 12:10 ` [PATCH net 0/2] net: u64_stats fixups for 32bit patchwork-bot+netdevbpf
  2 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-08-25 11:36 UTC (permalink / raw)
  To: netdev
  Cc: Jakub Kicinski, David S. Miller, Thomas Gleixner, Peter Zijlstra,
	Sebastian Andrzej Siewior, Catherine Sullivan, David Awogbemila,
	Dimitris Michailidis, Eric Dumazet, Hans Ulli Kroll,
	Jeroen de Borst, Johannes Berg, Linus Walleij, Paolo Abeni,
	Simon Horman, linux-arm-kernel, linux-wireless, oss-drivers,
	stable

On 32bit-UP u64_stats_fetch_begin() disables only preemption. If the
reader is in preemptible context and the writer side
(u64_stats_update_begin*()) runs in an interrupt context (IRQ or
softirq) then the writer can update the stats during the read operation.
This update remains undetected.

Use u64_stats_fetch_begin_irq() to ensure the stats fetch on 32bit-UP
are not interrupted by a writer. 32bit-SMP remains unaffected by this
change.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Catherine Sullivan <csully@google.com>
Cc: David Awogbemila <awogbemila@google.com>
Cc: Dimitris Michailidis <dmichail@fungible.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Hans Ulli Kroll <ulli.kroll@googlemail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jeroen de Borst <jeroendb@google.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <simon.horman@corigine.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: oss-drivers@corigine.com
Cc: stable@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/cortina/gemini.c         | 24 +++++++++----------
 .../ethernet/fungible/funeth/funeth_txrx.h    |  4 ++--
 drivers/net/ethernet/google/gve/gve_ethtool.c | 16 ++++++-------
 drivers/net/ethernet/google/gve/gve_main.c    | 12 +++++-----
 drivers/net/ethernet/huawei/hinic/hinic_rx.c  |  4 ++--
 drivers/net/ethernet/huawei/hinic/hinic_tx.c  |  4 ++--
 .../ethernet/netronome/nfp/nfp_net_common.c   |  8 +++----
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  |  8 +++----
 drivers/net/netdevsim/netdev.c                |  4 ++--
 net/mac80211/sta_info.c                       |  8 +++----
 net/mpls/af_mpls.c                            |  4 ++--
 11 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 9e6de2f968fa3..6dae768671e3d 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1919,7 +1919,7 @@ static void gmac_get_stats64(struct net_device *netdev,
 
 	/* Racing with RX NAPI */
 	do {
-		start = u64_stats_fetch_begin(&port->rx_stats_syncp);
+		start = u64_stats_fetch_begin_irq(&port->rx_stats_syncp);
 
 		stats->rx_packets = port->stats.rx_packets;
 		stats->rx_bytes = port->stats.rx_bytes;
@@ -1931,11 +1931,11 @@ static void gmac_get_stats64(struct net_device *netdev,
 		stats->rx_crc_errors = port->stats.rx_crc_errors;
 		stats->rx_frame_errors = port->stats.rx_frame_errors;
 
-	} while (u64_stats_fetch_retry(&port->rx_stats_syncp, start));
+	} while (u64_stats_fetch_retry_irq(&port->rx_stats_syncp, start));
 
 	/* Racing with MIB and TX completion interrupts */
 	do {
-		start = u64_stats_fetch_begin(&port->ir_stats_syncp);
+		start = u64_stats_fetch_begin_irq(&port->ir_stats_syncp);
 
 		stats->tx_errors = port->stats.tx_errors;
 		stats->tx_packets = port->stats.tx_packets;
@@ -1945,15 +1945,15 @@ static void gmac_get_stats64(struct net_device *netdev,
 		stats->rx_missed_errors = port->stats.rx_missed_errors;
 		stats->rx_fifo_errors = port->stats.rx_fifo_errors;
 
-	} while (u64_stats_fetch_retry(&port->ir_stats_syncp, start));
+	} while (u64_stats_fetch_retry_irq(&port->ir_stats_syncp, start));
 
 	/* Racing with hard_start_xmit */
 	do {
-		start = u64_stats_fetch_begin(&port->tx_stats_syncp);
+		start = u64_stats_fetch_begin_irq(&port->tx_stats_syncp);
 
 		stats->tx_dropped = port->stats.tx_dropped;
 
-	} while (u64_stats_fetch_retry(&port->tx_stats_syncp, start));
+	} while (u64_stats_fetch_retry_irq(&port->tx_stats_syncp, start));
 
 	stats->rx_dropped += stats->rx_missed_errors;
 }
@@ -2031,18 +2031,18 @@ static void gmac_get_ethtool_stats(struct net_device *netdev,
 	/* Racing with MIB interrupt */
 	do {
 		p = values;
-		start = u64_stats_fetch_begin(&port->ir_stats_syncp);
+		start = u64_stats_fetch_begin_irq(&port->ir_stats_syncp);
 
 		for (i = 0; i < RX_STATS_NUM; i++)
 			*p++ = port->hw_stats[i];
 
-	} while (u64_stats_fetch_retry(&port->ir_stats_syncp, start));
+	} while (u64_stats_fetch_retry_irq(&port->ir_stats_syncp, start));
 	values = p;
 
 	/* Racing with RX NAPI */
 	do {
 		p = values;
-		start = u64_stats_fetch_begin(&port->rx_stats_syncp);
+		start = u64_stats_fetch_begin_irq(&port->rx_stats_syncp);
 
 		for (i = 0; i < RX_STATUS_NUM; i++)
 			*p++ = port->rx_stats[i];
@@ -2050,13 +2050,13 @@ static void gmac_get_ethtool_stats(struct net_device *netdev,
 			*p++ = port->rx_csum_stats[i];
 		*p++ = port->rx_napi_exits;
 
-	} while (u64_stats_fetch_retry(&port->rx_stats_syncp, start));
+	} while (u64_stats_fetch_retry_irq(&port->rx_stats_syncp, start));
 	values = p;
 
 	/* Racing with TX start_xmit */
 	do {
 		p = values;
-		start = u64_stats_fetch_begin(&port->tx_stats_syncp);
+		start = u64_stats_fetch_begin_irq(&port->tx_stats_syncp);
 
 		for (i = 0; i < TX_MAX_FRAGS; i++) {
 			*values++ = port->tx_frag_stats[i];
@@ -2065,7 +2065,7 @@ static void gmac_get_ethtool_stats(struct net_device *netdev,
 		*values++ = port->tx_frags_linearized;
 		*values++ = port->tx_hw_csummed;
 
-	} while (u64_stats_fetch_retry(&port->tx_stats_syncp, start));
+	} while (u64_stats_fetch_retry_irq(&port->tx_stats_syncp, start));
 }
 
 static int gmac_get_ksettings(struct net_device *netdev,
diff --git a/drivers/net/ethernet/fungible/funeth/funeth_txrx.h b/drivers/net/ethernet/fungible/funeth/funeth_txrx.h
index 53b7e95213a85..671f51135c269 100644
--- a/drivers/net/ethernet/fungible/funeth/funeth_txrx.h
+++ b/drivers/net/ethernet/fungible/funeth/funeth_txrx.h
@@ -206,9 +206,9 @@ struct funeth_rxq {
 
 #define FUN_QSTAT_READ(q, seq, stats_copy) \
 	do { \
-		seq = u64_stats_fetch_begin(&(q)->syncp); \
+		seq = u64_stats_fetch_begin_irq(&(q)->syncp); \
 		stats_copy = (q)->stats; \
-	} while (u64_stats_fetch_retry(&(q)->syncp, (seq)))
+	} while (u64_stats_fetch_retry_irq(&(q)->syncp, (seq)))
 
 #define FUN_INT_NAME_LEN (IFNAMSIZ + 16)
 
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 50b384910c839..7b9a2d9d96243 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -177,14 +177,14 @@ gve_get_ethtool_stats(struct net_device *netdev,
 				struct gve_rx_ring *rx = &priv->rx[ring];
 
 				start =
-				  u64_stats_fetch_begin(&priv->rx[ring].statss);
+				  u64_stats_fetch_begin_irq(&priv->rx[ring].statss);
 				tmp_rx_pkts = rx->rpackets;
 				tmp_rx_bytes = rx->rbytes;
 				tmp_rx_skb_alloc_fail = rx->rx_skb_alloc_fail;
 				tmp_rx_buf_alloc_fail = rx->rx_buf_alloc_fail;
 				tmp_rx_desc_err_dropped_pkt =
 					rx->rx_desc_err_dropped_pkt;
-			} while (u64_stats_fetch_retry(&priv->rx[ring].statss,
+			} while (u64_stats_fetch_retry_irq(&priv->rx[ring].statss,
 						       start));
 			rx_pkts += tmp_rx_pkts;
 			rx_bytes += tmp_rx_bytes;
@@ -198,10 +198,10 @@ gve_get_ethtool_stats(struct net_device *netdev,
 		if (priv->tx) {
 			do {
 				start =
-				  u64_stats_fetch_begin(&priv->tx[ring].statss);
+				  u64_stats_fetch_begin_irq(&priv->tx[ring].statss);
 				tmp_tx_pkts = priv->tx[ring].pkt_done;
 				tmp_tx_bytes = priv->tx[ring].bytes_done;
-			} while (u64_stats_fetch_retry(&priv->tx[ring].statss,
+			} while (u64_stats_fetch_retry_irq(&priv->tx[ring].statss,
 						       start));
 			tx_pkts += tmp_tx_pkts;
 			tx_bytes += tmp_tx_bytes;
@@ -259,13 +259,13 @@ gve_get_ethtool_stats(struct net_device *netdev,
 			data[i++] = rx->fill_cnt - rx->cnt;
 			do {
 				start =
-				  u64_stats_fetch_begin(&priv->rx[ring].statss);
+				  u64_stats_fetch_begin_irq(&priv->rx[ring].statss);
 				tmp_rx_bytes = rx->rbytes;
 				tmp_rx_skb_alloc_fail = rx->rx_skb_alloc_fail;
 				tmp_rx_buf_alloc_fail = rx->rx_buf_alloc_fail;
 				tmp_rx_desc_err_dropped_pkt =
 					rx->rx_desc_err_dropped_pkt;
-			} while (u64_stats_fetch_retry(&priv->rx[ring].statss,
+			} while (u64_stats_fetch_retry_irq(&priv->rx[ring].statss,
 						       start));
 			data[i++] = tmp_rx_bytes;
 			data[i++] = rx->rx_cont_packet_cnt;
@@ -331,9 +331,9 @@ gve_get_ethtool_stats(struct net_device *netdev,
 			}
 			do {
 				start =
-				  u64_stats_fetch_begin(&priv->tx[ring].statss);
+				  u64_stats_fetch_begin_irq(&priv->tx[ring].statss);
 				tmp_tx_bytes = tx->bytes_done;
-			} while (u64_stats_fetch_retry(&priv->tx[ring].statss,
+			} while (u64_stats_fetch_retry_irq(&priv->tx[ring].statss,
 						       start));
 			data[i++] = tmp_tx_bytes;
 			data[i++] = tx->wake_queue;
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 6cafee55efc32..044db3ebb071c 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -51,10 +51,10 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
 		for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
 			do {
 				start =
-				  u64_stats_fetch_begin(&priv->rx[ring].statss);
+				  u64_stats_fetch_begin_irq(&priv->rx[ring].statss);
 				packets = priv->rx[ring].rpackets;
 				bytes = priv->rx[ring].rbytes;
-			} while (u64_stats_fetch_retry(&priv->rx[ring].statss,
+			} while (u64_stats_fetch_retry_irq(&priv->rx[ring].statss,
 						       start));
 			s->rx_packets += packets;
 			s->rx_bytes += bytes;
@@ -64,10 +64,10 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
 		for (ring = 0; ring < priv->tx_cfg.num_queues; ring++) {
 			do {
 				start =
-				  u64_stats_fetch_begin(&priv->tx[ring].statss);
+				  u64_stats_fetch_begin_irq(&priv->tx[ring].statss);
 				packets = priv->tx[ring].pkt_done;
 				bytes = priv->tx[ring].bytes_done;
-			} while (u64_stats_fetch_retry(&priv->tx[ring].statss,
+			} while (u64_stats_fetch_retry_irq(&priv->tx[ring].statss,
 						       start));
 			s->tx_packets += packets;
 			s->tx_bytes += bytes;
@@ -1274,9 +1274,9 @@ void gve_handle_report_stats(struct gve_priv *priv)
 			}
 
 			do {
-				start = u64_stats_fetch_begin(&priv->tx[idx].statss);
+				start = u64_stats_fetch_begin_irq(&priv->tx[idx].statss);
 				tx_bytes = priv->tx[idx].bytes_done;
-			} while (u64_stats_fetch_retry(&priv->tx[idx].statss, start));
+			} while (u64_stats_fetch_retry_irq(&priv->tx[idx].statss, start));
 			stats[stats_idx++] = (struct stats) {
 				.stat_name = cpu_to_be32(TX_WAKE_CNT),
 				.value = cpu_to_be64(priv->tx[idx].wake_queue),
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
index a866bea651103..e5828a658caf4 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
@@ -74,14 +74,14 @@ void hinic_rxq_get_stats(struct hinic_rxq *rxq, struct hinic_rxq_stats *stats)
 	unsigned int start;
 
 	do {
-		start = u64_stats_fetch_begin(&rxq_stats->syncp);
+		start = u64_stats_fetch_begin_irq(&rxq_stats->syncp);
 		stats->pkts = rxq_stats->pkts;
 		stats->bytes = rxq_stats->bytes;
 		stats->errors = rxq_stats->csum_errors +
 				rxq_stats->other_errors;
 		stats->csum_errors = rxq_stats->csum_errors;
 		stats->other_errors = rxq_stats->other_errors;
-	} while (u64_stats_fetch_retry(&rxq_stats->syncp, start));
+	} while (u64_stats_fetch_retry_irq(&rxq_stats->syncp, start));
 }
 
 /**
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_tx.c b/drivers/net/ethernet/huawei/hinic/hinic_tx.c
index 5051cdff2384b..3b6c7b5857376 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_tx.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_tx.c
@@ -99,14 +99,14 @@ void hinic_txq_get_stats(struct hinic_txq *txq, struct hinic_txq_stats *stats)
 	unsigned int start;
 
 	do {
-		start = u64_stats_fetch_begin(&txq_stats->syncp);
+		start = u64_stats_fetch_begin_irq(&txq_stats->syncp);
 		stats->pkts    = txq_stats->pkts;
 		stats->bytes   = txq_stats->bytes;
 		stats->tx_busy = txq_stats->tx_busy;
 		stats->tx_wake = txq_stats->tx_wake;
 		stats->tx_dropped = txq_stats->tx_dropped;
 		stats->big_frags_pkts = txq_stats->big_frags_pkts;
-	} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
+	} while (u64_stats_fetch_retry_irq(&txq_stats->syncp, start));
 }
 
 /**
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index cf4d6f1129fa2..349a2b1a19a24 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -1630,21 +1630,21 @@ static void nfp_net_stat64(struct net_device *netdev,
 		unsigned int start;
 
 		do {
-			start = u64_stats_fetch_begin(&r_vec->rx_sync);
+			start = u64_stats_fetch_begin_irq(&r_vec->rx_sync);
 			data[0] = r_vec->rx_pkts;
 			data[1] = r_vec->rx_bytes;
 			data[2] = r_vec->rx_drops;
-		} while (u64_stats_fetch_retry(&r_vec->rx_sync, start));
+		} while (u64_stats_fetch_retry_irq(&r_vec->rx_sync, start));
 		stats->rx_packets += data[0];
 		stats->rx_bytes += data[1];
 		stats->rx_dropped += data[2];
 
 		do {
-			start = u64_stats_fetch_begin(&r_vec->tx_sync);
+			start = u64_stats_fetch_begin_irq(&r_vec->tx_sync);
 			data[0] = r_vec->tx_pkts;
 			data[1] = r_vec->tx_bytes;
 			data[2] = r_vec->tx_errors;
-		} while (u64_stats_fetch_retry(&r_vec->tx_sync, start));
+		} while (u64_stats_fetch_retry_irq(&r_vec->tx_sync, start));
 		stats->tx_packets += data[0];
 		stats->tx_bytes += data[1];
 		stats->tx_errors += data[2];
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index eeb1455a4e5db..b1b1b648e40cb 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -649,7 +649,7 @@ static u64 *nfp_vnic_get_sw_stats(struct net_device *netdev, u64 *data)
 		unsigned int start;
 
 		do {
-			start = u64_stats_fetch_begin(&nn->r_vecs[i].rx_sync);
+			start = u64_stats_fetch_begin_irq(&nn->r_vecs[i].rx_sync);
 			data[0] = nn->r_vecs[i].rx_pkts;
 			tmp[0] = nn->r_vecs[i].hw_csum_rx_ok;
 			tmp[1] = nn->r_vecs[i].hw_csum_rx_inner_ok;
@@ -657,10 +657,10 @@ static u64 *nfp_vnic_get_sw_stats(struct net_device *netdev, u64 *data)
 			tmp[3] = nn->r_vecs[i].hw_csum_rx_error;
 			tmp[4] = nn->r_vecs[i].rx_replace_buf_alloc_fail;
 			tmp[5] = nn->r_vecs[i].hw_tls_rx;
-		} while (u64_stats_fetch_retry(&nn->r_vecs[i].rx_sync, start));
+		} while (u64_stats_fetch_retry_irq(&nn->r_vecs[i].rx_sync, start));
 
 		do {
-			start = u64_stats_fetch_begin(&nn->r_vecs[i].tx_sync);
+			start = u64_stats_fetch_begin_irq(&nn->r_vecs[i].tx_sync);
 			data[1] = nn->r_vecs[i].tx_pkts;
 			data[2] = nn->r_vecs[i].tx_busy;
 			tmp[6] = nn->r_vecs[i].hw_csum_tx;
@@ -670,7 +670,7 @@ static u64 *nfp_vnic_get_sw_stats(struct net_device *netdev, u64 *data)
 			tmp[10] = nn->r_vecs[i].hw_tls_tx;
 			tmp[11] = nn->r_vecs[i].tls_tx_fallback;
 			tmp[12] = nn->r_vecs[i].tls_tx_no_fallback;
-		} while (u64_stats_fetch_retry(&nn->r_vecs[i].tx_sync, start));
+		} while (u64_stats_fetch_retry_irq(&nn->r_vecs[i].tx_sync, start));
 
 		data += NN_RVEC_PER_Q_STATS;
 
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index e470e3398abc2..9a1a5b2036240 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -67,10 +67,10 @@ nsim_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	unsigned int start;
 
 	do {
-		start = u64_stats_fetch_begin(&ns->syncp);
+		start = u64_stats_fetch_begin_irq(&ns->syncp);
 		stats->tx_bytes = ns->tx_bytes;
 		stats->tx_packets = ns->tx_packets;
-	} while (u64_stats_fetch_retry(&ns->syncp, start));
+	} while (u64_stats_fetch_retry_irq(&ns->syncp, start));
 }
 
 static int
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index cb23da9aff1e6..3bcb0a1767fd2 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2316,9 +2316,9 @@ static inline u64 sta_get_tidstats_msdu(struct ieee80211_sta_rx_stats *rxstats,
 	u64 value;
 
 	do {
-		start = u64_stats_fetch_begin(&rxstats->syncp);
+		start = u64_stats_fetch_begin_irq(&rxstats->syncp);
 		value = rxstats->msdu[tid];
-	} while (u64_stats_fetch_retry(&rxstats->syncp, start));
+	} while (u64_stats_fetch_retry_irq(&rxstats->syncp, start));
 
 	return value;
 }
@@ -2384,9 +2384,9 @@ static inline u64 sta_get_stats_bytes(struct ieee80211_sta_rx_stats *rxstats)
 	u64 value;
 
 	do {
-		start = u64_stats_fetch_begin(&rxstats->syncp);
+		start = u64_stats_fetch_begin_irq(&rxstats->syncp);
 		value = rxstats->bytes;
-	} while (u64_stats_fetch_retry(&rxstats->syncp, start));
+	} while (u64_stats_fetch_retry_irq(&rxstats->syncp, start));
 
 	return value;
 }
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 35b5f806fdda1..b52afe316dc41 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1079,9 +1079,9 @@ static void mpls_get_stats(struct mpls_dev *mdev,
 
 		p = per_cpu_ptr(mdev->stats, i);
 		do {
-			start = u64_stats_fetch_begin(&p->syncp);
+			start = u64_stats_fetch_begin_irq(&p->syncp);
 			local = p->stats;
-		} while (u64_stats_fetch_retry(&p->syncp, start));
+		} while (u64_stats_fetch_retry_irq(&p->syncp, start));
 
 		stats->rx_packets	+= local.rx_packets;
 		stats->rx_bytes		+= local.rx_bytes;
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net 2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch.
  2022-08-25 11:36 ` [PATCH net 2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch Sebastian Andrzej Siewior
@ 2022-08-29  9:55   ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2022-08-29  9:55 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: netdev, Jakub Kicinski, David S. Miller, Thomas Gleixner,
	Peter Zijlstra, Catherine Sullivan, David Awogbemila,
	Dimitris Michailidis, Eric Dumazet, Hans Ulli Kroll,
	Jeroen de Borst, Johannes Berg, Linus Walleij, Paolo Abeni,
	linux-arm-kernel, linux-wireless, oss-drivers, stable

On Thu, Aug 25, 2022 at 01:36:45PM +0200, Sebastian Andrzej Siewior wrote:
> On 32bit-UP u64_stats_fetch_begin() disables only preemption. If the
> reader is in preemptible context and the writer side
> (u64_stats_update_begin*()) runs in an interrupt context (IRQ or
> softirq) then the writer can update the stats during the read operation.
> This update remains undetected.
> 
> Use u64_stats_fetch_begin_irq() to ensure the stats fetch on 32bit-UP
> are not interrupted by a writer. 32bit-SMP remains unaffected by this
> change.

Thanks,

for the NFP driver portion:

Reviewed-by: Simon Horman <simon.horman@corigine.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net 0/2] net: u64_stats fixups for 32bit.
  2022-08-25 11:36 [PATCH net 0/2] net: u64_stats fixups for 32bit Sebastian Andrzej Siewior
  2022-08-25 11:36 ` [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update Sebastian Andrzej Siewior
  2022-08-25 11:36 ` [PATCH net 2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch Sebastian Andrzej Siewior
@ 2022-08-29 12:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-08-29 12:10 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: netdev, kuba, davem, tglx, peterz

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 25 Aug 2022 13:36:43 +0200 you wrote:
> Hi,
> 
> while looking at the u64-stats patch
> 	https://lore.kernel.org/all/20220817162703.728679-10-bigeasy@linutronix.de
> 
> I noticed that u64_stats_fetch_begin() is used. That suspicious thing
> about it is that network processing, including stats update, is
> performed in NAPI and so I would expect to see
> u64_stats_fetch_begin_irq() in order to avoid updates from NAPI during
> the read. This is only needed on 32bit-UP where the seqcount is not
> used. This is address in 2/2. The remaining user take some kind of
> precaution and may use u64_stats_fetch_begin().
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update
    https://git.kernel.org/netdev/net/c/3f8ae9fe0409
  - [net,2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch.
    https://git.kernel.org/netdev/net/c/278d3ba61563

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-08-29 12:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-25 11:36 [PATCH net 0/2] net: u64_stats fixups for 32bit Sebastian Andrzej Siewior
2022-08-25 11:36 ` [PATCH net 1/2] net: dsa: xrs700x: Use irqsave variant for u64 stats update Sebastian Andrzej Siewior
2022-08-25 11:51   ` George McCollister
2022-08-25 11:36 ` [PATCH net 2/2] net: Use u64_stats_fetch_begin_irq() for stats fetch Sebastian Andrzej Siewior
2022-08-29  9:55   ` Simon Horman
2022-08-29 12:10 ` [PATCH net 0/2] net: u64_stats fixups for 32bit patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).