All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
	linux-mm@kvack.org, akpm@linux-foundation.org,
	linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org
Cc: pavel.tatashin@microsoft.com, mhocko@suse.com,
	jglisse@redhat.com, rppt@linux.vnet.ibm.com, mingo@kernel.org,
	kirill.shutemov@linux.intel.com
Subject: Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
Date: Tue, 25 Sep 2018 15:27:59 -0700	[thread overview]
Message-ID: <61358149-b233-6194-adcf-2c16b9112fd7@linux.intel.com> (raw)
In-Reply-To: <75dde720-c997-51a4-d2e2-8b08eb201550@intel.com>



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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

WARNING: multiple messages have this Message-ID (diff)
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
	linux-mm@kvack.org, akpm@linux-foundation.org,
	linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org
Cc: pavel.tatashin@microsoft.com, mhocko@suse.com,
	dave.jiang@intel.com, jglisse@redhat.com,
	rppt@linux.vnet.ibm.com, dan.j.williams@intel.com,
	logang@deltatee.com, mingo@kernel.org,
	kirill.shutemov@linux.intel.com
Subject: Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
Date: Tue, 25 Sep 2018 15:27:59 -0700	[thread overview]
Message-ID: <61358149-b233-6194-adcf-2c16b9112fd7@linux.intel.com> (raw)
In-Reply-To: <75dde720-c997-51a4-d2e2-8b08eb201550@intel.com>



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.

WARNING: multiple messages have this Message-ID (diff)
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
	linux-mm@kvack.org, akpm@linux-foundation.org,
	linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org
Cc: pavel.tatashin@microsoft.com, mhocko@suse.com,
	dave.jiang@intel.com, jglisse@redhat.com,
	rppt@linux.vnet.ibm.com, dan.j.williams@intel.com,
	logang@deltatee.com, mingo@kernel.org,
	kirill.shutemov@linux.intel.com
Subject: Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
Date: Tue, 25 Sep 2018 15:27:59 -0700	[thread overview]
Message-ID: <61358149-b233-6194-adcf-2c16b9112fd7@linux.intel.com> (raw)
In-Reply-To: <75dde720-c997-51a4-d2e2-8b08eb201550@intel.com>



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:
>>>> +A A A  vm_debug[=options]A A A  [KNL] Available with CONFIG_DEBUG_VM=y.
>>>> +A A A A A A A A A A A  May slow down system boot speed, especially when
>>>> +A A A A A A A A A A A  enabled on systems with a large amount of memory.
>>>> +A A A A A A A A A A A  All options are enabled by default, and this
>>>> +A A A A A A A A A A A  interface is meant to allow for selectively
>>>> +A A A A A A A A A A A  enabling or disabling specific virtual memory
>>>> +A A A A A A A A A A A  debugging features.
>>>> +
>>>> +A A A A A A A A A A A  Available options are:
>>>> +A A A A A A A A A A A A A  PA A A  Enable page structure init time poisoning
>>>> +A A A A A A A A A A A A A  -A A A  Disable all of the above options
>>>
>>> Can we have vm_debug=off for turning things off, please?A  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.A  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.

  reply	other threads:[~2018-09-25 22:28 UTC|newest]

