x86_64: simplify the memtest parameter setting

PATCH In review Yinghai Lu 4 participants 5 messages Apr 19, 2008

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Yinghai Lu

author

2008-04-19 00:49 UTC #0 lore raw

PATCH 1/1 x86_64: simplify the memtest parameter setting

use CONFIG_MEMTEST only. if it is set, will have memtest=0 (disabled)

need to have memtest=4 in command line to test more patterns.

Signed-off-by Yinghai Lu <[email protected]>

arch/x86/Kconfig +7 ~~-23~~

index 21825ea..42d6568 100644
@@ -389,35 +389,19 @@ config PARAVIRT
 
 endif
 
-config MEMTEST_BOOTPARAM
-	bool "Memtest boot parameter"
+config MEMTEST
+	bool "Memtest"
 	depends on X86_64
 	default y
 	help
 	  This option adds a kernel parameter 'memtest', which allows memtest
-	  to be disabled at boot.  If this option is selected, memtest
-	  functionality can be disabled with memtest=0 on the kernel
-	  command line.  The purpose of this option is to allow a single
-	  kernel image to be distributed with memtest built in, but not
-	  necessarily enabled.
-
+	  to be set.
+		memtest=0, mean disabled; -- default
+		memtest=1, mean do 1 test pattern;
+		...
+		memtest=4, mean do 4 test patterns.
 	  If you are unsure how to answer this question, answer Y.
 
-config MEMTEST_BOOTPARAM_VALUE
-	int "Memtest boot parameter default value (0-4)"
-	depends on MEMTEST_BOOTPARAM
-	range 0 4
-	default 0
-	help
-	  This option sets the default value for the kernel parameter
-	  'memtest', which allows memtest to be disabled at boot.  If this
-	  option is set to 0 (zero), the memtest kernel parameter will
-	  default to 0, disabling memtest at bootup.  If this option is
-	  set to 4, the memtest kernel parameter will default to 4,
-	  enabling memtest at bootup, and use that as pattern number.
-
-	  If you are unsure how to answer this question, answer 0.
-
 config ACPI_SRAT
 	def_bool y
 	depends on X86_32 && ACPI && NUMA && (X86_SUMMIT || X86_GENERICARCH)

arch/x86/mm/init_64.c +3 -2

index b67ede4..82a0319 100644
@@ -432,7 +432,7 @@ static void __init init_gbpages(void)
 		direct_gbpages = 0;
 }
 
-#ifdef CONFIG_MEMTEST_BOOTPARAM
+#ifdef CONFIG_MEMTEST
 
 static void __init memtest(unsigned long start_phys, unsigned long size,
 				 unsigned pattern)

@@ -494,7 +494,8 @@ static void __init memtest(unsigned long start_phys, unsigned long size,
 
 }
 
