[PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

[PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

[alexey.pakhunov]

From: Alex Pakhunov <alexey.pakhunov@spacex.com>

This change moves [rt]x_dropped counters to tg3_napi so that they can be
updated by a single writer, race-free.

Signed-off-by: Alex Pakhunov <alexey.pakhunov@spacex.com>
Signed-off-by: Vincent Wong <vincent.wong2@spacex.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 39 +++++++++++++++++++++++++----
 drivers/net/ethernet/broadcom/tg3.h |  4 +--
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 14b311196b8f..de74a63a02dd 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -6845,7 +6845,7 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
 				       desc_idx, *post_ptr);
 		drop_it_no_recycle:
 			/* Other statistics kept track of by card. */
-			tp->rx_dropped++;
+			tnapi->rx_dropped++;
 			goto next_pkt;
 		}
 
@@ -8146,7 +8146,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
 drop:
 	dev_kfree_skb_any(skb);
 drop_nofree:
-	tp->tx_dropped++;
+	tnapi->tx_dropped++;
 	return NETDEV_TX_OK;
 }
 
@@ -9325,7 +9325,7 @@ static void __tg3_set_rx_mode(struct net_device *);
 /* tp->lock is held. */
 static int tg3_halt(struct tg3 *tp, int kind, bool silent)
 {
-	int err;
+	int err, i;
 
 	tg3_stop_fw(tp);
 
@@ -9346,6 +9346,14 @@ static int tg3_halt(struct tg3 *tp, int kind, bool silent)
 
 		/* And make sure the next sample is new data */
 		memset(tp->hw_stats, 0, sizeof(struct tg3_hw_stats));
+
+		for (i = 0; i < TG3_IRQ_MAX_VECS; ++i)
+		{
+			struct tg3_napi *tnapi = &tp->napi[i];
+
+			tnapi->rx_dropped = 0;
+			tnapi->tx_dropped = 0;
+		}
 	}
 
 	return err;
