[PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Peter P Waskiewicz Jr]

On certain BIOSes, SRAT enumeration isn't exported correctly.
This leads to NUMA node enumeration failure, and causes the kernel
to fall back onto a single node treated as flat memory.  This
can happen on large, multi-socket systems (4 or more sockets), and
becomes problematic for performance.

This patch adds a boot parameter to allow a kernel to be booted
with the option to skip the SRAT check.  There are BIOSes in
production that have these failures, so this will allow people
in the field to work around these BIOS issues.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
---

 Documentation/x86/x86_64/boot-options.txt |    4 ++++
 arch/x86/mm/srat_64.c                     |   20 +++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 7fbbaf8..7863d9c 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -316,3 +316,7 @@ Miscellaneous
 		Do not use GB pages for kernel direct mappings.
 	gbpages
 		Use GB pages for kernel direct mappings.
+ 	sratbypassbios
+		If specified, will skip an SRAT check for PXM coverage
+		from BIOS enumeration.  Only to be used on systems with
+		buggy BIOSes that munge the SRAT enumeration.
diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
index f9897f7..8719472 100644
--- a/arch/x86/mm/srat_64.c
+++ b/arch/x86/mm/srat_64.c
@@ -351,6 +351,15 @@ int __init acpi_get_nodes(struct bootnode *physnodes)
 	return ret;
 }
 
+int srat_bypass_bios = 0;
+
+static int __init srat_bypass_bios_setup(char *str)
+{
+        srat_bypass_bios = 1;
+        return 0;
+}
+early_param("sratbypassbios", srat_bypass_bios_setup);
+
 /* Use the information discovered above to actually set up the nodes. */
 int __init acpi_scan_nodes(unsigned long start, unsigned long end)
 {
@@ -425,9 +434,14 @@ int __init acpi_scan_nodes(unsigned long start, unsigned long end)
 						nodes[i].end >> PAGE_SHIFT);
 	/* for out of order entries in SRAT */
 	sort_node_map();
-	if (!nodes_cover_memory(nodes)) {
-		bad_srat();
-		return -1;
+	if (!srat_bypass_bios) {
+		if (!nodes_cover_memory(nodes)) {
+			bad_srat();
+			return -1;
+		}
+	} else {
+		printk(KERN_INFO
+		           "SRAT: Bypassing NUMA sanity check...bad BIOS...\n");
 	}
 
 	/* Account for nodes with cpus and no memory */

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Andi Kleen]

On Wed, Sep 01, 2010 at 02:33:18PM -0700, Peter P Waskiewicz Jr wrote:
> On certain BIOSes, SRAT enumeration isn't exported correctly.
> This leads to NUMA node enumeration failure, and causes the kernel
> to fall back onto a single node treated as flat memory.  This
> can happen on large, multi-socket systems (4 or more sockets), and
> becomes problematic for performance.
> 
> This patch adds a boot parameter to allow a kernel to be booted
> with the option to skip the SRAT check.  There are BIOSes in
> production that have these failures, so this will allow people
> in the field to work around these BIOS issues.

Looks good.

Reviewed-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[rdunlap]

On Wed, September 1, 2010 2:33 pm, Peter P Waskiewicz Jr wrote:
> Documentation/x86/x86_64/boot-options.txt |    4 ++++
> arch/x86/mm/srat_64.c                     |   20 +++++++++++++++++--- 2
> files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c index
> f9897f7..8719472 100644 --- a/arch/x86/mm/srat_64.c
> +++ b/arch/x86/mm/srat_64.c
> @@ -351,6 +351,15 @@ int __init acpi_get_nodes(struct bootnode *physnodes)
>  return ret; }
>
>
> +int srat_bypass_bios = 0;

static int srat_bypass_bios;

Don't need to init to 0.

