Skip to contentSkip to conversation

tg3: adaptive interrupt coalescing, non-napi mode

PATCH In review David Decotigny 4 participants 11 messages Dec 16, 2011

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover tg3: adaptive interrupt coalescing, non-napi mode

This series implements adaptive interrupt coalescing for tg3 NIC, improving performance substancially. It also implements non-NAPI mode for specific system loads.

############################################
# Patch Set Summary:

David Decotigny (2):
tg3: fix possible infinite loop
tg3: move functions related to reset_task together

Maciej Żenczykowski (1):
tg3: Remove IRQF_SAMPLE_RANDOM flag from internal tests

Tom Herbert (1):
tg3: implementation of a non-NAPI mode

Ying Cai (2):
tg3: Implement adaptive interrupt coalescing
tg3: use netif_tx_start_queue instead of wake_queue when no
reschedule needed

 drivers/net/ethernet/broadcom/Kconfig |    8 +
 drivers/net/ethernet/broadcom/tg3.c   |  379 ++++++++++++++++++++++++++++++---
 drivers/net/ethernet/broadcom/tg3.h   |   42 ++++
 3 files changed, 397 insertions(+), 32 deletions(-)
Signature · 1 line
1.7.3.1
#1 lore raw

PATCH 1/6 tg3: fix possible infinite loop

Found by browsing the code.

Tested:
Not tested along the affected path. No regression observed.

---
 drivers/net/ethernet/broadcom/tg3.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/ethernet/broadcom/tg3.c +3 -1
index 8bf11ca..e04c4f9 100644
@@ -13877,8 +13877,10 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
 				continue;
 			}
 			if (pci_id->rev != PCI_ANY_ID) {
-				if (bridge->revision > pci_id->rev)
+				if (bridge->revision > pci_id->rev) {
+					pci_id++;
 					continue;
+				}
 			}
 			if (bridge->subordinate &&
 			    (bridge->subordinate->number ==
Signature · 1 line
1.7.3.1
#2 lore raw

PATCH 2/6 tg3: Remove IRQF_SAMPLE_RANDOM flag from internal tests

In reply to David Decotigny

From: Maciej Żenczykowski <[email protected]>

This complements commit ab392d2d6d4 ("Remove IRQF_SAMPLE_RANDOM flag from network drivers"), removing IRQF_SAMPLE_RANDOM flag from internal and self tests.

Tested: no visible regression on 1 actual host + ethtool -t passes.

---
 drivers/net/ethernet/broadcom/tg3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/ethernet/broadcom/tg3.c +1 -1
index e04c4f9..a65b419 100644
@@ -9387,7 +9387,7 @@ static int tg3_test_interrupt(struct tg3 *tp)
 	}
 
 	err = request_irq(tnapi->irq_vec, tg3_test_isr,
-			  IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, tnapi);
+			  IRQF_SHARED, dev->name, tnapi);
 	if (err)
 		return err;
 
Signature · 1 line
1.7.3.1
#3 lore raw

PATCH 3/6 tg3: Implement adaptive interrupt coalescing

In reply to David Decotigny

From: Ying Cai <[email protected]>

Implement adaptive coalescing in the tg3 driver. On an opteron-based test system, interrupt rate can be reduced from 40K intrs/sec to less than 10K intrs/sec on netperf tests, with netperf performances gaining 2 to 14% (depending on the tests).

Example with 200 netperf streams in parallel for 100s, irq/s measured at the netperf client host, netperf figure is cumulative on all 200 streams:

Without this patch:

TCP_RR       netperf=284208               eth0 irq/s=55141.9
TCP_CRR      netperf=32204.5              eth0 irq/s=15727.7
TCP_MAERTS   netperf=944.68               eth0 irq/s=16255.5

pktgen loopback (pkt_size 60) 484718 pps

With patch:

TCP_RR       netperf=317511  (111.72%)    eth0 irq/s=8307.77 (15.07%)
TCP_CRR      netperf=35890.1 (111.44%)    eth0 irq/s=4390.89 (27.92%)
TCP_MAERTS   netperf=971.64  (102.85%)    eth0 irq/s=8135.58 (50.05%)

