[PATCH/RFC] mm/debug_pagealloc: enable it by default

[PATCH/RFC] mm/debug_pagealloc: enable it by default

[Christian Borntraeger]

since commit 031bc5743f158 ("mm/debug-pagealloc: make debug-pagealloc
boottime configurable") CONFIG_DEBUG_PAGEALLOC is by default a no-op.

This resulted in several unnoticed bugs, e.g.
https://lkml.kernel.org/g/<569F5E29.3090107@de.ibm.com>
or
https://lkml.kernel.org/g/<56A20F30.4050705@de.ibm.com>

as this behaviour change was not even documented in Kconfig.

Let's go back to the original default and reverse the command
line parameter to allow disabling the feature.

Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/kernel-parameters.txt | 8 ++++----
 mm/Kconfig.debug                    | 2 ++
 mm/page_alloc.c                     | 6 +++---
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 742f69d..7553c87 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -854,13 +854,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			tracking down these problems.
 
 	debug_pagealloc=
-			[KNL] When CONFIG_DEBUG_PAGEALLOC is set, this
-			parameter enables the feature at boot time. In
-			default, it is disabled. We can avoid allocating huge
+			[KNL] When CONFIG_DEBUG_PAGEALLOC is set to off, this
+			parameter disables the feature at boot time. In
+			default, it is enabled. We can avoid allocating huge
 			chunk of memory for debug pagealloc if we don't enable
 			it at boot time and the system will work mostly same
 			with the kernel built without CONFIG_DEBUG_PAGEALLOC.
-			on: enable the feature
+			off: disable the feature
 
 	debugpat	[X86] Enable PAT debugging
 
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index 957d3da..3f991a4 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -25,6 +25,8 @@ config DEBUG_PAGEALLOC
 	  this option cannot be enabled in combination with hibernation as
 	  that would result in incorrect warnings of memory corruption after
 	  a resume because free pages are not saved to the suspend image.
+	  The feature can be disable with the kernel parameter
+	  debug_pagealloc=off.
 
 config PAGE_POISONING
 	bool
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9d666df..8a03efd 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -479,7 +479,7 @@ void prep_compound_page(struct page *page, unsigned int order)
 
 #ifdef CONFIG_DEBUG_PAGEALLOC
 unsigned int _debug_guardpage_minorder;
-bool _debug_pagealloc_enabled __read_mostly;
+bool _debug_pagealloc_enabled __read_mostly = true;
 bool _debug_guardpage_enabled __read_mostly;
 
 static int __init early_debug_pagealloc(char *buf)
@@ -487,8 +487,8 @@ static int __init early_debug_pagealloc(char *buf)
 	if (!buf)
 		return -EINVAL;
 
-	if (strcmp(buf, "on") == 0)
-		_debug_pagealloc_enabled = true;
+	if (strcmp(buf, "off") == 0)
+		_debug_pagealloc_enabled = false;
 
 	return 0;
 }
-- 
2.3.0

Re: [PATCH/RFC] mm/debug_pagealloc: enable it by default

[Joonsoo Kim]

Hello,