> +
> +static int __init srat_bypass_bios_setup(char *str)
> +{
> +        srat_bypass_bios = 1;
> +        return 0;
> +}
> +early_param("sratbypassbios", srat_bypass_bios_setup);
> +
> /* Use the information discovered above to actually set up the nodes. */
> int __init acpi_scan_nodes(unsigned long start, unsigned long end) {
> @@ -425,9 +434,14 @@ int __init acpi_scan_nodes(unsigned long start,
> unsigned long end) nodes[i].end >> PAGE_SHIFT); /* for out of order entries
> in SRAT */ sort_node_map(); -	if (!nodes_cover_memory(nodes)) {
> -		bad_srat();
> -		return -1;
> +	if (!srat_bypass_bios) {
> +		if (!nodes_cover_memory(nodes)) {
> +			bad_srat();
> +			return -1;
> +		}
> +	} else {
> +		printk(KERN_INFO
> +		           "SRAT: Bypassing NUMA sanity check...bad BIOS...\n");
> }

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Peter P Waskiewicz Jr]

On Wed, 2010-09-01 at 15:04 -0700, rdunlap@xenotime.net wrote:
> On Wed, September 1, 2010 2:33 pm, Peter P Waskiewicz Jr wrote:
> > Documentation/x86/x86_64/boot-options.txt |    4 ++++
> > arch/x86/mm/srat_64.c                     |   20 +++++++++++++++++--- 2
> > files changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c index
> > f9897f7..8719472 100644 --- a/arch/x86/mm/srat_64.c
> > +++ b/arch/x86/mm/srat_64.c
> > @@ -351,6 +351,15 @@ int __init acpi_get_nodes(struct bootnode *physnodes)
> >  return ret; }
> >
> >
> > +int srat_bypass_bios = 0;
> 
> static int srat_bypass_bios;
> 
> Don't need to init to 0.

I'll fix and send a second version.  Thanks Randy.

-PJ

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[H. Peter Anvin]

On 09/01/2010 03:11 PM, Peter P Waskiewicz Jr wrote:
> On Wed, 2010-09-01 at 15:04 -0700, rdunlap@xenotime.net wrote:
>> On Wed, September 1, 2010 2:33 pm, Peter P Waskiewicz Jr wrote:
>>> Documentation/x86/x86_64/boot-options.txt |    4 ++++
>>> arch/x86/mm/srat_64.c                     |   20 +++++++++++++++++--- 2
>>> files changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c index
>>> f9897f7..8719472 100644 --- a/arch/x86/mm/srat_64.c
>>> +++ b/arch/x86/mm/srat_64.c
>>> @@ -351,6 +351,15 @@ int __init acpi_get_nodes(struct bootnode *physnodes)
>>>  return ret; }
>>>
>>>
>>> +int srat_bypass_bios = 0;
>>
>> static int srat_bypass_bios;
>>
>> Don't need to init to 0.
> 
> I'll fix and send a second version.  Thanks Randy.
> 

Should be static *bool* srat_bypass_bios;

Please use bool for actual booleans; it prevents someone from being
"smart" and using values like 2 or -1 for "special cases".

	-hpa

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Andi Kleen]

> > +int srat_bypass_bios = 0;
> 
> static int srat_bypass_bios;
> 
> Don't need to init to 0.

It actually doesn't matter to gcc, it generates the same
code for both cases.

-Andi

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Ingo Molnar]

* Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com> wrote:

> On certain BIOSes, SRAT enumeration isn't exported correctly. This 
> leads to NUMA node enumeration failure, and causes the kernel to fall 
> back onto a single node treated as flat memory.  This can happen on 
> large, multi-socket systems (4 or more sockets), and becomes 
> problematic for performance.
> 
> This patch adds a boot parameter to allow a kernel to be booted with 
> the option to skip the SRAT check.  There are BIOSes in production 
> that have these failures, so this will allow people in the field to 
> work around these BIOS issues.

> + 	sratbypassbios
> +		If specified, will skip an SRAT check for PXM coverage
> +		from BIOS enumeration.  Only to be used on systems with
> +		buggy BIOSes that munge the SRAT enumeration.

