S2io: Performance improvements

PATCH In review [email protected] 8 participants 20 messages Jul 7, 2005 – Aug 2, 2005

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

[email protected]

author

2005-07-07 22:27 UTC #0 lore raw

PATCH 5/12 S2io: Performance improvements

Hi,
This patch relates to mostly performance related changes.
1. Fixed incorrect computation of PANIC level in rx_buffer_level().
2. Removed unnecessary PIOs(read/write of tx_traffic_int and rx_traffic_int) from interrupt handler and removed read of general_int_status register from xmit routine.
3. Enable two-buffer mode(for Rx path) automatically for SGI systems. This improves Rx performance dramatically on SGI systems.

Signed-off-by Ravinandan Arakali <[email protected]>

Signed-off-by Raghavendra Koushik <[email protected]>

---

drivers/net/s2io.c +19 ~~-27~~

diff -uprN vanilla_kernel/drivers/net/s2io.c linux-2.6.12-rc6/drivers/net/s2io.c
@@ -99,8 +99,7 @@ static inline int rx_buffer_level(nic_t 
 	mac_control = &sp->mac_control;
 	if ((mac_control->rings[ring].pkt_cnt - rxb_size) > 16) {
 		level = LOW;
-		if ((mac_control->rings[ring].pkt_cnt - rxb_size) <
-				MAX_RXDS_PER_BLOCK) {
+		if (rxb_size <= MAX_RXDS_PER_BLOCK) {
 			level = PANIC;
 		}
 	}

@@ -2194,7 +2193,6 @@ static void rx_intr_handler(ring_info_t 
 {
 	nic_t *nic = ring_data->nic;
 	struct net_device *dev = (struct net_device *) nic->dev;
-	XENA_dev_config_t __iomem *bar0 = nic->bar0;
 	int get_block, get_offset, put_block, put_offset, ring_bufs;
 	rx_curr_get_info_t get_info, put_info;
 	RxD_t *rxdp;

@@ -2202,8 +2200,6 @@ static void rx_intr_handler(ring_info_t 
 #ifndef CONFIG_S2IO_NAPI
 	int pkt_cnt = 0;
 #endif
-	register u64 val64;
-
 	spin_lock(&nic->rx_lock);
 	if (atomic_read(&nic->card_state) == CARD_DOWN) {
 		DBG_PRINT(ERR_DBG, "%s: %s going down for reset\n",

@@ -2211,13 +2207,6 @@ static void rx_intr_handler(ring_info_t 
 		spin_unlock(&nic->rx_lock);
 	}
 
-	/*
-	 * rx_traffic_int reg is an R1 register, hence we read and write
-	 * back the same value in the register to clear it
-	 */
-	val64 = readq(&bar0->tx_traffic_int);
-	writeq(val64, &bar0->tx_traffic_int);
-
 	get_info = ring_data->rx_curr_get_info;
 	get_block = get_info.block_index;
 	put_info = ring_data->rx_curr_put_info;

@@ -2313,20 +2302,11 @@ static void rx_intr_handler(ring_info_t 
 static void tx_intr_handler(fifo_info_t *fifo_data)
 {
 	nic_t *nic = fifo_data->nic;
-	XENA_dev_config_t __iomem *bar0 = nic->bar0;
 	struct net_device *dev = (struct net_device *) nic->dev;
 	tx_curr_get_info_t get_info, put_info;
 	struct sk_buff *skb;
 	TxD_t *txdlp;
 	u16 j, frg_cnt;
-	register u64 val64 = 0;
-
-	/*
-	 * tx_traffic_int reg is an R1 register, hence we read and write
-	 * back the same value in the register to clear it
-	 */
-	val64 = readq(&bar0->tx_traffic_int);
-	writeq(val64, &bar0->tx_traffic_int);
 
 	get_info = fifo_data->tx_curr_get_info;
 	put_info = fifo_data->tx_curr_put_info;

@@ -2819,7 +2799,6 @@ int s2io_xmit(struct sk_buff *skb, struc
 #endif
 	mac_info_t *mac_control;
 	struct config_param *config;
-	XENA_dev_config_t __iomem *bar0 = sp->bar0;
 
 	mac_control = &sp->mac_control;
 	config = &sp->config;

@@ -2871,7 +2850,6 @@ int s2io_xmit(struct sk_buff *skb, struc
 	}
 
 	txdp->Control_2 |= config->tx_intr_type;
-
 	txdp->Control_1 |= (TXD_BUFFER0_SIZE(frg_len) |
 			    TXD_GATHER_CODE_FIRST);
 	txdp->Control_1 |= TXD_LIST_OWN_XENA;

@@ -2891,6 +2869,8 @@ int s2io_xmit(struct sk_buff *skb, struc
 	val64 = mac_control->fifos[queue].list_info[put_off].list_phy_addr;
 	writeq(val64, &tx_fifo->TxDL_Pointer);
 
+	wmb();
+
 	val64 = (TX_FIFO_LAST_TXD_NUM(frg_cnt) | TX_FIFO_FIRST_LIST |
 		 TX_FIFO_LAST_LIST);

@@ -2900,9 +2880,6 @@ int s2io_xmit(struct sk_buff *skb, struc
 #endif
 	writeq(val64, &tx_fifo->List_Control);
 
-	/* Perform a PCI read to flush previous writes */
-	val64 = readq(&bar0->general_int_status);
-
 	put_off++;
 	put_off %= mac_control->fifos[queue].tx_curr_put_info.fifo_len + 1;
 	mac_control->fifos[queue].tx_curr_put_info.offset = put_off;

@@ -2941,7 +2918,7 @@ static irqreturn_t s2io_isr(int irq, voi
 	nic_t *sp = dev->priv;
 	XENA_dev_config_t __iomem *bar0 = sp->bar0;
 	int i;
-	u64 reason = 0;
+	u64 reason = 0, val64;
 	mac_info_t *mac_control;
 	struct config_param *config;

@@ -2979,6 +2956,13 @@ static irqreturn_t s2io_isr(int irq, voi
 #else
 	/* If Intr is because of Rx Traffic */
 	if (reason & GEN_INTR_RXTRAFFIC) {
+		/*
+		 * rx_traffic_int reg is an R1 register, writing all 1's
+		 * will ensure that the actual interrupt causing bit get's
+		 * cleared and hence a read can be avoided.
+		 */
+		val64 = 0xFFFFFFFFFFFFFFFFULL;
+		writeq(val64, &bar0->rx_traffic_int);
 		for (i = 0; i < config->rx_ring_num; i++) {
 			rx_intr_handler(&mac_control->rings[i]);
 		}

@@ -2987,6 +2971,14 @@ static irqreturn_t s2io_isr(int irq, voi
 
 	/* If Intr is because of Tx Traffic */
 	if (reason & GEN_INTR_TXTRAFFIC) {
+		/*
+		 * tx_traffic_int reg is an R1 register, writing all 1's
+		 * will ensure that the actual interrupt causing bit get's
+		 * cleared and hence a read can be avoided.
+		 */
+		val64 = 0xFFFFFFFFFFFFFFFFULL;
+		writeq(val64, &bar0->tx_traffic_int);
+
 		for (i = 0; i < config->tx_fifo_num; i++)
 			tx_intr_handler(&mac_control->fifos[i]);
 	}

drivers/net/s2io.h +5 -0

diff -uprN vanilla_kernel/drivers/net/s2io.h linux-2.6.12-rc6/drivers/net/s2io.h
@@ -13,6 +13,11 @@
 #ifndef _S2IO_H
 #define _S2IO_H
 
+/* Enable 2 buffer mode by default for SGI system */
+#ifdef CONFIG_IA64_SGI_SN2
+#define CONFIG_2BUFF_MODE
+#endif
+
 #define TBD 0
 #define BIT(loc)		(0x8000000000000000ULL >> (loc))
 #define vBIT(val, loc, sz)	(((u64)val) << (64-loc-sz))

Arthur Kepner

2005-07-07 23:15 UTC #1 lore raw

Quoted 15 lines · On Thu, 7 Jul 2005 [email protected] wrote:

.......
2. Removed unnecessary PIOs(read/write of tx_traffic_int and rx_traffic_int) from interrupt handler and removed read of general_int_status register from xmit routine.
......
drivers/net/s2io.c +2 -0
@@ -2891,6 +2869,8 @@ int s2io_xmit(struct sk_buff *skb, struc
 	val64 = mac_control->fifos[queue].list_info[put_off].list_phy_addr;
 	writeq(val64, &tx_fifo->TxDL_Pointer);
 
+	wmb();
+
 	val64 = (TX_FIFO_LAST_TXD_NUM(frg_cnt) | TX_FIFO_FIRST_LIST |
 		 TX_FIFO_LAST_LIST);
 

drivers/net/s2io.c int s2io_xmit(struct sk_buff *skb, struc · line 2883

Quoted 7 lines

drivers/net/s2io.c +0 -3

@@ -2900,9 +2880,6 @@ int s2io_xmit(struct sk_buff *skb, struc
 #endif
 	writeq(val64, &tx_fifo->List_Control);
 
-	/* Perform a PCI read to flush previous writes */
-	val64 = readq(&bar0->general_int_status);
-
 	put_off++;

I thought that an mmiowb() was called for here (to order the PIO writes above more cheaply than doing the readq()). I posted a patch like this some time ago:

http://marc.theaimsgroup.com/?l=linux-netdev&m=111508292028110&w=2

FWIW, I've done quite a few performance measurements with the patch I posted earlier, and it's worked well. For 1500 byte mtus throughput goes up by ~20%. Is even the mmiowb() unnecessary?

What is the wmb() above for?

--
Arthur

Raghavendra Koushik

author

2005-07-08 01:06 UTC #2 lore raw

Quoted 5 lines

I thought that an mmiowb() was called for here (to order the PIO writes above more cheaply than doing the readq()). I posted a patch like this some time ago:
http://marc.theaimsgroup.com/?l=linux-netdev&m=111508292028110&w=2

On an Altix machine I believe the readq was necessary to flush the PIO writes. How long did you run the tests? I had seen in long duration tests that an occasional write
(TXDL control word and the address) would be missed and the xmit Get's stuck.

Quoted 5 lines

FWIW, I've done quite a few performance measurements with the patch I posted earlier, and it's worked well. For 1500 byte mtus throughput goes up by ~20%. Is even the mmiowb() unnecessary?

Was this on 2.4 kernel because I think the readq would not have a significant impact on 2.6 kernels due to TSO.
(with TSO on the number of packets that actually enter the Xmit routine would be reduced apprx 40 times).

Quoted 1 line

What is the wmb() above for?

wmb() is to ensure ordered PIO writes.

Thanks
- Koushik

Quoted 52 lines

-----Original Message-----
From: Arthur Kepner [mailto:[email protected]]
Sent: Thursday, July 07, 2005 4:15 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Review int s2io_xmit(struct sk_buff *skb, struc · line 2869
Quoted 8 lines · On Thu, 7 Jul 2005 [email protected] wrote:
.......
2. Removed unnecessary PIOs(read/write of tx_traffic_int and rx_traffic_int) from interrupt handler and removed read of general_int_status register from xmit routine.
......
(quoted diff) +0 -0
@@ -2891,6 +2869,8 @@ int s2io_xmit(struct sk_buff *skb, struc
 	val64 = 
mac_control->fifos[queue].list_info[put_off].list_phy_addr;
Quoted 8 lines
drivers/net/s2io.c +2 -0
 	writeq(val64, &tx_fifo->TxDL_Pointer);
 
+	wmb();
+
 	val64 = (TX_FIFO_LAST_TXD_NUM(frg_cnt) | TX_FIFO_FIRST_LIST |
 		 TX_FIFO_LAST_LIST);
 
drivers/net/s2io.c int s2io_xmit(struct sk_buff *skb, struc · line 2883
Quoted 7 lines
drivers/net/s2io.c +0 -3
@@ -2900,9 +2880,6 @@ int s2io_xmit(struct sk_buff *skb, struc
 #endif
 	writeq(val64, &tx_fifo->List_Control);
 
-	/* Perform a PCI read to flush previous writes */
-	val64 = readq(&bar0->general_int_status);
-
 	put_off++;
I thought that an mmiowb() was called for here (to order the PIO writes above more cheaply than doing the readq()). I posted a patch like this some time ago:
http://marc.theaimsgroup.com/?l=linux-netdev&m=111508292028110&w=2
FWIW, I've done quite a few performance measurements with the patch I posted earlier, and it's worked well. For 1500 byte mtus throughput goes up by ~20%. Is even the mmiowb() unnecessary?
What is the wmb() above for?
--
Arthur

David S. Miller

maintainer

2005-07-08 03:00 UTC #3 lore raw

From: "Raghavendra Koushik" <[email protected]>
Date: Thu, 7 Jul 2005 18:06:19 -0700

Quoted 1 line

wmb() is to ensure ordered PIO writes.

wmb() does no such thing. It only has influence on load and store instructions done by the local processor, it has no effect on what the PCI bus may do with PIO writes (ie. post them).

If you need a PIO to complete in a specific order, you have to read it back. If you need PIO operations to occur in a specific order wrt. cpu memory operations, mmiowb() is what you need to use.

Jeff Garzik

2005-07-08 03:08 UTC #4 lore raw

Quoted 2 lines · David S. Miller

If you need a PIO to complete in a specific order, you have to read it back. If you need PIO operations to occur

Correct.

A PCI read is the only way to ensure that all the CPU/PCI bridge buffers are flushed to the device.

Whenever Arjan and I complain about "PCI posting" problems, we are indicating a need for additional readl() calls to ensure ordering/flushing. Delaying immediately after a writel() is a classic PCI posting mistake. Assuming ordering is another.

Jeff

Arthur Kepner

2005-07-08 15:31 UTC #5 lore raw

In reply to Raghavendra Koushik

Quoted 7 lines · Raghavendra Koushik

....
On an Altix machine I believe the readq was necessary to flush the PIO writes. How long did you run the tests? I had seen in long duration tests that an occasional write
(TXDL control word and the address) would be missed and the xmit Get's stuck.

The most recent tests I did used pktgen, and they ran for a total time of ~.5 hours (changing pkt_size every 30 seconds or so). The pktgen tests and other tests (like nttcp) have been run several times, so I've exercised the card for a total of several hours without any problems.

Quoted 7 lines

Quoted 6 lines
FWIW, I've done quite a few performance measurements with the patch I posted earlier, and it's worked well. For 1500 byte mtus throughput goes up by ~20%. Is even the mmiowb() unnecessary?

Quoted 5 lines

Was this on 2.4 kernel because I think the readq would not have a significant impact on 2.6 kernels due to TSO.
(with TSO on the number of packets that actually enter the Xmit routine would be reduced apprx 40 times).
.....

This was with a 2.6 kernel (with TSO on). PIO reads are pretty expensive on Altix, so eliminating them really helps us.

For big mtus (>=4KBytes) the benefit of replacing the readq() with mmiowb() in s2io_xmit() is negligible.

--
Arthur

Raghavendra Koushik

author

2005-07-08 18:16 UTC #6 lore raw

I'll include this fix in the next patch that incorporates any other review comments coming my way.. Thanks for pointing it out.

-Koushik

Quoted 50 lines

-----Original Message-----
From: Arthur Kepner [mailto:[email protected]]
Sent: Friday, July 08, 2005 8:31 AM
To: Raghavendra Koushik
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Quoted 7 lines · Raghavendra Koushik
....
On an Altix machine I believe the readq was necessary to flush the PIO writes. How long did you run the tests? I had seen in long duration tests that an occasional write
(TXDL control word and the address) would be missed and the xmit Get's stuck.
The most recent tests I did used pktgen, and they ran for a total time of ~.5 hours (changing pkt_size every 30 seconds or so). The pktgen tests and other tests (like nttcp) have been run several times,
so I've exercised the card for a total of several hours without any problems.
Quoted 3 lines
Quoted 3 lines
FWIW, I've done quite a few performance measurements with
the patch
Quoted 1 line
Quoted 1 line
I posted earlier, and it's worked well. For 1500 byte
mtus throughput
Quoted 8 lines
Quoted 2 lines
goes up by ~20%. Is even the mmiowb() unnecessary?
Was this on 2.4 kernel because I think the readq would not have a significant impact on 2.6 kernels due to TSO.
(with TSO on the number of packets that actually enter the Xmit routine would be reduced apprx 40 times).
.....
This was with a 2.6 kernel (with TSO on). PIO reads are pretty expensive on Altix, so eliminating them really helps us.
For big mtus (>=4KBytes) the benefit of replacing the readq() with mmiowb() in s2io_xmit() is negligible.
--
Arthur

Ravinandan Arakali

2005-07-08 18:17 UTC #7 lore raw

In reply to Arthur Kepner

Arthur/David/Jeff,
Thanks for pointing that out. We will wait for any other comments on our 12 patches. If there are no other, will send out a patch13 to include the mmiowb() change.

Thanks,
Ravi

-----Original Message-----
From: Arthur Kepner [mailto:[email protected]]
Sent: Friday, July 08, 2005 8:31 AM
To: Raghavendra Koushik
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

Quoted 7 lines · Raghavendra Koushik

....
On an Altix machine I believe the readq was necessary to flush the PIO writes. How long did you run the tests? I had seen in long duration tests that an occasional write
(TXDL control word and the address) would be missed and the xmit Get's stuck.

Quoted 7 lines

Quoted 6 lines
FWIW, I've done quite a few performance measurements with the patch I posted earlier, and it's worked well. For 1500 byte mtus throughput goes up by ~20%. Is even the mmiowb() unnecessary?

Quoted 5 lines

Was this on 2.4 kernel because I think the readq would not have a significant impact on 2.6 kernels due to TSO.
(with TSO on the number of packets that actually enter the Xmit routine would be reduced apprx 40 times).
.....

This was with a 2.6 kernel (with TSO on). PIO reads are pretty expensive on Altix, so eliminating them really helps us.

For big mtus (>=4KBytes) the benefit of replacing the readq() with mmiowb() in s2io_xmit() is negligible.

--
Arthur

Christoph Hellwig

2005-07-12 20:27 UTC #8 lore raw

In reply to [email protected]

drivers/net/s2io.h line 19

Quoted 8 lines

3. Enable two-buffer mode(for Rx path) automatically for SGI systems. This improves Rx performance dramatically on SGI systems.
drivers/net/s2io.h +4 -0
+/* Enable 2 buffer mode by default for SGI system */
+#ifdef CONFIG_IA64_SGI_SN2
+#define CONFIG_2BUFF_MODE
+#endif

this enabled it only on kernel that are built to only run on SN2 hardware, which is completely useless in practice. Besides that defining a CONFIG_ symbol from source files is a big no-go.

What exactly is the 2buff mode, and more specific what are the downsides of enabling it on non-SGI hardware?

David S. Miller

maintainer

2005-07-12 20:34 UTC #9 lore raw

From: Christoph Hellwig <[email protected]>
Date: Tue, 12 Jul 2005 21:27:54 +0100

Quoted 8 lines

drivers/net/s2io.h line 19
Quoted 4 lines
drivers/net/s2io.h +4 -0
+/* Enable 2 buffer mode by default for SGI system */
+#ifdef CONFIG_IA64_SGI_SN2
+#define CONFIG_2BUFF_MODE
+#endif
this enabled it only on kernel that are built to only run on SN2 hardware, which is completely useless in practice. Besides that defining a CONFIG_ symbol from source files is a big no-go.

Yes, do this in the Kconfig file instead.

Ravinandan Arakali

2005-07-12 21:00 UTC #10 lore raw

The two-buffer mode was added as a configurable option to Kconfig file several months ago. Hence the macro is CONFIG_2BUFF_MODE.

The two-buffer receive mode involves two buffers (128 byte aligned) for each packet. This mode drastically increases performance on SGI platforms and hence enabled only for these platforms. On other platforms, there's no difference compared to one-buffer mode but the added complexity and extra memory allocated does not make it worthwhile to enable this mode for non-SGI platforms. Also, most of our QA cycle on non-SGI platforms has been done with one-buffer mode.

Thanks,
Ravi

Quoted 8 lines

drivers/net/s2io.h line 19
Quoted 4 lines
drivers/net/s2io.h +4 -0
+/* Enable 2 buffer mode by default for SGI system */
+#ifdef CONFIG_IA64_SGI_SN2
+#define CONFIG_2BUFF_MODE
+#endif
this enabled it only on kernel that are built to only run on SN2 hardware, which is completely useless in practice. Besides that defining a CONFIG_ symbol from source files is a big no-go.

Yes, do this in the Kconfig file instead.

David S. Miller

maintainer

2005-07-12 21:04 UTC #11 lore raw

From: "Ravinandan Arakali" <[email protected]>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700

Quoted 3 lines

The two-buffer mode was added as a configurable option to Kconfig file several months ago. Hence the macro is CONFIG_2BUFF_MODE.

We're saying that you should choose CONFIG_2BUFF_MODE, when CONFIG_IA64_SGI_SN2 is set, inside the Kconfig file using the "default" Kconfig directive.

You should never change the setting of CONFIG_* macros in C source.

Christoph Hellwig

2005-07-12 21:07 UTC #12 lore raw

Quoted 11 lines · David S. Miller

From: "Ravinandan Arakali" <[email protected]>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700
Quoted 3 lines
The two-buffer mode was added as a configurable option to Kconfig file several months ago. Hence the macro is CONFIG_2BUFF_MODE.
We're saying that you should choose CONFIG_2BUFF_MODE, when CONFIG_IA64_SGI_SN2 is set, inside the Kconfig file using the "default" Kconfig directive.

And that doesn't help either, CONFIG_IA64_SGI_SN2 isn't used in practice, any production Altix with 26 kernels runs CONFIG_IA64_GENERIC kernels.

You have to make this run-time selectable.

Andi Kleen

2005-07-12 21:26 UTC #13 lore raw

Quoted 3 lines · Christoph Hellwig

And that doesn't help either, CONFIG_IA64_SGI_SN2 isn't used in practice, any production Altix with 26 kernels runs CONFIG_IA64_GENERIC kernels.

At least SLES9/ia64 has a SN2 kernel.

-Andi

Ravinandan Arakali

2005-07-12 21:54 UTC #14 lore raw

In reply to David S. Miller

Okay, got it. Will submit patch on Kconfig file after the macro for SGI platforms is confirmed.

Ravi

-----Original Message-----
From: David S. Miller [mailto:[email protected]]
Sent: Tuesday, July 12, 2005 2:04 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

From: "Ravinandan Arakali" <[email protected]>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700

Quoted 3 lines

The two-buffer mode was added as a configurable option to Kconfig file several months ago. Hence the macro is CONFIG_2BUFF_MODE.

We're saying that you should choose CONFIG_2BUFF_MODE, when CONFIG_IA64_SGI_SN2 is set, inside the Kconfig file using the "default" Kconfig directive.

You should never change the setting of CONFIG_* macros in C source.

Ravinandan Arakali

2005-07-29 16:37 UTC #15 lore raw

In reply to David S. Miller

David,
We are trying to use the "default" directive in Kconfig. We tried using an unconditional directive(just to test it out) such as "default y" and a conditional one such as "default y if CONFIG_IA64_SGI_SN2".

But when we run "make menuconfig", it does not seem to pickup any of these changes from Kconfig.
Any idea what we might be missing ?

Once this is fixed, we'll send out a patch to address comments from previous 12 patches as well as couple of issues we found in the meantime.

Thanks,
Ravi

From: "Ravinandan Arakali" <[email protected]>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700

Quoted 3 lines

The two-buffer mode was added as a configurable option to Kconfig file several months ago. Hence the macro is CONFIG_2BUFF_MODE.

We're saying that you should choose CONFIG_2BUFF_MODE, when CONFIG_IA64_SGI_SN2 is set, inside the Kconfig file using the "default" Kconfig directive.

You should never change the setting of CONFIG_* macros in C source.

Christoph Hellwig

2005-07-31 14:05 UTC #16 lore raw

Quoted 5 lines · Ravinandan Arakali

David,
We are trying to use the "default" directive in Kconfig. We tried using an unconditional directive(just to test it out) such as "default y" and a conditional one such as "default y if CONFIG_IA64_SGI_SN2".

Again, please make this a module option, CONFIG_IA64_SGI_SN2 does not mean runs on Altix but that this is a kernel that only supports Altix, which is a non-standard case that doesn't cover 90% or more of actual Altix systems in the field and running 2.6.

Ravinandan Arakali

2005-08-02 23:13 UTC #17 lore raw

Hi Christoph,
Following is SGI's stand on this issue:

SGI recommends that customers use the -sn2 kernel. This is the kernel that is installed by our factory when we ship systems. The -sn2 kernel is also the kernel that must be run if the Altix has more that 128 CPUs. So I'd be surprised it the majority of the Altix systems in the field are not running the -sn2 kernel.

Thanks,
Ravi

-----Original Message-----
From: Christoph Hellwig [mailto:[email protected]]
Sent: Sunday, July 31, 2005 7:05 AM
To: Ravinandan Arakali
Cc: 'David S. Miller'; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

Quoted 5 lines · Ravinandan Arakali

David,
We are trying to use the "default" directive in Kconfig. We tried using an unconditional directive(just to test it out) such as "default y" and a conditional one such as "default y if CONFIG_IA64_SGI_SN2".

'Christoph Hellwig'

2005-08-02 23:26 UTC #18 lore raw

Quoted 8 lines · Ravinandan Arakali

Hi Christoph,
Following is SGI's stand on this issue:
SGI recommends that customers use the -sn2 kernel. This is the kernel that is installed by our factory when we ship systems. The -sn2 kernel is also the kernel that must be run if the Altix has more that 128 CPUs. So I'd be surprised it the majority of the Altix systems in the field are not running the -sn2 kernel.

The my argument is wrong for SuSE ;-) This still needs to be a runtime switch though, not just for this reason. Platform ifdefs are not the way to go.

Leonid Grossman

2005-07-12 20:56 UTC #19 lore raw

In reply to Christoph Hellwig

Quoted 23 lines

-----Original Message-----
From: Christoph Hellwig [mailto:[email protected]]
Sent: Tuesday, July 12, 2005 1:28 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Review
Quoted 6 lines
3. Enable two-buffer mode(for Rx path) automatically for SGI systems. This improves Rx performance dramatically on SGI systems.
(quoted diff) +2 -0
+/* Enable 2 buffer mode by default for SGI system */ #ifdef 
+CONFIG_IA64_SGI_SN2 #define CONFIG_2BUFF_MODE #endif
this enabled it only on kernel that are built to only run on SN2 hardware, which is completely useless in practice. Besides that defining a CONFIG_ symbol from source files is a big no-go.

Quoted 2 lines

What exactly is the 2buff mode, and more specific what are the downsides of enabling it on non-SGI hardware?

In short, this is one of the ASIC modes where headers and payload are separated by the hardware, and placed in separate receive buffers. (More details are in the ASIC programming manual that is posted on ns1.s2io.com). On SGI platforms, the two buffer mode results in a significant rx performance boost since it allows to achieve both aligned transfers on the bus and aligned data copies.
It has been tested on Altix systems quite a bit (the card is OEMed and shipped by SGI).
On other platforms, we haven't seen significant benefits from header separation modes (unless they are combined with different features, which is another story), and we decided not to introduce an extra bus transfer unless it is clearly justified.
Also, the feature has not been sufficiently tested on other platforms, For these reasons, it is left as a feature that's specific to SGI chipset - but it is really beneficial there.

Leonid

Quoted 2 lines