On Fri, Jan 22, 2016 at 12:44:40PM +0100, Christian Borntraeger wrote:
> since commit 031bc5743f158 ("mm/debug-pagealloc: make debug-pagealloc
> boottime configurable") CONFIG_DEBUG_PAGEALLOC is by default a no-op.
> 
> This resulted in several unnoticed bugs, e.g.
> https://lkml.kernel.org/g/<569F5E29.3090107@de.ibm.com>
> or
> https://lkml.kernel.org/g/<56A20F30.4050705@de.ibm.com>
> 
> as this behaviour change was not even documented in Kconfig.
> 
> Let's go back to the original default and reverse the command
> line parameter to allow disabling the feature.

Default disable is requested by Andrew and I agreed that.

https://lkml.org/lkml/2014/11/27/3

I think that default disable is good thing because we can use
the kernel compiled with CONFIG_DEBUG_PAGEALLOC even for production.
Unless boot param is provided, it's runtime cost is nearly zero.
This could help to debug in some situations and make this debugging
feature more useful. But, yes, old users can be surprised
by this change. How about changing Kconfig to ask user for default mode?

Thanks.

Re: [PATCH/RFC] mm/debug_pagealloc: enable it by default

[Christian Borntraeger]

On 01/25/2016 07:41 AM, Joonsoo Kim wrote:
> Hello,
> 
> On Fri, Jan 22, 2016 at 12:44:40PM +0100, Christian Borntraeger wrote:
>> since commit 031bc5743f158 ("mm/debug-pagealloc: make debug-pagealloc
>> boottime configurable") CONFIG_DEBUG_PAGEALLOC is by default a no-op.
>>
>> This resulted in several unnoticed bugs, e.g.
>> https://lkml.kernel.org/g/<569F5E29.3090107@de.ibm.com>
>> or
>> https://lkml.kernel.org/g/<56A20F30.4050705@de.ibm.com>
>>
>> as this behaviour change was not even documented in Kconfig.
>>
>> Let's go back to the original default and reverse the command
>> line parameter to allow disabling the feature.
> 
> Default disable is requested by Andrew and I agreed that.
> 
> https://lkml.org/lkml/2014/11/27/3

Well, Andrew's assumption was "I suspect the number of "old users" is one"
and this is wrong. I can at least claim, that most of the s390 test were 
conducted with CONFIG_DEBUG_PAGEALLOC enabled (at least regularly) - and 
about a year ago this was changed to "no longer useful". I have posted 2
real bugs that I triggered pretty quickly after I enabled things on the 
command line.
So my claim is, that the theoretical win of detecting more bugs by enabling 
this on the command line for distro kernels is far less than the real loss
of testers not triggering bugs in the last year.

Let me do some quick check with the kernels that I find in the rpm/deb repos
rawhide: # CONFIG_DEBUG_PAGEALLOC is not set
opensuse leap: # CONFIG_DEBUG_PAGEALLOC is not set
opensuse tumbleweed: # CONFIG_DEBUG_PAGEALLOC is not set
ubuntu 4.3 (kernel team repo) # CONFIG_DEBUG_PAGEALLOC is not set

So at least the communication was not done properly. :-(

> I think that default disable is good thing because we can use
> the kernel compiled with CONFIG_DEBUG_PAGEALLOC even for production.
> Unless boot param is provided, it's runtime cost is nearly zero.
> This could help to debug in some situations and make this debugging
> feature more useful. But, yes, old users can be surprised
> by this change. How about changing Kconfig to ask user for default mode?

A default mode would be ok for me as it makes things obvious. Will send
a patch.

Re: [PATCH/RFC] mm/debug_pagealloc: enable it by default

[Dave Jones]

On Mon, Jan 25, 2016 at 09:21:53AM +0100, Christian Borntraeger wrote:

 > >> Let's go back to the original default and reverse the command
 > >> line parameter to allow disabling the feature.
 > > 
 > > Default disable is requested by Andrew and I agreed that.
 > > 
 > > https://lkml.org/lkml/2014/11/27/3
 > 
 > Well, Andrew's assumption was "I suspect the number of "old users" is one"
 > and this is wrong. I can at least claim, that most of the s390 test were 
 > conducted with CONFIG_DEBUG_PAGEALLOC enabled (at least regularly) - and 
 > about a year ago this was changed to "no longer useful". I have posted 2
 > real bugs that I triggered pretty quickly after I enabled things on the 
 > command line.

Nearly all the fuzz-test runs I've done over the last few years have had this
enabled most the time.  I was wondering why things got faster last year.

 > > I think that default disable is good thing because we can use
 > > the kernel compiled with CONFIG_DEBUG_PAGEALLOC even for production.
 > > Unless boot param is provided, it's runtime cost is nearly zero.
 > > This could help to debug in some situations and make this debugging
 > > feature more useful. But, yes, old users can be surprised
 > > by this change. How about changing Kconfig to ask user for default mode?
 > 
 > A default mode would be ok for me as it makes things obvious. Will send
 > a patch.

The messaging around changing a default like this was really poor.
When we do behaviour changes like this, we typically rename the CONFIG option
to make it obvious that things aren't as they used to be.

ho-hum.

	Dave

Re: [PATCH/RFC] mm/debug_pagealloc: enable it by default

[Dave Jones]

On Mon, Jan 25, 2016 at 02:22:10PM -0500, Dave Jones wrote:

 >  > A default mode would be ok for me as it makes things obvious. Will send
 >  > a patch.
 > 
 > The messaging around changing a default like this was really poor.
 > When we do behaviour changes like this, we typically rename the CONFIG option
 > to make it obvious that things aren't as they used to be.

Looking at this closer, shouldn't the original diff to make this runtime
have also changed the x86 implementation details in arch/x86/mm/pageattr.c ?
Even when we don't boot with the enable flag, we're doing the "split 2M page into 4Ks"
thing.  It looks like a s/debug_pagealloc/debug_pagealloc_enabled()/ might
do the right thing maybe ?

It's almost comedic that I walked into change c9e0d39126af thinking all this
stuff was working _because_ we were doing that page splitting.

	Dave

Re: [PATCH/RFC] mm/debug_pagealloc: enable it by default

[Christian Borntraeger]

On 01/25/2016 08:38 PM, Dave Jones wrote:
>  Content preview:  On Mon, Jan 25, 2016 at 02:22:10PM -0500, Dave Jones wrote:
>     > > A default mode would be ok for me as it makes things obvious. Will send
>     > > a patch. > > The messaging around changing a default like this was really
>     poor. > When we do behaviour changes like this, we typically rename the CONFIG
>     option > to make it obvious that things aren't as they used to be. [...] 
> 
>  Content analysis details:   (-2.9 points, 5.0 required)
> 
>   pts rule name              description
>  ---- ---------------------- --------------------------------------------------
>  -1.0 ALL_TRUSTED            Passed through trusted hosts only via SMTP
>  -1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%
>                              [score: 0.0000]
> X-ZLA-Header: unknown; 0
> X-ZLA-DetailInfo: BA=6.00004114; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZH=6.00000000; ZP=6.00000000; ZU=6.00000002; UDB=6.00294159; UTC=2016-01-25 19:38:24
> x-cbid: 16012519-0041-0000-0000-0000074865D5
> X-IBM-ISS-SpamDetectors: Score=0.40962; FLB=0; FLI=0; BY=0; FL=0; FP=0; FZ=0;
>  HX=0; KW=0; PH=0; RB=0; SC=0.40962; ST=0; TS=0; UL=0; ISC=
> X-IBM-ISS-DetailInfo:  BY=3.00004831; HX=3.00000240; KW=3.00000007;
>  PH=3.00000004; SC=3.00000137; SDB=6.00650474; UDB=6.00294159; UTC=2016-01-25
>  19:38:24
> X-TM-AS-MML: disable
> 
> On Mon, Jan 25, 2016 at 02:22:10PM -0500, Dave Jones wrote:
> 
>  >  > A default mode would be ok for me as it makes things obvious. Will send
>  >  > a patch.
>  > 
>  > The messaging around changing a default like this was really poor.
>  > When we do behaviour changes like this, we typically rename the CONFIG option
>  > to make it obvious that things aren't as they used to be.
> 
> Looking at this closer, shouldn't the original diff to make this runtime
> have also changed the x86 implementation details in arch/x86/mm/pageattr.c ?
> Even when we don't boot with the enable flag, we're doing the "split 2M page into 4Ks"
> thing.  It looks like a s/debug_pagealloc/debug_pagealloc_enabled()/ might
> do the right thing maybe ?

I have some patches ready, that fix the identity mapping for s390 and x86. sparc
is hard to do as it allocates the TSB very early. Will send around what I have soon.

Re: [PATCH/RFC] mm/debug_pagealloc: enable it by default

[Joonsoo Kim]

On Mon, Jan 25, 2016 at 08:51:56PM +0100, Christian Borntraeger wrote:
> On 01/25/2016 08:38 PM, Dave Jones wrote:
> >  Content preview:  On Mon, Jan 25, 2016 at 02:22:10PM -0500, Dave Jones wrote:
> >     > > A default mode would be ok for me as it makes things obvious. Will send
> >     > > a patch. > > The messaging around changing a default like this was really
> >     poor. > When we do behaviour changes like this, we typically rename the CONFIG
> >     option > to make it obvious that things aren't as they used to be. [...] 
> > 
> >  Content analysis details:   (-2.9 points, 5.0 required)
> > 
> >   pts rule name              description
> >  ---- ---------------------- --------------------------------------------------
> >  -1.0 ALL_TRUSTED            Passed through trusted hosts only via SMTP
> >  -1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%
> >                              [score: 0.0000]
> > X-ZLA-Header: unknown; 0
> > X-ZLA-DetailInfo: BA=6.00004114; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZH=6.00000000; ZP=6.00000000; ZU=6.00000002; UDB=6.00294159; UTC=2016-01-25 19:38:24
> > x-cbid: 16012519-0041-0000-0000-0000074865D5
> > X-IBM-ISS-SpamDetectors: Score=0.40962; FLB=0; FLI=0; BY=0; FL=0; FP=0; FZ=0;
> >  HX=0; KW=0; PH=0; RB=0; SC=0.40962; ST=0; TS=0; UL=0; ISC=
> > X-IBM-ISS-DetailInfo:  BY=3.00004831; HX=3.00000240; KW=3.00000007;
> >  PH=3.00000004; SC=3.00000137; SDB=6.00650474; UDB=6.00294159; UTC=2016-01-25
> >  19:38:24
> > X-TM-AS-MML: disable
> > 
> > On Mon, Jan 25, 2016 at 02:22:10PM -0500, Dave Jones wrote:
> > 
> >  >  > A default mode would be ok for me as it makes things obvious. Will send
> >  >  > a patch.
> >  > 
> >  > The messaging around changing a default like this was really poor.
> >  > When we do behaviour changes like this, we typically rename the CONFIG option
> >  > to make it obvious that things aren't as they used to be.
> > 
> > Looking at this closer, shouldn't the original diff to make this runtime
> > have also changed the x86 implementation details in arch/x86/mm/pageattr.c ?
> > Even when we don't boot with the enable flag, we're doing the "split 2M page into 4Ks"
> > thing.  It looks like a s/debug_pagealloc/debug_pagealloc_enabled()/ might
> > do the right thing maybe ?
> 
> I have some patches ready, that fix the identity mapping for s390 and x86. sparc
> is hard to do as it allocates the TSB very early. Will send around what I have soon.

Thanks for taking care of this.

I searched other sites using "#ifdef CONFIG_DEBUG_PAGEALLOC" and found
that we can do more. Maybe, powerpc and tile also can get benefit from such
a change. And, could you handle vmap_debug_free_range() and
get_freepointer_safe(), too?

And, when debug_pagealloc is disabled, it's better not to print
DEBUG_PAGEALLOC in some error logs.

Thanks.