This isnt a particularly useful solution to users of said systems - they 
have to figure out that this option exists, and then they have to enter 
this option on the boot line.

A better solution would be to match these systems using DMI filters - 
_and_ to also have the boot time option to cover the case where a new 
system comes out with such a breakage. (or there's some system not yet 
mapped)


	Ingo

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Andi Kleen]

> This isnt a particularly useful solution to users of said systems - they 
> have to figure out that this option exists, and then they have to enter 
> this option on the boot line.

This usually only happens in early preproduction systems. So far
the BIOS always got fixed before they shipped to users.

But it's still useful to have this option so that the early
users have a workaround.

It's difficult to maintain DMI strings for early systems,
they are too bleeding edge and the entries would be stale
as soon as the BIOS is fixed.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> > This isnt a particularly useful solution to users of said systems - 
> > they have to figure out that this option exists, and then they have 
> > to enter this option on the boot line.
> 
> This usually only happens in early preproduction systems. So far the 
> BIOS always got fixed before they shipped to users.

'Usually' != 'always'. Read the changelog:

  ' There are BIOSes in production that have these failures, so this will 
    allow people in the field to work around these BIOS issues. '

Peter, which system in production that has this problem? That one needs 
a DMI match.

Preproduction systems can certainly be hacked around with the boot 
option and are not worth the DMI match. (preproduction systems also tend 
to have have very unspecific DMI strings in most cases - things like 
'1234567890' or 'to be filled by OEM')

In important cases a PCI ID match can be done for serious bugs in 
widespread preproduction systems - we have a handful of examples in-tree 
for such workarounds - but i doubt this is such a case - we use it for 
crasher bugs, not for 'boots up slow' cases.

Nevertheless, production systems need a DMI match.

Thanks,

	Ingo

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Peter P Waskiewicz Jr]

On Thu, 2010-09-02 at 23:39 -0700, Ingo Molnar wrote:
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > This isnt a particularly useful solution to users of said systems - 
> > > they have to figure out that this option exists, and then they have 
> > > to enter this option on the boot line.
> > 
> > This usually only happens in early preproduction systems. So far the 
> > BIOS always got fixed before they shipped to users.
> 
> 'Usually' != 'always'. Read the changelog:
> 
>   ' There are BIOSes in production that have these failures, so this will 
>     allow people in the field to work around these BIOS issues. '
> 
> Peter, which system in production that has this problem? That one needs 
> a DMI match.

It's one SKU of a Nehalem-EX system.  The BIOS for that SKU has an issue
with resolving SRAT hotplug enumeration, and screws up the table.  Other
SKU's of this same platform do not have the issue.  Efforts are underway
to get this BIOS fixed, but in the meantime, there's nothing for users
to work around the bug (aside from disabling memory hotplug in the
BIOS).  Another platform almost shipped with the same symptoms, but
caught it and had it fixed before it shipped (didn't catch it early
because Windows wasn't failing, and most of the testing on that platform
was done under Windows).

I agree with Andi that adding DMI strings would be overkill and would
leave clutter once the BIOS is fixed.  I look at this patch as a
stop-gap measure for people to fall back on until a newer BIOS is
available to correct the NUMA enumeration issues.  Without it, we have
nothing to point users to when they run into this, waiting for a new
BIOS.

Cheers,
-PJ

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Ingo Molnar]

* Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com> wrote:

