sata_vsc.c cache line size question

sata_vsc.c cache line size question

[Dailey, Nate]

Hoping someone on this list might shed some light on this...

I was investigating a problem of poor sequential write performance
(IOmeter, various size sequential writes) with an embedded Vitesse 7174,
maxing out (with disk write cache on) at around 10 MB/s...

After noticing that Windows on the same hardware was using 0x10 for the
cache line size, but Linux was using 0x80, I tried removing the
following from sata_vsc.c:

381         /*
382          * Due to a bug in the chip, the default cache line size
can't be used
383          */
384         pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);

Now, with cache line size the same as Windows, Linux is doing more like
43 MB/s.

Just wondering what the deal with this "bug in the chip" might be, since
for me it seems that the default cache line size is better? If there's a
real bug, I don't want to do anything dangerous by removing this code
(though I've heard--haven't seen the code--that the Windows driver
doesn't touch the cache line size, nor does the Linux non-libata
reference driver from Vitesse).

Thanks!
Nate

Re: sata_vsc.c cache line size question

[Jeremy Higdon]

On Fri, Jan 12, 2007 at 02:45:23PM -0500, Dailey, Nate wrote:
> Hoping someone on this list might shed some light on this...
> 
> I was investigating a problem of poor sequential write performance
> (IOmeter, various size sequential writes) with an embedded Vitesse 7174,
> maxing out (with disk write cache on) at around 10 MB/s...
> 
> After noticing that Windows on the same hardware was using 0x10 for the
> cache line size, but Linux was using 0x80, I tried removing the
> following from sata_vsc.c:
> 
> 381         /*
> 382          * Due to a bug in the chip, the default cache line size
> can't be used
> 383          */
> 384         pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> 
> Now, with cache line size the same as Windows, Linux is doing more like
> 43 MB/s.
> 
> Just wondering what the deal with this "bug in the chip" might be, since
> for me it seems that the default cache line size is better? If there's a
> real bug, I don't want to do anything dangerous by removing this code
> (though I've heard--haven't seen the code--that the Windows driver
> doesn't touch the cache line size, nor does the Linux non-libata
> reference driver from Vitesse).


The problem is that it can't be zero, which is the default value
after reset.

So I suppose the driver should be modified to set it to 0x80 only
if it's 0.  I believe that most PCI implementations will set it in
the BIOS or whatever.

Care to send a patch?

jeremy

Re: sata_vsc.c cache line size question

[Alan]

> So I suppose the driver should be modified to set it to 0x80 only
> if it's 0.  I believe that most PCI implementations will set it in
> the BIOS or whatever.

pci_set_master should handle this automtically on most platforms.

On x86-32 you also need to check "pcibios_max_latency" if you write the
latency yourself directly AND the card could be plugged into an arbitary
PCI slot on a 32bit PC.

Alan

Re: sata_vsc.c cache line size question

[Jeff Garzik]

Alan wrote:
>> So I suppose the driver should be modified to set it to 0x80 only
>> if it's 0.  I believe that most PCI implementations will set it in
>> the BIOS or whatever.
> 
> pci_set_master should handle this automtically on most platforms.

pci_set_master() sets the latency timer not cacheline size

pci_set_mwi() on x86-ish platforms sets cacheline size.

	Jeff

RE: sata_vsc.c cache line size question

[Dailey, Nate]

Here's a patch that does what you suggest.

Because the default cache line size on my system is 0x10, I tested the
patch by checking against this value rather than 0... it worked as
expected.

This patch is against 2.6.19.2 that I just downloaded from kernel.org. I
actually tested on RHEL4 update 4, a 2.6.9 kernel, but I'll try building
the 2.6.19.2 on my system to make sure it works in that version as well.

Nate