pktgen loopback (pkt_size 60) 552185 pps (113.92%)

---
 drivers/net/ethernet/broadcom/tg3.c |  174 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/tg3.h |   37 ++++++++
 2 files changed, 211 insertions(+), 0 deletions(-)
drivers/net/ethernet/broadcom/tg3.c +174 -0
index a65b419..9deb6a6 100644
@@ -52,6 +52,7 @@
 #include <linux/io.h>
 #include <asm/byteorder.h>
 #include <linux/uaccess.h>
+#include <linux/jiffies.h>
 
 #ifdef CONFIG_SPARC
 #include <asm/idprom.h>
@@ -413,6 +414,9 @@ static const struct {
 #define TG3_NUM_TEST	ARRAY_SIZE(ethtool_test_keys)
 
 
+static inline void tg3_full_lock(struct tg3 *tp, int irq_sync);
+static inline void tg3_full_unlock(struct tg3 *tp);
+
 static void tg3_write32(struct tg3 *tp, u32 off, u32 val)
 {
 	writel(val, tp->regs + off);
@@ -5503,6 +5507,168 @@ static void tg3_recycle_rx(struct tg3_napi *tnapi,
 	src_map->data = NULL;
 }
 
+static inline int tg3_coal_adaptive_init(struct tg3 *tp)
+{
+	tp->ad.rx_jiffies         = jiffies;
+	tp->ad.rx_interval        = 0;
+	tp->ad.rx_frames          = 0;
+	tp->ad.rx_average_frames  = 0;
+	tp->ad.rx_reg_frames      = TG3_COAL_RX_FRAMES;
+	tp->ad.rx_adaptive_mode   = TG3_COAL_ADAPTIVE_MODE;
+	tp->ad.rx_frames_high     = TG3_COAL_ADAPTIVE_MAX_FRAMES;
+	tp->ad.rx_usecs_high      = TG3_COAL_ADAPTIVE_MAX_USECS;
+	tp->ad.rx_sample_interval = msecs_to_jiffies(TG3_COAL_ADAPTIVE_SAMPLE);
+
+	/* over-write tg3 stored coalescing values. Using 2.6.11 tg3
+	 * adaptive coalescing values
+	 */
+	tp->coal.rx_coalesce_usecs = TG3_COAL_RX_TICKS;
+	tp->coal.tx_coalesce_usecs = TG3_COAL_TX_TICKS;
+	tp->coal.rx_max_coalesced_frames = TG3_COAL_RX_FRAMES;
+	tp->coal.tx_max_coalesced_frames = TG3_COAL_TX_FRAMES;
+	tp->coal.rx_max_coalesced_frames_irq = TG3_COAL_RX_FRAMES;
+
+	return 0;
+}
+
+static int tg3_coal_adaptive_set(struct net_device *dev,
+				 struct ethtool_coalesce *cmd)
+{
+	struct tg3 *tp = netdev_priv(dev);
+	int i = 0;
+
+	tg3_full_lock(tp, 0);
+	/* changing adaptive coalescing resets the rx frames coalescing to
+	 * the default value. turning off adaptive coalescing, means use the
+	 * default behavior, while turning it on means starts computing now....
+	 * note that this is done before setting any hardcoded values, thus
+	 * allowing a single call to turn off adaptive coalescing and setting
+	 * a new value for hard coded (static) coalescing
+	 */
+
+	/* reset host coalescing engine. */
+	tw32(HOSTCC_MODE, 0);
+	for (i = 0; i < 2000; i++) {
+		if (!(tr32(HOSTCC_MODE) & HOSTCC_MODE_ENABLE))
+			break;
+		udelay(10);
+	}
+
+	if (tp->ad.rx_adaptive_mode != cmd->use_adaptive_rx_coalesce) {
+		tp->ad.rx_jiffies  = jiffies;
+		tp->ad.rx_frames   = 0;
+		tp->ad.rx_interval = 0;
+		tp->ad.rx_average_frames = 0;
+	}
+
+	tp->ad.rx_adaptive_mode   = cmd->use_adaptive_rx_coalesce;
+	tp->ad.rx_usecs_high      = cmd->rx_coalesce_usecs_high;
+	tp->ad.rx_frames_high     = cmd->rx_max_coalesced_frames_high;
+	tp->ad.rx_sample_interval = msecs_to_jiffies(cmd->rate_sample_interval);
+
+	tw32(HOSTCC_MODE, HOSTCC_MODE_ENABLE | tp->coalesce_mode);
+
+	tg3_full_unlock(tp);
+
+	return 0;
+}
+
+static int tg3_coal_adaptive_get(struct net_device *dev,
+				 struct ethtool_coalesce *cmd)
+{
+	struct tg3 *tp = netdev_priv(dev);
+
+	if (tp->ad.rx_adaptive_mode) {
+		tg3_full_lock(tp, 0);
+
+		cmd->rx_coalesce_usecs       = tr32(HOSTCC_RXCOL_TICKS);
+		cmd->rx_max_coalesced_frames = tr32(HOSTCC_RXMAX_FRAMES);
+
+		cmd->tx_coalesce_usecs       = tr32(HOSTCC_TXCOL_TICKS);
+		cmd->tx_max_coalesced_frames = tr32(HOSTCC_TXMAX_FRAMES);
+
+		cmd->rx_max_coalesced_frames_irq = tr32(HOSTCC_RXCOAL_MAXF_INT);
+		cmd->tx_max_coalesced_frames_irq = tr32(HOSTCC_TXCOAL_MAXF_INT);
+
+		cmd->use_adaptive_rx_coalesce     = tp->ad.rx_adaptive_mode;
+		cmd->rx_coalesce_usecs_high       = tp->ad.rx_usecs_high;
+		cmd->rx_max_coalesced_frames_high = tp->ad.rx_frames_high;
+		cmd->rate_sample_interval         = jiffies_to_msecs(
+		    tp->ad.rx_sample_interval);
+
+		tg3_full_unlock(tp);
+	}
+	return 0;
+}
+
+static inline int tg3_coal_adaptive_rx(struct tg3 *tp, int received)
+{
+	unsigned long cur_jiffies = jiffies;
+
+	unsigned long diff = cur_jiffies - tp->ad.rx_jiffies;
+
+	tp->ad.rx_interval += diff;
+
+	tp->ad.rx_jiffies  = cur_jiffies;
+	tp->ad.rx_frames  += received;
+
+	if ((tp->ad.rx_interval >= tp->ad.rx_sample_interval) &&
+	    (0 != tp->ad.rx_interval)) {
+		unsigned long rx_rate;
+
+		/* average packet per ms   */
+		tp->ad.rx_frames /= jiffies_to_msecs(tp->ad.rx_interval);
+		/* apply coalescing factor */
+		tp->ad.rx_frames >>= TG3_COAL_FACTOR_EXP;
+
+		/* compute a running average of the packet rate
+		 * the goal of the running average is to provide a faster
+		 * response to lowering rate compare to slowly increasing rate.
+		 *
+		 * if the new sample interval rate as decreased from the running
+		 * average, set the register coalescing to new sample interval
+		 * rate. else, average the new rate with the running average
+		 */
+		tp->ad.rx_average_frames += tp->ad.rx_frames;
+		tp->ad.rx_average_frames >>= 1;
+
+		if (tp->ad.rx_frames <= tp->ad.rx_average_frames)
+			rx_rate = tp->ad.rx_frames;
+		else
+			rx_rate = tp->ad.rx_average_frames;
+
+		/* adjust based on max values. Also do not set a '0' value in
+		 * the average frames, always set the average to at least one
+		 * frame. From the BCM documentation it is recommended to set
+		 * the register value to get an interrupt for every rx packet.
+		 * we could use 0, which would disable coalescing and should
+		 * have the same result.
+		 */
+		if (rx_rate > tp->ad.rx_frames_high)
+			rx_rate = tp->ad.rx_frames_high;
+		else if (0 == rx_rate)
+			rx_rate = 1;
+
+		if (rx_rate != tp->ad.rx_reg_frames) {
+			unsigned long rx_usecs;
+			rx_usecs = rx_rate *  TG3_COAL_TICK_PER_FRAME;
+			if (rx_usecs > tp->ad.rx_usecs_high)
+				rx_usecs = tp->ad.rx_usecs_high;
+
+			tw32(HOSTCC_RXCOL_TICKS, rx_usecs);
+			tw32(HOSTCC_RXMAX_FRAMES, rx_rate);
+			tw32(HOSTCC_RXCOAL_MAXF_INT, rx_rate);
+			tp->ad.rx_reg_frames = rx_rate;
+		}
+
+		tp->ad.rx_frames   = 0;
+		tp->ad.rx_interval = 0;
+
+	}
+
+	return 0;
+}
+
 /* The RX ring scheme is composed of multiple rings which post fresh
  * buffers to the chip, and one special ring the chip uses to report
  * status back to the host.
@@ -5679,6 +5845,9 @@ next_pkt_nopost:
 		}
 	}
 
+	if (TG3_COAL_ADAPTIVE_ON == tp->ad.rx_adaptive_mode)
+		tg3_coal_adaptive_rx(tp, received);
+
 	/* ACK the status ring. */
 	tnapi->rx_rcb_ptr = sw_idx;
 	tw32_rx_mbox(tnapi->consmbox, sw_idx);
@@ -11866,6 +12035,7 @@ static int tg3_get_coalesce(struct net_device *dev, struct ethtool_coalesce *ec)
 	struct tg3 *tp = netdev_priv(dev);
 
 	memcpy(ec, &tp->coal, sizeof(*ec));
+	tg3_coal_adaptive_get(dev, ec);
 	return 0;
 }
 
@@ -11915,6 +12085,8 @@ static int tg3_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec)
 	tp->coal.tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
 	tp->coal.stats_block_coalesce_usecs = ec->stats_block_coalesce_usecs;
 
