[X86] PCI: Use generic cacheline sizing instead of per-vendor tests.

[X86] PCI: Use generic cacheline sizing instead of per-vendor tests.

[Dave Jones]

Instead of the PCI code needing to have code to determine the 
cacheline size of each processor, use the data the cpu identification
code should have already determined during early boot.

(The vendor checks are also incomplete, and don't take into account
 modern CPUs)

I've been carrying a variant of this code in Fedora for a while,
that prints debug information.  There are a number of cases where we
are currently setting the PCI cacheline size to 32 bytes, when the CPU
cacheline size is 64 bytes.  With this patch, we set them both the same.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 1331fcf..b9f9373 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -422,15 +422,20 @@ int __init pcibios_init(void)
 	}
 
 	/*
-	 * Assume PCI cacheline size of 32 bytes for all x86s except K7/K8
-	 * and P4. It's also good for 386/486s (which actually have 16)
+	 * Set PCI cacheline size to that of the CPU if the CPU has reported it.
+	 * (For older CPUs that don't support cpuid, we se it to 32 bytes
+	 * It's also good for 386/486s (which actually have 16)
 	 * as quite a few PCI devices do not support smaller values.
 	 */
-	pci_cache_line_size = 32 >> 2;
-	if (c->x86 >= 6 && c->x86_vendor == X86_VENDOR_AMD)
-		pci_cache_line_size = 64 >> 2;	/* K7 & K8 */
-	else if (c->x86 > 6 && c->x86_vendor == X86_VENDOR_INTEL)
-		pci_cache_line_size = 128 >> 2;	/* P4 */
+
+	if (c->x86_clflush_size > 0) {
+		pci_cache_line_size = c->x86_clflush_size >> 2;
+		printk(KERN_DEBUG "PCI: pci_cache_line_size set to %d bytes\n",
+			pci_cache_line_size << 2);
+	} else {
+		pci_cache_line_size = 32 >> 2;
+		printk(KERN_DEBUG "PCI: Unknown cacheline size. Setting to 32 bytes\n");
+	}
 
 	pcibios_resource_survey();
 

Re: [X86] PCI: Use generic cacheline sizing instead of per-vendor tests.

[Jesse Barnes]

On Wed, 14 Oct 2009 16:31:39 -0400
Dave Jones <davej@redhat.com> wrote:

> Instead of the PCI code needing to have code to determine the 
> cacheline size of each processor, use the data the cpu identification
> code should have already determined during early boot.
> 
> (The vendor checks are also incomplete, and don't take into account
>  modern CPUs)
> 
> I've been carrying a variant of this code in Fedora for a while,
> that prints debug information.  There are a number of cases where we
> are currently setting the PCI cacheline size to 32 bytes, when the CPU
> cacheline size is 64 bytes.  With this patch, we set them both the
> same.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 

Does this improve performance enough to warrant putting it into the
current cycle?  Or is queuing it for 2.6.33 sufficient?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [X86] PCI: Use generic cacheline sizing instead of per-vendor tests.

[Dave Jones]

On Wed, Oct 14, 2009 at 02:30:54PM -0700, Jesse Barnes wrote:
 > On Wed, 14 Oct 2009 16:31:39 -0400
 > Dave Jones <davej@redhat.com> wrote:
 > 
 > > Instead of the PCI code needing to have code to determine the 
 > > cacheline size of each processor, use the data the cpu identification
 > > code should have already determined during early boot.
 > > 
 > > (The vendor checks are also incomplete, and don't take into account
 > >  modern CPUs)
 > > 
 > > I've been carrying a variant of this code in Fedora for a while,
 > > that prints debug information.  There are a number of cases where we
 > > are currently setting the PCI cacheline size to 32 bytes, when the CPU
 > > cacheline size is 64 bytes.  With this patch, we set them both the
 > > same.
 > > 
 > > Signed-off-by: Dave Jones <davej@redhat.com>
 > > 
 > 
 > Does this improve performance enough to warrant putting it into the
 > current cycle?  Or is queuing it for 2.6.33 sufficient?

I haven't done any performance testing with/without. My intentions
were purely from a correctness standpoint. 

It's not critical, and we've lived with this bug for a long time,
so waiting is fine.

	Dave

Re: [X86] PCI: Use generic cacheline sizing instead of per-vendor tests.

[Dave Jones]

On Wed, Oct 14, 2009 at 04:31:39PM -0400, Dave Jones wrote:

 > I've been carrying a variant of this code in Fedora for a while,
 > that prints debug information.  There are a number of cases where we
 > are currently setting the PCI cacheline size to 32 bytes, when the CPU
 > cacheline size is 64 bytes.  With this patch, we set them both the same.
 
In case anyone is curious, here's the version we carried in Fedora for a while..

	Dave

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 2202b62..f371fe8 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -432,6 +432,22 @@ int __init pcibios_init(void)
 	else if (c->x86 > 6 && c->x86_vendor == X86_VENDOR_INTEL)
 		pci_cache_line_size = 128 >> 2;	/* P4 */
 
+	if (c->x86_clflush_size != (pci_cache_line_size <<2))
+		printk(KERN_DEBUG "PCI: old code would have set cacheline "
+			"size to %d bytes, but clflush_size = %d\n",
+			pci_cache_line_size << 2,
+			c->x86_clflush_size);
+
+	/* Once we know this logic works, all the above code can be deleted. */
+	if (c->x86_clflush_size > 0) {
+		pci_cache_line_size = c->x86_clflush_size >> 2;
+		printk(KERN_DEBUG "PCI: pci_cache_line_size set to %d bytes\n",
+			pci_cache_line_size << 2);
+	} else {
+		pci_cache_line_size = 32 >> 2;
+		printk(KERN_DEBUG "PCI: Unknown cacheline size. Setting to 32 bytes\n");
+	}
+
 	pcibios_resource_survey();
 
 	if (pci_bf_sort >= pci_force_bf)

Re: [X86] PCI: Use generic cacheline sizing instead of per-vendor tests.

[Jesse Barnes]

On Wed, 14 Oct 2009 16:31:39 -0400
Dave Jones <davej@redhat.com> wrote:

> Instead of the PCI code needing to have code to determine the 
> cacheline size of each processor, use the data the cpu identification
> code should have already determined during early boot.
> 
> (The vendor checks are also incomplete, and don't take into account
>  modern CPUs)
> 
> I've been carrying a variant of this code in Fedora for a while,
> that prints debug information.  There are a number of cases where we
> are currently setting the PCI cacheline size to 32 bytes, when the CPU
> cacheline size is 64 bytes.  With this patch, we set them both the
> same.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

Applied this; had to fix up a few conflicts due to Tejun's recent CLS
improvements though...

-- 
Jesse Barnes, Intel Open Source Technology Center