[PATCH] x86_64: simplify the memtest parameter setting

[PATCH] x86_64: simplify the memtest parameter setting

[Yinghai Lu]

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 <yhlu.kernel@gmail.com>

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 21825ea..42d6568 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -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)
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index b67ede4..82a0319 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -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)
 {

Re: [PATCH] x86_64: simplify the memtest parameter setting

[Frans Pop]

Yinghai Lu wrote:
> -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.

>  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.

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.

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

Also makes no sense anymore.

Cheers,
FJP

Re: [PATCH] x86_64: simplify the memtest parameter setting

[Frans Pop]

On Saturday 19 April 2008, Frans Pop wrote:
> Yinghai Lu wrote:
> > -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.
>
> >  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.
>
> s/mean/means/
> s/; -- default/ (default)/

These are still good (IMO).

> Maybe some explanation of what a "test pattern" is would be useful.
>
> >  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

Re: [PATCH] x86_64: simplify the memtest parameter setting

[Yinghai Lu]

On Fri, Apr 18, 2008 at 7:15 PM, Frans Pop <elendil@planet.nl> wrote:
> On Saturday 19 April 2008, Frans Pop wrote:
>  > Yinghai Lu wrote:
>  > > -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.
>  >
>  > >  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.
>  >
>  > s/mean/means/
>  > s/; -- default/ (default)/
>
>  These are still good (IMO).
>
>
>  > Maybe some explanation of what a "test pattern" is would be useful.
>  >
>  > >  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).

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

YH

Re: [PATCH] x86_64: simplify the memtest parameter setting

[Stefan Richter]

> On Fri, Apr 18, 2008 at 7:15 PM, Frans Pop <elendil@planet.nl> wrote:
>>  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,
-- 
Stefan Richter
-=====-==--- -=-- =--==
http://arcgraph.de/sr/