> On Thu, 2010-09-02 at 23:39 -0700, Ingo Molnar wrote:
> > * Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > > This isnt a particularly useful solution to users of said systems - 
> > > > they have to figure out that this option exists, and then they have 
> > > > to enter this option on the boot line.
> > > 
> > > This usually only happens in early preproduction systems. So far the 
> > > BIOS always got fixed before they shipped to users.
> > 
> > 'Usually' != 'always'. Read the changelog:
> > 
> >   ' There are BIOSes in production that have these failures, so this will 
> >     allow people in the field to work around these BIOS issues. '
> > 
> > Peter, which system in production that has this problem? That one needs 
> > a DMI match.
> 
> It's one SKU of a Nehalem-EX system.  The BIOS for that SKU has an 
> issue with resolving SRAT hotplug enumeration, and screws up the 
> table.  Other SKU's of this same platform do not have the issue.  
> Efforts are underway to get this BIOS fixed, but in the meantime, 
> there's nothing for users to work around the bug (aside from disabling 
> memory hotplug in the BIOS).  Another platform almost shipped with the 
> same symptoms, but caught it and had it fixed before it shipped 
> (didn't catch it early because Windows wasn't failing, and most of the 
> testing on that platform was done under Windows).
>
> I agree with Andi that adding DMI strings would be overkill and would 
> leave clutter once the BIOS is fixed. [...]

We use the following policy for hardware/firmware workarounds in 
upstream arch/x86: if the system got shipped and if the vendor/OEM wants 
it fixed, then it has real DMI info (or some PCI ID match method, etc.) 
and an automatic workaround is very well possible and desirable.

If the vendor cannot be bothered to add a few lines based on a simple 
reading of dmidecode output and test it, then we dont really want/need 
the rest of the patch upstream either.

It should be literally 5 minutes of work to add a DMI match.

> I look at this patch as a stop-gap measure for people to fall back on 
> until a newer BIOS is available to correct the NUMA enumeration 
> issues. [...]

We dont do half-done stop-gap measures in the upstream kernel like that, 
and for various good reasons.

Furthermore, since Windows doesnt have a problem booting with this, i'm 
afraid that we are bound to see repeat problems of this sort, so we 
better have the DMI path beaten out - even if in this case it's a single 
model.

> [...]  Without it, we have nothing to point users to when they run 
> into this, waiting for a new BIOS.

I by all means support you to give users a real fix - one that applies 
the workaround automatically with a DMI match. Also, as i said, we can 
also add the boot option in the same patch.

Thanks,

	Ingo

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[H. Peter Anvin]

On 09/07/2010 12:38 PM, Peter P Waskiewicz Jr wrote:
> 
> It's one SKU of a Nehalem-EX system.  The BIOS for that SKU has an issue
> with resolving SRAT hotplug enumeration, and screws up the table.  Other
> SKU's of this same platform do not have the issue.  Efforts are underway
> to get this BIOS fixed, but in the meantime, there's nothing for users
> to work around the bug (aside from disabling memory hotplug in the
> BIOS).  Another platform almost shipped with the same symptoms, but
> caught it and had it fixed before it shipped (didn't catch it early
> because Windows wasn't failing, and most of the testing on that platform
> was done under Windows).
> 
> I agree with Andi that adding DMI strings would be overkill and would
> leave clutter once the BIOS is fixed.  I look at this patch as a
> stop-gap measure for people to fall back on until a newer BIOS is
> available to correct the NUMA enumeration issues.  Without it, we have
> nothing to point users to when they run into this, waiting for a new
> BIOS.
> 

No, this is exactly the kind of stuff for which a DMI match is ideal.  A
specific system with bounded propagation of the problem.  Thus, the DMI
match acts as a whitelist -- "we know this system and it is safe to
activate this hack on it."  This is a very good thing.

If this is a production BIOS it should have this information.

	-hpa

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Peter P Waskiewicz Jr]

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