--- sata_vsc.c.orig     2007-01-15 11:06:17.000000000 -0500
+++ sata_vsc.c  2007-01-15 11:10:29.000000000 -0500
@@ -340,6 +340,7 @@ static int __devinit vsc_sata_init_one (
        int pci_dev_busy = 0;
        void __iomem *mmio_base;
        int rc;
+       u8 cls;

        if (!printed_version++)
                dev_printk(KERN_DEBUG, &pdev->dev, "version "
DRV_VERSION "\n");
@@ -389,9 +390,13 @@ static int __devinit vsc_sata_init_one (
        base = (unsigned long) mmio_base;

        /*
-        * Due to a bug in the chip, the default cache line size can't
be used
+        * Due to a bug in the chip, the default cache line size can't
be
+        * used (unless the default is non-zero).
         */
-       pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
+       pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cls);
+       if (cls == 0x00) {
+               pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
+       }

        probe_ent->sht = &vsc_sata_sht;
        probe_ent->port_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |





-----Original Message-----
From: Jeremy Higdon [mailto:jeremy@sgi.com] 
Sent: Sunday, January 14, 2007 3:03 AM
To: Dailey, Nate
Cc: linux-ide@vger.kernel.org
Subject: Re: sata_vsc.c cache line size question

On Fri, Jan 12, 2007 at 02:45:23PM -0500, Dailey, Nate wrote:
> Hoping someone on this list might shed some light on this...
> 
> I was investigating a problem of poor sequential write performance
> (IOmeter, various size sequential writes) with an embedded Vitesse
7174,
> maxing out (with disk write cache on) at around 10 MB/s...
> 
> After noticing that Windows on the same hardware was using 0x10 for
the
> cache line size, but Linux was using 0x80, I tried removing the
> following from sata_vsc.c:
> 
> 381         /*
> 382          * Due to a bug in the chip, the default cache line size
> can't be used
> 383          */
> 384         pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> 
> Now, with cache line size the same as Windows, Linux is doing more
like
> 43 MB/s.
> 
> Just wondering what the deal with this "bug in the chip" might be,
since
> for me it seems that the default cache line size is better? If there's
a
> real bug, I don't want to do anything dangerous by removing this code
> (though I've heard--haven't seen the code--that the Windows driver
> doesn't touch the cache line size, nor does the Linux non-libata
> reference driver from Vitesse).


The problem is that it can't be zero, which is the default value
after reset.

So I suppose the driver should be modified to set it to 0x80 only
if it's 0.  I believe that most PCI implementations will set it in
the BIOS or whatever.

Care to send a patch?

jeremy

Re: sata_vsc.c cache line size question

[Jeremy Higdon]

On Mon, Jan 15, 2007 at 11:26:59AM -0500, Dailey, Nate wrote:
> Here's a patch that does what you suggest.
> 
> Because the default cache line size on my system is 0x10, I tested the
> patch by checking against this value rather than 0... it worked as
> expected.
> 
> This patch is against 2.6.19.2 that I just downloaded from kernel.org. I
> actually tested on RHEL4 update 4, a 2.6.9 kernel, but I'll try building
> the 2.6.19.2 on my system to make sure it works in that version as well.
> 
> Nate

Hi Nate,

The patch looks fine, except that your mailer wrapped the lines.
Try sending it as a text attachment instead.

Thanks

jeremy

> 
> --- sata_vsc.c.orig     2007-01-15 11:06:17.000000000 -0500
> +++ sata_vsc.c  2007-01-15 11:10:29.000000000 -0500
> @@ -340,6 +340,7 @@ static int __devinit vsc_sata_init_one (
>         int pci_dev_busy = 0;
>         void __iomem *mmio_base;
>         int rc;
> +       u8 cls;
> 
>         if (!printed_version++)
>                 dev_printk(KERN_DEBUG, &pdev->dev, "version "
> DRV_VERSION "\n");
> @@ -389,9 +390,13 @@ static int __devinit vsc_sata_init_one (
>         base = (unsigned long) mmio_base;
> 
>         /*
> -        * Due to a bug in the chip, the default cache line size can't
> be used
> +        * Due to a bug in the chip, the default cache line size can't
> be
> +        * used (unless the default is non-zero).
>          */
> -       pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> +       pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cls);
> +       if (cls == 0x00) {
> +               pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> +       }
> 
>         probe_ent->sht = &vsc_sata_sht;
>         probe_ent->port_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |

RE: sata_vsc.c cache line size question

[Dailey, Nate]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Okay, trying again as an attachment...

Nate

 

-----Original Message-----
From: Jeremy Higdon [mailto:jeremy@sgi.com] 
Sent: Thursday, January 18, 2007 2:12 AM
To: Dailey, Nate
Cc: linux-ide@vger.kernel.org
Subject: Re: sata_vsc.c cache line size question

On Mon, Jan 15, 2007 at 11:26:59AM -0500, Dailey, Nate wrote:
> Here's a patch that does what you suggest.
> 
> Because the default cache line size on my system is 0x10, I tested the
> patch by checking against this value rather than 0... it worked as
> expected.
> 
> This patch is against 2.6.19.2 that I just downloaded from kernel.org.
I
> actually tested on RHEL4 update 4, a 2.6.9 kernel, but I'll try
building
> the 2.6.19.2 on my system to make sure it works in that version as
well.
> 
> Nate

Hi Nate,

The patch looks fine, except that your mailer wrapped the lines.
Try sending it as a text attachment instead.

Thanks

jeremy

> 
> --- sata_vsc.c.orig     2007-01-15 11:06:17.000000000 -0500
> +++ sata_vsc.c  2007-01-15 11:10:29.000000000 -0500
> @@ -340,6 +340,7 @@ static int __devinit vsc_sata_init_one (
>         int pci_dev_busy = 0;
>         void __iomem *mmio_base;
>         int rc;
> +       u8 cls;
> 
>         if (!printed_version++)
>                 dev_printk(KERN_DEBUG, &pdev->dev, "version "
> DRV_VERSION "\n");
> @@ -389,9 +390,13 @@ static int __devinit vsc_sata_init_one (
>         base = (unsigned long) mmio_base;
> 
>         /*
> -        * Due to a bug in the chip, the default cache line size can't
> be used
> +        * Due to a bug in the chip, the default cache line size can't
> be
> +        * used (unless the default is non-zero).
>          */
> -       pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> +       pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cls);
> +       if (cls == 0x00) {
> +               pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE,
0x80);
> +       }
> 
>         probe_ent->sht = &vsc_sata_sht;
>         probe_ent->port_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |

[-- Attachment #2: sata_vsc_cls.patch --]
[-- Type: application/octet-stream, Size: 929 bytes --]

--- sata_vsc.c.orig	2007-01-18 09:17:00.000000000 -0500
+++ sata_vsc.c	2007-01-18 09:31:48.000000000 -0500
@@ -340,6 +340,7 @@ static int __devinit vsc_sata_init_one (
 	int pci_dev_busy = 0;
 	void __iomem *mmio_base;
 	int rc;
+	u8 cls;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
@@ -389,9 +390,13 @@ static int __devinit vsc_sata_init_one (
 	base = (unsigned long) mmio_base;
 
 	/*
-	 * Due to a bug in the chip, the default cache line size can't be used
+	 * Due to a bug in the chip, the default cache line size can't be
+	 * used (unless the default is non-zero).
 	 */
-	pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
+	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cls);
+	if (cls == 0x00) {
+		pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
+	}
 
 	probe_ent->sht = &vsc_sata_sht;
 	probe_ent->port_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |

Re: sata_vsc.c cache line size question

[Jeremy Higdon]

On Thu, Jan 18, 2007 at 09:42:44AM -0500, Dailey, Nate wrote:
> Okay, trying again as an attachment...
> 
> Nate

Hi Nate, sorry about the delay.  This patch works fine.  Please see
http://linux.yyz.us/patch-format.html and send a new patch, if you would.
Then presumably jgarzik will take it, since it's a good fix.
Or if you want, I could generate the new patch.

Thanks,

jeremy



> 
> -----Original Message-----
> From: Jeremy Higdon [mailto:jeremy@sgi.com] 
> Sent: Thursday, January 18, 2007 2:12 AM
> To: Dailey, Nate
> Cc: linux-ide@vger.kernel.org
> Subject: Re: sata_vsc.c cache line size question
> 
> On Mon, Jan 15, 2007 at 11:26:59AM -0500, Dailey, Nate wrote:
> > Here's a patch that does what you suggest.
> > 
> > Because the default cache line size on my system is 0x10, I tested the
> > patch by checking against this value rather than 0... it worked as
> > expected.
> > 
> > This patch is against 2.6.19.2 that I just downloaded from kernel.org.
> I
> > actually tested on RHEL4 update 4, a 2.6.9 kernel, but I'll try
> building
> > the 2.6.19.2 on my system to make sure it works in that version as
> well.
> > 
> > Nate
> 
> Hi Nate,
> 
> The patch looks fine, except that your mailer wrapped the lines.
> Try sending it as a text attachment instead.
> 
> Thanks
> 
> jeremy