+	tg3_coal_adaptive_set(dev, ec);
+
 	if (netif_running(dev)) {
 		tg3_full_lock(tp, 0);
 		__tg3_set_coalesce(tp, &tp->coal);
@@ -15319,6 +15491,8 @@ static void __devinit tg3_init_coal(struct tg3 *tp)
 		ec->tx_coalesce_usecs_irq = 0;
 		ec->stats_block_coalesce_usecs = 0;
 	}
+
+	tg3_coal_adaptive_init(tp);
 }
 
 static const struct net_device_ops tg3_netdev_ops = {
drivers/net/ethernet/broadcom/tg3.h +37 -0
index aea8f72..695cf14 100644
@@ -3211,6 +3211,43 @@ struct tg3 {
 
 	struct ethtool_coalesce		coal;
 
+	struct {
+		unsigned long  rx_jiffies;	  /* last read jiffies */
+		unsigned long  rx_interval;	  /* rcv interval in jiffies */
+		unsigned long  rx_reg_frames;	  /* current register value */
+		unsigned long  rx_frames;	  /* received frame in interval*/
+		unsigned long  rx_average_frames; /* computed received average */
+		unsigned int   rx_adaptive_mode;  /* adaptive mode on/off */
+		unsigned long  rx_frames_high;	  /* max coalescing frame in */
+						  /* adaptive mode */
+		unsigned long  rx_usecs_high;	  /* max usecs in adaptive  */
+						  /* mode */
+		unsigned long  rx_sample_interval;/* adaptive sample rate */
+						  /* in jiffies (msecs) */
+	} ad; /* adaptive coalescing */
+
+#define TG3_COAL_ADAPTIVE_ON	1
+#define TG3_COAL_ADAPTIVE_OFF	0
+#define TG3_COAL_ADAPTIVE_MODE	TG3_COAL_ADAPTIVE_ON
+
+/* Ticks are in TG3 register units and not in system units */
+#define TG3_COAL_FACTOR_EXP	3 /* coalescence factor, num irq per ms max. */
+				  /* this is the exponent of a power of two.  */
+
+#define TG3_COAL_TICK_PER_FRAME	10 /* tick per frame, num of us per tick */
+				   /* per frame. Used tg3 ms tick unit */
+
+#define TG3_COAL_ADAPTIVE_MAX_FRAMES	32
+#define TG3_COAL_ADAPTIVE_MAX_USECS	(TG3_COAL_ADAPTIVE_MAX_FRAMES \
+					 << TG3_COAL_TICK_PER_FRAME)
+
+#define TG3_COAL_ADAPTIVE_SAMPLE	10 /* ms samples */
+
+#define TG3_COAL_TX_FRAMES	32  /* must be <= to 1/2 TG3_TX_RING_SIZE */
+#define TG3_COAL_TX_TICKS	(TG3_COAL_TX_FRAMES * TG3_COAL_TICK_PER_FRAME)
+#define TG3_COAL_RX_FRAMES	1
+#define TG3_COAL_RX_TICKS	(TG3_COAL_RX_FRAMES * TG3_COAL_TICK_PER_FRAME)
+
 	/* firmware info */
 	const char			*fw_needed;
 	const struct firmware		*fw;
Signature · 1 line
1.7.3.1
#4 lore raw

PATCH 4/6 tg3: move functions related to reset_task together

In reply to David Decotigny

This prepares next patches: simply move functions related to reset_task close to its definition.

---
 drivers/net/ethernet/broadcom/tg3.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)