@@ -11895,6 +11903,9 @@ static void tg3_get_nstats(struct tg3 *tp, struct rtnl_link_stats64 *stats)
 {
 	struct rtnl_link_stats64 *old_stats = &tp->net_stats_prev;
 	struct tg3_hw_stats *hw_stats = tp->hw_stats;
+	unsigned long rx_dropped;
+	unsigned long tx_dropped;
+	int i;
 
 	stats->rx_packets = old_stats->rx_packets +
 		get_stat64(&hw_stats->rx_ucast_packets) +
@@ -11941,8 +11952,26 @@ static void tg3_get_nstats(struct tg3 *tp, struct rtnl_link_stats64 *stats)
 	stats->rx_missed_errors = old_stats->rx_missed_errors +
 		get_stat64(&hw_stats->rx_discards);
 
-	stats->rx_dropped = tp->rx_dropped;
-	stats->tx_dropped = tp->tx_dropped;
+	/* Aggregate per-queue counters. The per-queue counters are updated
+	 * by a single writer, race-free. The result computed by this loop
+	 * might not be 100% accurate (counters can be updated in the middle of
+	 * the loop) but the next tg3_get_nstats() will recompute the current
+	 * value so it is acceptable.
+	 *
+	 * Note that these counters wrap around at 4G on 32bit machines.
+	 */
+	rx_dropped = (unsigned long)(old_stats->rx_dropped);
+	tx_dropped = (unsigned long)(old_stats->tx_dropped);
+
+	for (i = 0; i < tp->irq_cnt; i++) {
+		struct tg3_napi *tnapi = &tp->napi[i];
+
+		rx_dropped += tnapi->rx_dropped;
+		tx_dropped += tnapi->tx_dropped;
+	}
+
+	stats->rx_dropped = rx_dropped;
+	stats->tx_dropped = tx_dropped;
 }
 
 static int tg3_get_regs_len(struct net_device *dev)
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index 1000c894064f..8d753f8c5b06 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -3018,6 +3018,7 @@ struct tg3_napi {
 	u16				*rx_rcb_prod_idx;
 	struct tg3_rx_prodring_set	prodring;
 	struct tg3_rx_buffer_desc	*rx_rcb;
+	unsigned long			rx_dropped;
 
 	u32				tx_prod	____cacheline_aligned;
 	u32				tx_cons;
@@ -3026,6 +3027,7 @@ struct tg3_napi {
 	u32				prodmbox;
 	struct tg3_tx_buffer_desc	*tx_ring;
 	struct tg3_tx_ring_info		*tx_buffers;
+	unsigned long			tx_dropped;
 
 	dma_addr_t			status_mapping;
 	dma_addr_t			rx_rcb_mapping;
@@ -3219,8 +3221,6 @@ struct tg3 {
 
 
 	/* begin "everything else" cacheline(s) section */
-	unsigned long			rx_dropped;
-	unsigned long			tx_dropped;
 	struct rtnl_link_stats64	net_stats_prev;
 	struct tg3_ethtool_stats	estats_prev;
 

base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
-- 
2.39.3

[PATCH v2 2/2] tg3: Increment tx_dropped in tg3_tso_bug()

[alexey.pakhunov]

From: Alex Pakhunov <alexey.pakhunov@spacex.com>

tg3_tso_bug() drops a packet if it cannot be segmented for any reason.
The number of discarded frames should be incremented accordingly.

Signed-off-by: Alex Pakhunov <alexey.pakhunov@spacex.com>
Signed-off-by: Vincent Wong <vincent.wong2@spacex.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index de74a63a02dd..a71df37d78bf 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7874,8 +7874,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
 
 	segs = skb_gso_segment(skb, tp->dev->features &
 				    ~(NETIF_F_TSO | NETIF_F_TSO6));
-	if (IS_ERR(segs) || !segs)
+	if (IS_ERR(segs) || !segs) {
+		tnapi->tx_dropped++;
 		goto tg3_tso_bug_end;
+	}
 
 	skb_list_walk_safe(segs, seg, next) {
 		skb_mark_not_on_list(seg);
-- 
2.39.3

Re: [PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Thu, Nov 9, 2023 at 4:24 PM <alexey.pakhunov@spacex.com> wrote:
>
> From: Alex Pakhunov <alexey.pakhunov@spacex.com>
>
> This change moves [rt]x_dropped counters to tg3_napi so that they can be
> updated by a single writer, race-free.
>
> Signed-off-by: Alex Pakhunov <alexey.pakhunov@spacex.com>
> Signed-off-by: Vincent Wong <vincent.wong2@spacex.com>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH v2 2/2] tg3: Increment tx_dropped in tg3_tso_bug()

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Thu, Nov 9, 2023 at 4:24 PM <alexey.pakhunov@spacex.com> wrote:
>
> From: Alex Pakhunov <alexey.pakhunov@spacex.com>
>
> tg3_tso_bug() drops a packet if it cannot be segmented for any reason.
> The number of discarded frames should be incremented accordingly.
>
> Signed-off-by: Alex Pakhunov <alexey.pakhunov@spacex.com>
> Signed-off-by: Vincent Wong <vincent.wong2@spacex.com>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

[Andrew Lunn]

> @@ -11895,6 +11903,9 @@ static void tg3_get_nstats(struct tg3 *tp, struct rtnl_link_stats64 *stats)
>  {
>  	struct rtnl_link_stats64 *old_stats = &tp->net_stats_prev;
>  	struct tg3_hw_stats *hw_stats = tp->hw_stats;
> +	unsigned long rx_dropped;
> +	unsigned long tx_dropped;
> +	int i;
>  
>  	stats->rx_packets = old_stats->rx_packets +
>  		get_stat64(&hw_stats->rx_ucast_packets) +
> @@ -11941,8 +11952,26 @@ static void tg3_get_nstats(struct tg3 *tp, struct rtnl_link_stats64 *stats)
>  	stats->rx_missed_errors = old_stats->rx_missed_errors +
>  		get_stat64(&hw_stats->rx_discards);
>  
> -	stats->rx_dropped = tp->rx_dropped;
> -	stats->tx_dropped = tp->tx_dropped;
> +	/* Aggregate per-queue counters. The per-queue counters are updated
> +	 * by a single writer, race-free. The result computed by this loop
> +	 * might not be 100% accurate (counters can be updated in the middle of
> +	 * the loop) but the next tg3_get_nstats() will recompute the current
> +	 * value so it is acceptable.
> +	 *
> +	 * Note that these counters wrap around at 4G on 32bit machines.
> +	 */
> +	rx_dropped = (unsigned long)(old_stats->rx_dropped);
> +	tx_dropped = (unsigned long)(old_stats->tx_dropped);

Isn't this wrapping artificial? old_stats is of type
rtnl_link_stats64, so the counters are 64 bit.

> +
> +	for (i = 0; i < tp->irq_cnt; i++) {
> +		struct tg3_napi *tnapi = &tp->napi[i];
> +
> +		rx_dropped += tnapi->rx_dropped;
> +		tx_dropped += tnapi->tx_dropped;
> +	}
> +
> +	stats->rx_dropped = rx_dropped;
> +	stats->tx_dropped = tx_dropped;

state is also rtnl_link_stats64 so has 64 bit counters. So this code
is throwing away the upper 32 bits.

Why not use include/linux/u64_stats_sync.h, which should cost you
nothing on 64 bit machines, and do the right thing on 32 bit machines.

	Andrew

Re: [PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

[Jakub Kicinski]

On Thu, 9 Nov 2023 16:23:39 -0800 alexey.pakhunov@spacex.com wrote:
> +		for (i = 0; i < TG3_IRQ_MAX_VECS; ++i)
> +		{

checkpatch says:

ERROR: that open brace { should be on the previous line 
-- 
pw-bot: cr

Re: [PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

[Alex Pakhunov]

Hi,

> Isn't this wrapping artificial? old_stats is of type
> rtnl_link_stats64, so the counters are 64 bit.

The wrapping here is needed as long as tnapi->[rt]x_dropped counters are 32
bit wide. It makes sure the resulting value is correctly wrapped.
tnapi->[rt]x_dropped counters are 32 bit on 32 bit systems to make sure
they can be read atomically.

> Why not use include/linux/u64_stats_sync.h, which should cost you
> nothing on 64 bit machines, and do the right thing on 32 bit machines.

It should be possible to use include/linux/u64_stats_sync.h but it seems
like overkill in this case. First, we mostly care whether the counters are
not zero and/or incremening. We typically don't care about the exact value.
Second, the counters are unlikely to ever reach 4G. Essentially, they are
incremented on memory allocation failures only meaning that the system need
to be in a completely wedged state for a very long time for this to happen.

Given the above additional complexity of u64_stats_sync.h does not seem to
be worth it.

Alex.