lkml.org 
[lkml]   [2018]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
From
Date


On 9/25/2018 3:14 PM, Dave Hansen wrote:
> On 09/25/2018 01:38 PM, Alexander Duyck wrote:
>> On 9/25/2018 1:26 PM, Dave Hansen wrote:
>>> On 09/25/2018 01:20 PM, Alexander Duyck wrote:
>>>> +    vm_debug[=options]    [KNL] Available with CONFIG_DEBUG_VM=y.
>>>> +            May slow down system boot speed, especially when
>>>> +            enabled on systems with a large amount of memory.
>>>> +            All options are enabled by default, and this
>>>> +            interface is meant to allow for selectively
>>>> +            enabling or disabling specific virtual memory
>>>> +            debugging features.
>>>> +
>>>> +            Available options are:
>>>> +              P    Enable page structure init time poisoning
>>>> +              -    Disable all of the above options
>>>
>>> Can we have vm_debug=off for turning things off, please?  That seems to
>>> be pretty standard.
>>
>> No. The simple reason for that is that you had requested this work like
>> the slub_debug. If we are going to do that then each individual letter
>> represents a feature. That is why the "-" represents off. We cannot have
>> letters represent flags, and letters put together into words. For
>> example slub_debug=OFF would turn on sanity checks and turn off
>> debugging for caches that would have causes higher minimum slab orders.
>
> We don't have to have the same letters mean the same things for both
> options. We also can live without 'o' and 'f' being valid. We can
> *also* just say "don't do 'off'" if you want to enable things.

I'm not saying we do either. I would prefer it if we stuck to similar
behavior though. If we are going to do a slub_debug style parameter then
we should stick with similar behavior where "-" is used to indicate all
features off.

> I'd much rather have vm_debug=off do the right thing than have
> per-feature enable/disable. I know I'll *never* remember vm_debug=- and
> doing it this way will subject me to innumerable trips to Documentation/
> during my few remaining years.
>
> Surely you can make vm_debug=off happen. :)

I could, but then it is going to confuse people even more. I really feel
that if we want to do a slub_debug style interface we should use the
same switch for turning off all the features that they do for slub_debug.

>>> we need to document the defaults.  I think the default is "all
>>> debug options are enabled", but it would be nice to document that.
>>
>> In the description I call out "All options are enabled by default, and this interface is meant to allow for selectively enabling or disabling".
>
> I found "all options are enabled by default" really confusing. Maybe:
>
> "Control debug features which become available when CONFIG_DEBUG_VM=y.
> When this option is not specified, all debug features are enabled. Use
> this option enable a specific subset."
>
> Then, let's actually say what the options do, and what their impact is:
>
> P Enable 'struct page' poisoning at initialization.
> (Slows down boot time).
>

From my perspective I just don't see how this changes much since it
conveys the same message I had conveyed in my description. Since it
looks like Andrew applied the patch feel free to submit your suggestion
here as a follow-up patch and I would be willing to review/ack it.

\
 
 \ /
  Last update: 2018-09-26 00:28    [W:0.202 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site