Attachment 261141 Details for Bug 287165 – proposed patch

[patch] proposed patch

diff (text/plain), 3.49 KB, created by Mark Johnston on 2025-06-10 13:04:15 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Johnston

Created: 2025-06-10 13:04:15 UTC

Size: 3.49 KB

patch

obsolete

>commit d44352f62c2ca5804127e4cb944b12dc248a8166
>Author: Mark Johnston <markj@FreeBSD.org>
>Date:   Thu May 15 19:14:42 2025 +0000
>
>    e1000: Fix races in the AIM implementation
>    
>    ```
>    - Make sure to load per-ring packet and byte counts using atomic(9), as
>      they might be concurrently modified by transmit/receive tasks, and
>      there is no mutex synchronizing them with the interrupt handler.
>    - Rename a variable so that its value is more clear.
>    - Make sure that bytes and bytes_per_packet are always initialized.
>      It's possible to have tx_bytes != 0 && tx_packets == 0, or rx_bytes !=
>      0 && rx_bytes == 0, due to the race mentioned above, in which case the
>      state machine would test an uninitialized variable.
>    ```
>    
>    PR:             286819
>
>diff --git a/sys/dev/e1000/if_em.c b/sys/dev/e1000/if_em.c
>index f49682285875..f0ef6051fab1 100644
>--- a/sys/dev/e1000/if_em.c
>+++ b/sys/dev/e1000/if_em.c
>@@ -1644,14 +1644,16 @@ em_newitr(struct e1000_softc *sc, struct em_rx_queue *que,
>     struct tx_ring *txr, struct rx_ring *rxr)
> {
> 	struct e1000_hw *hw = &sc->hw;
>+	unsigned long bytes, bytes_per_packet, packets;
>+	unsigned long rxbytes, rxpackets, txbytes, txpackets;
> 	u32 newitr;
>-	u32 bytes;
>-	u32 bytes_packets;
>-	u32 packets;
> 	u8 nextlatency;
> 
>+	rxbytes = atomic_load_long(&rxr->rx_bytes);
>+	txbytes = atomic_load_long(&txr->tx_bytes);
>+
> 	/* Idle, do nothing */
>-	if ((txr->tx_bytes == 0) && (rxr->rx_bytes == 0))
>+	if (txbytes == 0 && rxbytes == 0)
> 		return;
> 
> 	newitr = 0;
>@@ -1671,17 +1673,20 @@ em_newitr(struct e1000_softc *sc, struct em_rx_queue *que,
> 			goto em_set_next_itr;
> 		}
> 
>+		bytes = bytes_per_packet = 0;
> 		/* Get largest values from the associated tx and rx ring */
>-		if (txr->tx_bytes && txr->tx_packets) {
>-			bytes = txr->tx_bytes;
>-			bytes_packets = txr->tx_bytes/txr->tx_packets;
>-			packets = txr->tx_packets;
>+		txpackets = atomic_load_long(&txr->tx_packets);
>+		if (txpackets != 0) {
>+			bytes = txbytes;
>+			bytes_per_packet = txbytes / txpackets;
>+			packets = txpackets;
> 		}
>-		if (rxr->rx_bytes && rxr->rx_packets) {
>-			bytes = max(bytes, rxr->rx_bytes);
>-			bytes_packets =
>-			    max(bytes_packets, rxr->rx_bytes/rxr->rx_packets);
>-			packets = max(packets, rxr->rx_packets);
>+		rxpackets = atomic_load_long(&rxr->rx_packets);
>+		if (rxpackets != 0) {
>+			bytes = lmax(bytes, rxbytes);
>+			bytes_per_packet =
>+			    lmax(bytes_per_packet, rxbytes / rxpackets);
>+			packets = lmax(packets, rxpackets);
> 		}
> 
> 		/* Latency state machine */
>@@ -1691,7 +1696,7 @@ em_newitr(struct e1000_softc *sc, struct em_rx_queue *que,
> 			break;
> 		case itr_latency_lowest: /* 70k ints/s */
> 			/* TSO and jumbo frames */
>-			if (bytes_packets > 8000)
>+			if (bytes_per_packet > 8000)
> 				nextlatency = itr_latency_bulk;
> 			else if ((packets < 5) && (bytes > 512))
> 				nextlatency = itr_latency_low;
>@@ -1699,14 +1704,14 @@ em_newitr(struct e1000_softc *sc, struct em_rx_queue *que,
> 		case itr_latency_low: /* 20k ints/s */
> 			if (bytes > 10000) {
> 				/* Handle TSO */
>-				if (bytes_packets > 8000)
>+				if (bytes_per_packet > 8000)
> 					nextlatency = itr_latency_bulk;
> 				else if ((packets < 10) ||
>-				    (bytes_packets > 1200))
>+				    (bytes_per_packet > 1200))
> 					nextlatency = itr_latency_bulk;
> 				else if (packets > 35)
> 					nextlatency = itr_latency_lowest;
>-			} else if (bytes_packets > 2000) {
>+			} else if (bytes_per_packet > 2000) {
> 				nextlatency = itr_latency_bulk;
> 			} else if (packets < 3 && bytes < 512) {
> 				nextlatency = itr_latency_lowest;

Actions: View | Diff

Attachments on bug 287165: 260809 | 261141 | 261142