drivers/net/ethernet/broadcom/tg3.c +13 -12
index 9deb6a6..ecd6ea5 100644
@@ -416,6 +416,7 @@ static const struct {
 
 static inline void tg3_full_lock(struct tg3 *tp, int irq_sync);
 static inline void tg3_full_unlock(struct tg3 *tp);
+static inline void tg3_reset_task_schedule(struct tg3 *tp);
 
 static void tg3_write32(struct tg3 *tp, u32 off, u32 val)
 {
@@ -6080,18 +6081,6 @@ static int tg3_poll_work(struct tg3_napi *tnapi, int work_done, int budget)
 	return work_done;
 }
 
-static inline void tg3_reset_task_schedule(struct tg3 *tp)
-{
-	if (!test_and_set_bit(TG3_FLAG_RESET_TASK_PENDING, tp->tg3_flags))
-		schedule_work(&tp->reset_task);
-}
-
-static inline void tg3_reset_task_cancel(struct tg3 *tp)
-{
-	cancel_work_sync(&tp->reset_task);
-	tg3_flag_clear(tp, RESET_TASK_PENDING);
-}
-
 static int tg3_poll_msix(struct napi_struct *napi, int budget)
 {
 	struct tg3_napi *tnapi = container_of(napi, struct tg3_napi, napi);
@@ -6543,6 +6532,18 @@ out:
 	tg3_flag_clear(tp, RESET_TASK_PENDING);
 }
 
+static inline void tg3_reset_task_schedule(struct tg3 *tp)
+{
+	if (!test_and_set_bit(TG3_FLAG_RESET_TASK_PENDING, tp->tg3_flags))
+		schedule_work(&tp->reset_task);
+}
+
+static inline void tg3_reset_task_cancel(struct tg3 *tp)
+{
+	cancel_work_sync(&tp->reset_task);
+	tg3_flag_clear(tp, RESET_TASK_PENDING);
+}
+
 static void tg3_tx_timeout(struct net_device *dev)
 {
 	struct tg3 *tp = netdev_priv(dev);
Signature · 1 line
1.7.3.1
#5 lore raw

PATCH 5/6 tg3: implementation of a non-NAPI mode

In reply to David Decotigny

From: Tom Herbert <[email protected]>

The tg3 NIC has a hard limit of 511 descriptors for the receive ring. Under heavy load of small packets, this device receive queue may not be serviced fast enough to prevent packets drops. This could be due to a variety of reasons such as lengthy processing delays of packets in the stack, softirqs being disabled too long, etc. If the driver is run in non-NAPI mode, the RX queue is serviced in the device interrupt, which is much less likely to be deferred for a substantial period of time.

There are some effects in not using NAPI that need to be considered. It does increase the chance of live-lock in interrupt handler, although since the tg3 does interrupt coalescing this is very unlikely to occur. Also, more code is being run with interrupts disabled potentially deferring other hardware interrupts. The amount of time spent in the interrupt handler should be minimized by dequeuing packets of the device queue and queuing them to a host queue as quickly as possible.

The default mode of operation remains NAPI and its performances are kept unchanged (code unchanged). Non-NAPI mode is enabled by commenting-out CONFIG_TIGON3_NAPI Kconfig parameter.

---
 drivers/net/ethernet/broadcom/Kconfig |    8 ++
 drivers/net/ethernet/broadcom/tg3.c   |  151 +++++++++++++++++++++++++++++++--
 drivers/net/ethernet/broadcom/tg3.h   |    5 +
 3 files changed, 157 insertions(+), 7 deletions(-)
drivers/net/ethernet/broadcom/Kconfig +8 -0
index f15e72e..e808b3d 100644
@@ -107,6 +107,14 @@ config TIGON3
 	  To compile this driver as a module, choose M here: the module
 	  will be called tg3.  This is recommended.
 
+if TIGON3
+config TIGON3_NAPI
+	bool "Use Rx Polling (NAPI)"
+	default y
+	---help---
+	  Use NAPI for Tigon3 driver. If unsure, say Y.
+endif # TIGON3
+
 config BNX2X
 	tristate "Broadcom NetXtremeII 10Gb support"
 	depends on PCI
drivers/net/ethernet/broadcom/tg3.c +144 -7

339 lines not on this page · raw message

drivers/net/ethernet/broadcom/tg3.h +5 -0
index 695cf14..b023e96 100644
@@ -2832,7 +2832,9 @@ struct tg3_rx_prodring_set {
 #define TG3_IRQ_MAX_VECS		TG3_IRQ_MAX_VECS_RSS
 
 struct tg3_napi {
+#ifdef CONFIG_TIGON3_NAPI
 	struct napi_struct		napi	____cacheline_aligned;
+#endif
 	struct tg3			*tp;
 	struct tg3_hw_status		*hw_status;
 
@@ -3167,6 +3169,9 @@ struct tg3 {
 	struct tg3_hw_stats		*hw_stats;
 	dma_addr_t			stats_mapping;
 	struct work_struct		reset_task;
+#ifndef CONFIG_TIGON3_NAPI
+	struct work_struct              poll_link_task;
+#endif
 
 	int				nvram_lock_cnt;
 	u32				nvram_size;
Signature · 1 line
1.7.3.1
#6 lore raw

Re: [PATCH net-next v1 5/6] tg3: implementation of a non-NAPI mode

Quoted 7 lines · David Decotigny

From: Tom Herbert <[email protected]>

The tg3 NIC has a hard limit of 511 descriptors for the receive ring. Under heavy load of small packets, this device receive queue may not be serviced fast enough to prevent packets drops. This could be due to a variety of reasons such as lengthy processing delays of packets in the stack, softirqs being disabled too long, etc.

[...]

I think those are bugs to be fixed, not worked around.

Various drivers had NAPI as a compile-time option for a while, and pretty much all of those have now been made to use NAPI unconditionally (I think tulip is the only one left). Adding such an option back is unlikely to be accepted now.

Ben.

Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
#8 lore raw

Re: [PATCH net-next v1 5/6] tg3: implementation of a non-NAPI mode

In reply to David Decotigny
Quoted 20 lines · David Decotigny

From: Tom Herbert <[email protected]>

The tg3 NIC has a hard limit of 511 descriptors for the receive ring. Under heavy load of small packets, this device receive queue may not be serviced fast enough to prevent packets drops. This could be due to a variety of reasons such as lengthy processing delays of packets in the stack, softirqs being disabled too long, etc. If the driver is run in non-NAPI mode, the RX queue is serviced in the device interrupt, which is much less likely to be deferred for a substantial period of time.

There are some effects in not using NAPI that need to be considered. It does increase the chance of live-lock in interrupt handler, although since the tg3 does interrupt coalescing this is very unlikely to occur. Also, more code is being run with interrupts disabled potentially deferring other hardware interrupts. The amount of time spent in the interrupt handler should be minimized by dequeuing packets of the device queue and queuing them to a host queue as quickly as possible.

Quoted 5 lines

The default mode of operation remains NAPI and its performances are kept unchanged (code unchanged). Non-NAPI mode is enabled by commenting-out CONFIG_TIGON3_NAPI Kconfig parameter.

Oh well, thats ugly :(

I suspect this was only used with RPS/RFS ?

Or interrupts stick on a given cpu ?

Because with a default setup, and IRQ serviced by multiple cpus, you endup with possible packet reorderings.

Packet1,2,3,4 handled by CPU0 : queued on netif_rx() queue. EndOfInterrupt
Packet4,5,6,7 handled by CPU1 : queued on netif_rx() queue. EndOfInterrupt

CPU0/CPU1 happily merge packets...

#9 lore raw

PATCH 6/6 tg3: use netif_tx_start_queue instead of wake_queue when no reschedule needed

In reply to David Decotigny

From: Ying Cai <[email protected]>

This commit replaces netif_tx_wake_queue() with netif_tx_start_queue() when __netif_schedule() is not needed. It also adds code to deal with race condition between netif_tx_start_queue() and netif_tx_stop_queue().

---
 drivers/net/ethernet/broadcom/tg3.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)
drivers/net/ethernet/broadcom/tg3.c +12 -11
index e3f221d..311e073 100644
@@ -6860,9 +6860,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct sk_buff *skb)
 {
 	struct sk_buff *segs, *nskb;
 	u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+	struct tg3_napi *tnapi = &tp->napi[0];
 
 	/* Estimate the number of fragments in the worst case */
-	if (unlikely(tg3_tx_avail(&tp->napi[0]) <= frag_cnt_est)) {
+	if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
 		netif_stop_queue(tp->dev);
 
 		/* netif_tx_stop_queue() must be done before checking
@@ -6871,10 +6872,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct sk_buff *skb)
 		 * netif_tx_queue_stopped().
 		 */
 		smp_mb();
-		if (tg3_tx_avail(&tp->napi[0]) <= frag_cnt_est)
+		if (tg3_tx_avail(tnapi) <= TG3_TX_WAKEUP_THRESH(tnapi))
 			return NETDEV_TX_BUSY;
 
-		netif_wake_queue(tp->dev);
+		netif_start_queue(tp->dev);
 	}
 
 	segs = skb_gso_segment(skb, tp->dev->features & ~NETIF_F_TSO);
@@ -6926,14 +6927,14 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	 * interrupt context.
 	 */
 	if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
-		if (!netif_tx_queue_stopped(txq)) {
-			netif_tx_stop_queue(txq);
+		/* This is a hard error, log it. */
+		netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
 
-			/* This is a hard error, log it. */
-			netdev_err(dev,
-				   "BUG! Tx Ring full when queue awake!\n");
-		}
-		return NETDEV_TX_BUSY;
+		netif_tx_stop_queue(txq);
+		smp_mb();
+		if (tg3_tx_avail(tnapi) <= TG3_TX_WAKEUP_THRESH(tnapi))
+			return NETDEV_TX_BUSY;
+		netif_tx_start_queue(txq);
 	}
 
 	entry = tnapi->tx_prod;
@@ -7100,7 +7101,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		 */
 		smp_mb();
 		if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
-			netif_tx_wake_queue(txq);
+			netif_tx_start_queue(txq);
 	}
 
 	mmiowb();
Signature · 1 line
1.7.3.1
maintainer
#10 lore raw
In reply to David Decotigny

From: David Decotigny <[email protected]>
Date: Fri, 16 Dec 2011 10:19:43 -0800

Quoted 3 lines

This series implements adaptive interrupt coalescing for tg3 NIC, improving performance substancially. It also implements non-NAPI mode for specific system loads.

I specifically removed the dynamic IRQ coalescing from this driver years ago.

It's too susceptible to state changes.

I am sure that you have some nice benchmark for which one scheme helps, but more generally it is not possible to make it such that you will avoid the case where the network flow pattern changes by the time you change the chip configuration and thus the result is suboptimal.

I highly recommend these changes are not applied, because they will hurt someone.