Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Prarit Bhargava]

> On Tue, Aug 02, 2005 at 04:13:57PM -0700, Ravinandan Arakali wrote:
>> 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.

Hi Ravinandan,

Ditto for SGI Altix on Red Hat -- platform specific ifdefs are not the proper 
way to do this.

P.

[PATCH 2.6.12.1 5/12] S2io: Performance improvements

[raghavendra.koushik]

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 <ravinandan.arakali@neterion.com>
Signed-off-by: Raghavendra Koushik <raghavendra.koushik@neterion.com>
---
diff -uprN vanilla_kernel/drivers/net/s2io.c linux-2.6.12-rc6/drivers/net/s2io.c
--- vanilla_kernel/drivers/net/s2io.c	2005-06-28 02:00:08.000000000 -0700
+++ linux-2.6.12-rc6/drivers/net/s2io.c	2005-06-28 02:00:18.000000000 -0700
@@ -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]);
 	}
diff -uprN vanilla_kernel/drivers/net/s2io.h linux-2.6.12-rc6/drivers/net/s2io.h
--- vanilla_kernel/drivers/net/s2io.h	2005-06-28 02:00:08.000000000 -0700
+++ linux-2.6.12-rc6/drivers/net/s2io.h	2005-06-28 02:00:18.000000000 -0700
@@ -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))

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Arthur Kepner]

On Thu, 7 Jul 2005 raghavendra.koushik@neterion.com 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.			    
> ......
> @@ -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++;
                                                                  
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

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Raghavendra Koushik]

> 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.


> 
> 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).

> What is the wmb() above for?
wmb() is to ensure ordered PIO writes.

Thanks
- Koushik

 

> -----Original Message-----
> From: Arthur Kepner [mailto:akepner@sgi.com] 
> Sent: Thursday, July 07, 2005 4:15 PM
> To: raghavendra.koushik@neterion.com
> Cc: jgarzik@pobox.com; netdev@oss.sgi.com; 
> netdev@vger.kernel.org; ravinandan.arakali@neterion.com; 
> leonid.grossman@neterion.com; rapuru.sriram@neterion.com
> Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
> 
> 
> On Thu, 7 Jul 2005 raghavendra.koushik@neterion.com 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.		
> 	    
> > ......
> > @@ -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++;
>                                                                   
> 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
> 

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[David S. Miller]

From: "Raghavendra Koushik" <raghavendra.koushik@neterion.com>
Date: Thu, 7 Jul 2005 18:06:19 -0700

> 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.

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Jeff Garzik]

David S. Miller wrote:
> 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

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Arthur Kepner]

On Thu, 7 Jul 2005, Raghavendra Koushik wrote:

> ....
> 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.

> 
> > 
> > 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).
> .....

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

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Raghavendra Koushik]

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

-Koushik

> -----Original Message-----
> From: Arthur Kepner [mailto:akepner@sgi.com] 
> Sent: Friday, July 08, 2005 8:31 AM
> To: Raghavendra Koushik
> Cc: jgarzik@pobox.com; netdev@oss.sgi.com; 
> netdev@vger.kernel.org; ravinandan.arakali@neterion.com; 
> leonid.grossman@neterion.com; rapuru.sriram@neterion.com
> Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
> 
> On Thu, 7 Jul 2005, Raghavendra Koushik wrote:
> 
> > ....
> > 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.
> 
> > 
> > > 
> > > 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).
> > .....
> 
> 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
> 

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Ravinandan Arakali]

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:akepner@sgi.com]
Sent: Friday, July 08, 2005 8:31 AM
To: Raghavendra Koushik
Cc: jgarzik@pobox.com; netdev@oss.sgi.com; netdev@vger.kernel.org;
ravinandan.arakali@neterion.com; leonid.grossman@neterion.com;
rapuru.sriram@neterion.com
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements


On Thu, 7 Jul 2005, Raghavendra Koushik wrote:

> ....
> 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.

> 
> > 
> > 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).
> .....

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

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Christoph Hellwig]

> 3. Enable two-buffer mode(for Rx path) automatically for SGI
>    systems. This improves Rx performance dramatically on 
>    SGI systems.

> +/* 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?

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[David S. Miller]

From: Christoph Hellwig <hch@infradead.org>
Date: Tue, 12 Jul 2005 21:27:54 +0100

> > +/* 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.

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Ravinandan Arakali]

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


> > +/* 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.

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[David S. Miller]

From: "Ravinandan Arakali" <ravinandan.arakali@neterion.com>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700

> 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.

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Christoph Hellwig]

On Tue, Jul 12, 2005 at 02:04:11PM -0700, David S. Miller wrote:
> From: "Ravinandan Arakali" <ravinandan.arakali@neterion.com>
> Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
> Date: Tue, 12 Jul 2005 14:00:52 -0700
> 
> > 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.

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Andi Kleen]

Christoph Hellwig <hch@infradead.org> writes:
> 
> 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

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Ravinandan Arakali]

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:davem@davemloft.net]
Sent: Tuesday, July 12, 2005 2:04 PM
To: ravinandan.arakali@neterion.com
Cc: hch@infradead.org; raghavendra.koushik@neterion.com;
jgarzik@pobox.com; netdev@oss.sgi.com; leonid.grossman@neterion.com;
rapuru.sriram@neterion.com
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements


From: "Ravinandan Arakali" <ravinandan.arakali@neterion.com>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700

> 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.

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[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".

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

-----Original Message-----
From: David S. Miller [mailto:davem@davemloft.net]
Sent: Tuesday, July 12, 2005 2:04 PM
To: ravinandan.arakali@neterion.com
Cc: hch@infradead.org; raghavendra.koushik@neterion.com;
jgarzik@pobox.com; netdev@oss.sgi.com; leonid.grossman@neterion.com;
rapuru.sriram@neterion.com
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements


From: "Ravinandan Arakali" <ravinandan.arakali@neterion.com>
Subject: RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
Date: Tue, 12 Jul 2005 14:00:52 -0700

> 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.

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Christoph Hellwig]

On Fri, Jul 29, 2005 at 09:37:55AM -0700, Ravinandan Arakali wrote:
> 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.

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[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.

Thanks,
Ravi

-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org]
Sent: Sunday, July 31, 2005 7:05 AM
To: Ravinandan Arakali
Cc: 'David S. Miller'; hch@infradead.org;
raghavendra.koushik@neterion.com; jgarzik@pobox.com; netdev@oss.sgi.com;
leonid.grossman@neterion.com; rapuru.sriram@neterion.com
Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements


On Fri, Jul 29, 2005 at 09:37:55AM -0700, Ravinandan Arakali wrote:
> 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.

Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

['Christoph Hellwig']

On Tue, Aug 02, 2005 at 04:13:57PM -0700, Ravinandan Arakali wrote:
> 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.

RE: [PATCH 2.6.12.1 5/12] S2io: Performance improvements

[Leonid Grossman]

 

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org] 
> Sent: Tuesday, July 12, 2005 1:28 PM
> To: raghavendra.koushik@neterion.com
> Cc: jgarzik@pobox.com; netdev@oss.sgi.com; 
> ravinandan.arakali@neterion.com; 
> leonid.grossman@neterion.com; rapuru.sriram@neterion.com
> Subject: Re: [PATCH 2.6.12.1 5/12] S2io: Performance improvements
> 
> > 3. Enable two-buffer mode(for Rx path) automatically for SGI
> >    systems. This improves Rx performance dramatically on 
> >    SGI systems.
> 
> > +/* 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?

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



> 
>