-static int memtest_pattern __initdata = CONFIG_MEMTEST_BOOTPARAM_VALUE;
+/* default is disabled */
+static int memtest_pattern __initdata = 0;
 
 static int __init parse_memtest(char *arg)
 {

Frans Pop

2008-04-19 01:57 UTC #1 lore raw

arch/x86/Kconfig config PARAVIRT · line 395

Quoted 6 lines · Yinghai Lu

arch/x86/Kconfig +2 -2

-config MEMTEST_BOOTPARAM
-	bool "Memtest boot parameter"
+config MEMTEST
+	bool "Memtest"
 depends on X86_64
 default y

With this patch, this default no longer makes any sense.

arch/x86/Kconfig config PARAVIRT · line 402

Quoted 13 lines

help
This option adds a kernel parameter 'memtest', which allows memtest

arch/x86/Kconfig +5 -6

-	  to be disabled at boot.  If this option is selected, memtest
-	  functionality can be disabled with memtest=0 on the kernel
-	  command line.  The purpose of this option is to allow a single
-	  kernel image to be distributed with memtest built in, but not
-	  necessarily enabled.
-
+	  to be set.
+		memtest=0, mean disabled; -- default
+		memtest=1, mean do 1 test pattern;
+		...
+		memtest=4, mean do 4 test patterns.

s/mean/means/
s/; -- default/ (default)/ although that does not really make sense either

Maybe some explanation of what a "test pattern" is would be useful.

Quoted 1 line

If you are unsure how to answer this question, answer Y.

Also makes no sense anymore.

Cheers,
FJP

Frans Pop

2008-04-19 02:16 UTC #2 lore raw

Quoted 10 lines · Frans Pop

arch/x86/Kconfig config PARAVIRT · line 395
Quoted 6 lines · Yinghai Lu
arch/x86/Kconfig +2 -2
-config MEMTEST_BOOTPARAM
-	bool "Memtest boot parameter"
+config MEMTEST
+	bool "Memtest"
 depends on X86_64
 default y
With this patch, this default no longer makes any sense.

Quoted 16 lines

arch/x86/Kconfig config PARAVIRT · line 402

Quoted 13 lines

help
This option adds a kernel parameter 'memtest', which allows memtest

arch/x86/Kconfig +5 -6

-	  to be disabled at boot.  If this option is selected, memtest
-	  functionality can be disabled with memtest=0 on the kernel
-	  command line.  The purpose of this option is to allow a single
-	  kernel image to be distributed with memtest built in, but not
-	  necessarily enabled.
-
+	  to be set.
+		memtest=0, mean disabled; -- default
+		memtest=1, mean do 1 test pattern;
+		...
+		memtest=4, mean do 4 test patterns.

s/mean/means/
s/; -- default/ (default)/

These are still good (IMO).

Quoted 5 lines

Maybe some explanation of what a "test pattern" is would be useful.
Quoted 1 line
If you are unsure how to answer this question, answer Y.
Also makes no sense anymore.

Oops. Just see I was too fast. After reading it again I see that the y/n value is correct after all.

However, it also means that the new description is probably too short. The phrase "allows memtest to be set" does not really mean anything to a user who does not know what memtest really does. It also does not explain that memtest is a boot parameter.

Also, it should probably be made more explicit that no memtest is actually performed unless the memtest boot parameter is passed at the boot prompt.

Looks like 4 is the maximum possible value. This should be mentioned explicitly (here as well as in Documentation/kernel-parameters.txt).

Maybe something like this:
This option allows a memory test to be executed when the system is booted. A memory test is only actually executed if the kernel boot parameter 'memtest' is passed.

The value of that parameter is the number to times the test is performed:
memtest=0: don't perform the test (default) memtest=1: perform the test once
...
memtest=4: perform the test 4 times (maximum).

If you are unsure how to answer this question, answer Y.

Hmmm. The original description mentions "N patterns". If those patterns are actually different, then a different description is probably needed.

Sorry for the confusion.

Cheers,
FJP

Yinghai Lu

2008-04-19 03:17 UTC #3 lore raw

Quoted 45 lines · Frans Pop

Quoted 24 lines · Frans Pop
arch/x86/Kconfig config PARAVIRT · line 395
Quoted 6 lines · Yinghai Lu
arch/x86/Kconfig +2 -2
-config MEMTEST_BOOTPARAM
-   bool "Memtest boot parameter"
+config MEMTEST
+   bool "Memtest"
 depends on X86_64
 default y
With this patch, this default no longer makes any sense.
arch/x86/Kconfig config PARAVIRT · line 402
Quoted 13 lines
help
This option adds a kernel parameter 'memtest', which allows memtest
arch/x86/Kconfig +5 -6
-     to be disabled at boot.  If this option is selected, memtest
-     functionality can be disabled with memtest=0 on the kernel
-     command line.  The purpose of this option is to allow a single
-     kernel image to be distributed with memtest built in, but not
-     necessarily enabled.
-
+     to be set.
+           memtest=0, mean disabled; -- default
+           memtest=1, mean do 1 test pattern;
+           ...
+           memtest=4, mean do 4 test patterns.
Quoted 2 lines
s/mean/means/
s/; -- default/ (default)/
These are still good (IMO).
Quoted 5 lines
Maybe some explanation of what a "test pattern" is would be useful.
Quoted 1 line
If you are unsure how to answer this question, answer Y.
Also makes no sense anymore.
Oops. Just see I was too fast. After reading it again I see that the y/n value is correct after all.
However, it also means that the new description is probably too short. The phrase "allows memtest to be set" does not really mean anything to a user who does not know what memtest really does. It also does not explain that memtest is a boot parameter.

Quoted 5 lines

Also, it should probably be made more explicit that no memtest is actually performed unless the memtest boot parameter is passed at the boot prompt.
Looks like 4 is the maximum possible value. This should be mentioned explicitly (here as well as in Documentation/kernel-parameters.txt).

current it only support 4 pattern, will try to move memtester pattern into it later...

Stefan Richter

2008-04-19 08:21 UTC #4 lore raw

Quoted 11 lines · Frans Pop

Quoted 10 lines
the new description is probably too short.
The phrase "allows memtest to be set" does not really mean anything to a user who does not know what memtest really does. It also does not explain that memtest is a boot parameter.
Also, it should probably be made more explicit that no memtest is actually performed unless the memtest boot parameter is passed at the boot prompt.
Looks like 4 is the maximum possible value. This should be mentioned explicitly (here as well as in Documentation/kernel-parameters.txt).

To add to this: The help text should IMO contain
- that a test for faulty RAM is being added if Y,
- that this feature can be controlled by the boot parameter (and, per the parameter's default, is normally off),
- how many loops with which data patterns are performed depending on the boot parameter value,
- how long this might take (so that nobody who hears of this for the first time enables it e.g. on a server which is supposed to not take too long to reboot),
- how reliable the results of this test are (I presume that some faults can only be found by many repeated re-runs of the memory tester),
- that saying Y slightly (?) increases the size of the kernel image but does not influence the kernel size after boot-up is completed.

Thanks,

Signature · 3 lines

Stefan Richter
-=====-==--- -=-- =--==
http://arcgraph.de/sr/