[PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Arthur Kepner]

The neterion 10gige driver uses a readq() to flush some PIO writes
in s2io_xmit(). Using mmiowb() can, in some cases, reduce CPU 
utilization, and/or allow higher throughput. This is particularly 
true when TSO is off, and small MTUs are in use. 

For example, in one test measurement I just did with 2.6.12-rc2
on an Altix, MTUs were set to 1500 bytes and TSO turned off. 
With this patch, transmit throughput improved by ~20%. Throughput 
was ultimately bound by the CPU with or without the patch. With 
large MTUs (9600 bytes) or with TSO turned on, there was no 
significant change to throughput or CPU utilization.

Signed-off-by: Arthur Kepner <akepner@sgi.com>

--- linux.orig/drivers/net/s2io.c	2005-05-02 16:40:17.469733509 -0700
+++ linux/drivers/net/s2io.c	2005-05-02 16:40:25.001043632 -0700
@@ -2759,8 +2759,8 @@ static int s2io_xmit(struct sk_buff *skb
 #endif
 	writeq(val64, &tx_fifo->List_Control);
 
-	/* Perform a PCI read to flush previous writes */
-	val64 = readq(&bar0->general_int_status);
+	/* Perform a mmiowb() to order previous writes */
+	mmiowb();
 
 	put_off++;
 	put_off %= mac_control->tx_curr_put_info[queue].fifo_len + 1;

--
Arthur

Re: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Anton Blanchard]

 
Hi,

> The neterion 10gige driver uses a readq() to flush some PIO writes
> in s2io_xmit(). Using mmiowb() can, in some cases, reduce CPU 
> utilization, and/or allow higher throughput. This is particularly 
> true when TSO is off, and small MTUs are in use. 
> 
> For example, in one test measurement I just did with 2.6.12-rc2
> on an Altix, MTUs were set to 1500 bytes and TSO turned off. 
> With this patch, transmit throughput improved by ~20%. Throughput 
> was ultimately bound by the CPU with or without the patch. With 
> large MTUs (9600 bytes) or with TSO turned on, there was no 
> significant change to throughput or CPU utilization.

I didnt know mmiowb is supposed to be a replacement for PCI write
posting. Most architectures define mmiowb as nothing and so will be
broken with your change.

Anton

> --- linux.orig/drivers/net/s2io.c	2005-05-02 16:40:17.469733509 -0700
> +++ linux/drivers/net/s2io.c	2005-05-02 16:40:25.001043632 -0700
> @@ -2759,8 +2759,8 @@ static int s2io_xmit(struct sk_buff *skb
>  #endif
>  	writeq(val64, &tx_fifo->List_Control);
>  
> -	/* Perform a PCI read to flush previous writes */
> -	val64 = readq(&bar0->general_int_status);
> +	/* Perform a mmiowb() to order previous writes */
> +	mmiowb();
>  
>  	put_off++;
>  	put_off %= mac_control->tx_curr_put_info[queue].fifo_len + 1;
> 
> --
> Arthur
> 

Re: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Arthur Kepner]

On Tue, 3 May 2005, Anton Blanchard wrote:

> 
> I didnt know mmiowb is supposed to be a replacement for PCI write
> posting. Most architectures define mmiowb as nothing and so will be
> broken with your change.
> 

I thought that an arch was supposed to define mmiowb() if it needs 
it. In this case, the readq() is only being used for the side-effect 
of ordering the previous writes (the neterion folks can correct me 
if I'm wrong). 

--
Arthur

RE: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Ramkrishna Vepa]

> > I didnt know mmiowb is supposed to be a replacement for PCI write
> > posting. Most architectures define mmiowb as nothing and so will be
> > broken with your change.
> >
> 
> I thought that an arch was supposed to define mmiowb() if it needs
> it. In this case, the readq() is only being used for the side-effect
> of ordering the previous writes (the neterion folks can correct me
> if I'm wrong).
[Ram] Yes, it is to ensure ordering as well as the flush of the io that is
required in some platforms.
 
> 
> --
> Arthur

Re: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Anton Blanchard]

 
Hi,

> I thought that an arch was supposed to define mmiowb() if it needs 
> it. In this case, the readq() is only being used for the side-effect 
> of ordering the previous writes (the neterion folks can correct me 
> if I'm wrong). 

The documentation suggests mmiowb does not do write posting:

      <para>
        In addition to write posting, on some large multiprocessing systems
        (e.g. SGI Challenge, Origin and Altix machines) posted writes won't
        be strongly ordered coming from different CPUs.  Thus it's important
        to properly protect parts of your driver that do memory-mapped writes
        with locks and use the <function>mmiowb</function> to make sure they
        arrive in the order intended.  Issuing a regular <function>readX
        </function> will also ensure write ordering, but should only be used
        when the driver has to be sure that the write has actually arrived
        at the device (not that it's simply ordered with respect to other
        writes), since a full <function>readX</function> is a relatively
        expensive operation.
      </para>

So if we require the write to be posted we will need the read. If
however we only need to order wrt subsequent IO writes then I agree
mmiowb can be used.

Anton

Re: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Anton Blanchard]

> [Ram] Yes, it is to ensure ordering as well as the flush of the io that is
> required in some platforms.

If the flush of the IO is required I cant see how we can remove the read.

Anton

Re: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Arthur Kepner]

On Tue, 3 May 2005, Anton Blanchard wrote:

> 
> > [Ram] Yes, it is to ensure ordering as well as the flush of the io that is
> > required in some platforms.
> 
> If the flush of the IO is required I cant see how we can remove the read.
> 

Agreed. Can someone from neterion comment? Is a flush required, or is 
ordering all that's required here?

--
Arthur

RE: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()

[Ramkrishna Vepa]

Ordering is all that is required. We can get rid of the flush.

Ram

> -----Original Message-----
> From: Arthur Kepner [mailto:akepner@sgi.com]
> Sent: Tuesday, May 03, 2005 7:52 AM
> To: Anton Blanchard
> Cc: Ramkrishna Vepa; 'Leonid Grossman'; muhammad.shafiq@neterion.com;
> ramkrishna.vepa@neterion.com; netdev@oss.sgi.com; 'Ravinandan Arakali'
> Subject: Re: [PATCH] s2io: replace readq() with mmiowb() in s2io_xmit()
> 
> On Tue, 3 May 2005, Anton Blanchard wrote:
> 
> >
> > > [Ram] Yes, it is to ensure ordering as well as the flush of the io
> that is
> > > required in some platforms.
> >
> > If the flush of the IO is required I cant see how we can remove the
> read.
> >
> 
> Agreed. Can someone from neterion comment? Is a flush required, or is
> ordering all that's required here?
> 
> --
> Arthur