On Tue, 2010-09-07 at 13:03 -0700, H. Peter Anvin wrote:
> On 09/07/2010 12:38 PM, Peter P Waskiewicz Jr wrote:
> > 
> > It's one SKU of a Nehalem-EX system.  The BIOS for that SKU has an issue
> > with resolving SRAT hotplug enumeration, and screws up the table.  Other
> > SKU's of this same platform do not have the issue.  Efforts are underway
> > to get this BIOS fixed, but in the meantime, there's nothing for users
> > to work around the bug (aside from disabling memory hotplug in the
> > BIOS).  Another platform almost shipped with the same symptoms, but
> > caught it and had it fixed before it shipped (didn't catch it early
> > because Windows wasn't failing, and most of the testing on that platform
> > was done under Windows).
> > 
> > I agree with Andi that adding DMI strings would be overkill and would
> > leave clutter once the BIOS is fixed.  I look at this patch as a
> > stop-gap measure for people to fall back on until a newer BIOS is
> > available to correct the NUMA enumeration issues.  Without it, we have
> > nothing to point users to when they run into this, waiting for a new
> > BIOS.
> > 
> 
> No, this is exactly the kind of stuff for which a DMI match is ideal.  A
> specific system with bounded propagation of the problem.  Thus, the DMI
> match acts as a whitelist -- "we know this system and it is safe to
> activate this hack on it."  This is a very good thing.
> 
> If this is a production BIOS it should have this information.

Responding for both your and Ingo's last email, I'll work on getting a
DMI match for this system.

Thanks,
-PJ

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4394 bytes --]

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[H. Peter Anvin]

On 09/07/2010 01:16 PM, Peter P Waskiewicz Jr wrote:
>>
>> If this is a production BIOS it should have this information.
> 
> Responding for both your and Ingo's last email, I'll work on getting a
> DMI match for this system.
> 

Great.

	-hpa

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Andi Kleen]

"H. Peter Anvin" <hpa@zytor.com> writes:
>
> If this is a production BIOS it should have this information.

The point was -- it wasn't a production BIOS and the production
BIOS will be fixed.

If you want you call fill the DMI tables with all bugs in preproduction
BIOS, but I suspect that will fill them up rather quickly. Preproduction
BIOS tend to be buggy (that is why they are pre-production)

Another problem also with adding stuff for early BIOS is that the later
BIOS likely have this fixed and then you have a quirk that runs but is
not needed (unless you add another DMI match for the fixed production
BIOS etc.) While for this case it would be probably harmless, 
this has caused problems in the past with other quirks.

Normally early users are also ok with having to specify an option
as a workaround, they just need something they can specify.

IMHO also it's good coding practice to have a command line option
for every DMI quirk anyways. This makes it easier to let 
users test if they need a particular quirk and helps
with users running older kernels.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[H. Peter Anvin]

On 09/07/2010 11:55 PM, Andi Kleen wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
>>
>> If this is a production BIOS it should have this information.
> 
> The point was -- it wasn't a production BIOS and the production
> BIOS will be fixed.
> 

>From the original description:

This patch adds a boot parameter to allow a kernel to be booted
with the option to skip the SRAT check.  There are BIOSes in
                                         ^^^^^^^^^^^^^^^^^^^
production that have these failures, so this will allow people
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
in the field to work around these BIOS issues.


Agreed that command-line option is a good thing to have in general and
that it is not necessary to have DMI quirks for preproduction BIOSes.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] [arch-x86] Allow SRAT integrity check to be skipped

[Peter P Waskiewicz Jr]

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

On Wed, 2010-09-08 at 11:03 -0700, H. Peter Anvin wrote:
> On 09/07/2010 11:55 PM, Andi Kleen wrote:
> > "H. Peter Anvin" <hpa@zytor.com> writes:
> >>
> >> If this is a production BIOS it should have this information.
> > 
> > The point was -- it wasn't a production BIOS and the production
> > BIOS will be fixed.
> > 
> 
> From the original description:
> 
> This patch adds a boot parameter to allow a kernel to be booted
> with the option to skip the SRAT check.  There are BIOSes in
>                                          ^^^^^^^^^^^^^^^^^^^
> production that have these failures, so this will allow people
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> in the field to work around these BIOS issues.
> 
> 
> Agreed that command-line option is a good thing to have in general and
> that it is not necessary to have DMI quirks for preproduction BIOSes.

Right.  The new patchset (when I finish it) will have the DMI quirk for
this specific production NHM-EX, plus the command-line option.

-PJ

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4394 bytes --]