Thread overview: 144+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-25 20:18 [PATCH v5 0/4] Address issues slowing persistent memory initialization Alexander Duyck
2018-09-25 20:18 ` Alexander Duyck
2018-09-25 20:19 ` [PATCH v5 1/4] mm: Remove now defunct NO_BOOTMEM from depends list for deferred init Alexander Duyck
2018-09-25 20:19   ` Alexander Duyck
2018-09-25 21:05   ` Mike Rapoport
2018-09-25 21:05     ` Mike Rapoport
2018-09-25 20:20 ` [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning Alexander Duyck
2018-09-25 20:20   ` Alexander Duyck
2018-09-25 20:26   ` Dave Hansen
2018-09-25 20:26     ` Dave Hansen
2018-09-25 20:38     ` Alexander Duyck
2018-09-25 20:38       ` Alexander Duyck
2018-09-25 22:14       ` Dave Hansen
2018-09-25 22:14         ` Dave Hansen
2018-09-25 22:14         ` Dave Hansen
2018-09-25 22:27         ` Alexander Duyck [this message]
2018-09-25 22:27           ` Alexander Duyck
2018-09-25 22:27           ` Alexander Duyck
2018-09-26  7:38   ` Michal Hocko
2018-09-26  7:38     ` Michal Hocko
2018-09-26 15:24     ` Alexander Duyck
2018-09-26 15:39       ` Michal Hocko
2018-09-26 15:39         ` Michal Hocko
2018-09-26 15:41       ` Dave Hansen
2018-09-26 15:41         ` Dave Hansen
2018-09-26 16:18         ` Alexander Duyck
2018-09-26 15:36     ` Dave Hansen
2018-09-26 22:36       ` Andrew Morton
2018-09-26 22:36         ` Andrew Morton
2018-09-25 20:20 ` [PATCH v5 3/4] mm: Create non-atomic version of SetPageReserved for init use Alexander Duyck
2018-09-25 20:20   ` Alexander Duyck
2018-09-25 20:21 ` [PATCH v5 4/4] mm: Defer ZONE_DEVICE page initialization to the point where we init pgmap Alexander Duyck
2018-09-25 20:21   ` Alexander Duyck
2018-09-26  7:55   ` Michal Hocko
2018-09-26 18:25     ` Alexander Duyck
2018-09-26 18:25       ` Alexander Duyck
2018-09-26 18:52       ` Dan Williams
2018-09-26 18:52         ` Dan Williams
2018-09-27 11:20         ` Michal Hocko
2018-09-27 11:20           ` Michal Hocko
2018-09-27 11:09       ` Michal Hocko
2018-09-27 11:09         ` Michal Hocko
2018-09-27 12:25         ` Oscar Salvador
2018-09-27 13:13           ` Michal Hocko
2018-09-27 14:50             ` Oscar Salvador
2018-09-27 14:50               ` Oscar Salvador
2018-09-27 14:50               ` Oscar Salvador
2018-09-27 15:41               ` David Hildenbrand
2018-09-27 15:41                 ` David Hildenbrand
2018-09-28  8:12             ` Oscar Salvador
2018-09-28  8:12               ` Oscar Salvador
2018-09-28  8:44               ` Oscar Salvador
2018-09-28  8:44                 ` Oscar Salvador
2018-09-28 15:50                 ` Dan Williams
2018-09-28 15:50                   ` Dan Williams
2018-09-27 12:32       ` Oscar Salvador
2018-10-08 21:01   ` Dan Williams
2018-10-08 21:01     ` Dan Williams
2018-10-08 21:38     ` Alexander Duyck
2018-10-08 21:38       ` Alexander Duyck
2018-10-08 22:00       ` Dan Williams
2018-10-08 22:00         ` Dan Williams
2018-10-08 22:00         ` Dan Williams
2018-10-08 22:07         ` Alexander Duyck
2018-10-08 22:07           ` Alexander Duyck
2018-10-08 22:36         ` Alexander Duyck
2018-10-08 22:36           ` Alexander Duyck
2018-10-08 22:59           ` Dan Williams
2018-10-08 23:34     ` [mm PATCH] memremap: Fix reference count for pgmap in devm_memremap_pages Alexander Duyck
2018-10-08 23:34       ` Alexander Duyck
2018-10-09  0:20       ` Dan Williams
2018-10-09  0:20         ` Dan Williams
2018-10-09 17:00   ` [PATCH v5 4/4] mm: Defer ZONE_DEVICE page initialization to the point where we init pgmap Yi Zhang
2018-10-09 17:00     ` Yi Zhang
2018-10-09 18:04     ` Dan Williams
2018-10-09 18:04       ` Dan Williams
2018-10-09 20:26       ` Alexander Duyck
2018-10-09 20:26         ` Alexander Duyck
2018-10-09 21:19         ` Dan Williams
2018-10-09 21:19           ` Dan Williams
2018-10-10 12:52           ` Yi Zhang
2018-10-10 12:52             ` Yi Zhang
2018-10-10 15:27             ` Alexander Duyck
2018-10-10 15:27               ` Alexander Duyck
2018-10-11  8:17               ` Yi Zhang
2018-10-11  8:17                 ` Yi Zhang
2018-10-10  9:58         ` Michal Hocko
2018-10-10 16:39           ` Alexander Duyck
2018-10-10 16:39             ` Alexander Duyck
2018-10-10 17:24             ` Michal Hocko
2018-10-10 17:24               ` Michal Hocko
2018-10-10 17:39               ` Alexander Duyck
2018-10-10 17:39                 ` Alexander Duyck
2018-10-10 17:53                 ` Michal Hocko
2018-10-10 17:53                   ` Michal Hocko
2018-10-10 18:13                   ` Alexander Duyck
2018-10-10 18:13                     ` Alexander Duyck
2018-10-10 18:52                 ` Michal Hocko
2018-10-10 18:52                   ` Michal Hocko
2018-10-11  8:55                   ` Michal Hocko
2018-10-11  8:55                     ` Michal Hocko
2018-10-11 17:38                     ` Alexander Duyck
2018-10-11 18:22                       ` Dan Williams
2018-10-11 18:22                         ` Dan Williams
2018-10-17  7:52                       ` Michal Hocko
2018-10-17  7:52                         ` Michal Hocko
2018-10-17 15:02                         ` Alexander Duyck
2018-10-17 15:02                           ` Alexander Duyck
2018-10-29 14:12                           ` Michal Hocko
2018-10-29 14:12                             ` Michal Hocko
2018-10-29 15:59                             ` Alexander Duyck
2018-10-29 15:59                               ` Alexander Duyck
2018-10-29 15:59                               ` Alexander Duyck
2018-10-29 16:35                               ` Michal Hocko
2018-10-29 16:35                                 ` Michal Hocko
2018-10-29 17:01                                 ` Alexander Duyck
2018-10-29 17:24                                   ` Michal Hocko
2018-10-29 17:24                                     ` Michal Hocko
2018-10-29 17:34                                     ` Dan Williams
2018-10-29 17:34                                       ` Dan Williams
2018-10-29 17:45                                       ` Michal Hocko
2018-10-29 17:45                                         ` Michal Hocko
2018-10-29 17:42                                     ` Alexander Duyck
2018-10-29 17:42                                       ` Alexander Duyck
2018-10-29 18:18                                       ` Michal Hocko
2018-10-29 18:18                                         ` Michal Hocko
2018-10-29 19:59                                         ` Alexander Duyck
2018-10-29 19:59                                           ` Alexander Duyck
2018-10-30  6:29                                           ` Michal Hocko
2018-10-30  6:29                                             ` Michal Hocko
2018-10-30  6:55                                             ` Dan Williams
2018-10-30  8:17                                               ` Michal Hocko
2018-10-30  8:17                                                 ` Michal Hocko
2018-10-30 15:57                                                 ` Dan Williams
2018-10-30  8:05                                           ` Oscar Salvador
2018-10-29 15:49                           ` Dan Williams
2018-10-29 15:49                             ` Dan Williams
2018-10-29 15:56                             ` Michal Hocko
2018-10-10 18:18               ` Dan Williams
2018-10-10 18:18                 ` Dan Williams
2018-10-11  8:39                 ` Yi Zhang
2018-10-11  8:39                   ` Yi Zhang
2018-10-11 15:38                   ` Alexander Duyck
2018-10-11 15:38                     ` Alexander Duyck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=61358149-b233-6194-adcf-2c16b9112fd7@linux.intel.com \
    --to=alexander.h.duyck@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=dave.hansen@intel.com \
    --cc=jglisse@redhat.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=mhocko@suse.com \
    --cc=mingo@kernel.org \
    --cc=pavel.tatashin@microsoft.com \
    --cc=rppt@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.