Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
On Wed, 26 Sep 2018 08:36:47 -0700 Dave Hansen wrote: > On 09/26/2018 12:38 AM, Michal Hocko wrote: > > Why cannot you simply go with [no]vm_page_poison[=on/off]? > > I was trying to look to the future a bit, if we end up with five or six > more other options we want to allow folks to enable/disable. I don't > want to end up in a situation where we have a bunch of different knobs > to turn all this stuff off at runtime. > > I'd really like to have one stop shopping so that folks who have a > system that's behaving well and don't need any debugging can get some of > their performance back. > > But, the *primary* thing we want here is a nice, quick way to turn as > much debugging off as we can. A nice-to-have is a future-proof, > slub-style option that will centralize things. Yup. DEBUG_VM just covers too much stuff nowadays. A general way to make these thing more fine-grained and without requiring a rebuild would be great. And I expect that quite a few of the debug features could be enabled/disabled after bootup as well, so a /proc knob is probably in our future. Any infrastructure which is added to support a new kernel-command-line option should be designed with that in mind. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
On 09/26/2018 08:24 AM, Alexander Duyck wrote: > With no options it works just like slub_debug and enables all > available options. So in our case it is a NOP since we wanted the > debugging enabled by default. Yeah, but slub_debug is different. First, nobody uses the slub_debug=- option because *that* is only used when you have SLUB_DEBUG=y *and* CONFIG_SLUB_DEBUG_ON=y, which not even Fedora does. slub_debug is *primarily* for *adding* debug features. For this, we need to turn them off. It sounds like following slub_debug was a bad idea, especially following its semantics too closely when it doesn't make sense. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
On 09/26/2018 12:38 AM, Michal Hocko wrote: > Why cannot you simply go with [no]vm_page_poison[=on/off]? I was trying to look to the future a bit, if we end up with five or six more other options we want to allow folks to enable/disable. I don't want to end up in a situation where we have a bunch of different knobs to turn all this stuff off at runtime. I'd really like to have one stop shopping so that folks who have a system that's behaving well and don't need any debugging can get some of their performance back. But, the *primary* thing we want here is a nice, quick way to turn as much debugging off as we can. A nice-to-have is a future-proof, slub-style option that will centralize things. Alex's patch fails at the primary goal, IMNHO because "vm_debug=-" is so weird. I'd much rather have "vm_debug=off" (the primary goal) and throw away the nice-to-have (future-proof fine-grained on/off). I think we can have both, but I guess the onus is on me to go and add a strcmp(..., "off"). :) ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
On Wed 26-09-18 08:24:56, Alexander Duyck wrote: > On 9/26/2018 12:38 AM, Michal Hocko wrote: > > On Tue 25-09-18 13:20:12, 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 > > > > I agree with Dave that this is confusing as hell. So what does vm_debug > > (without any options means). I assume it's NOP and all debugging is > > enabled and that is the default. What if I want to disable _only_ the > > page struct poisoning. The weird lookcing `-' will disable all other > > options that we might gather in the future. > > With no options it works just like slub_debug and enables all available > options. So in our case it is a NOP since we wanted the debugging enabled by > default. But isn't slub_debug more about _adding_ debugging features? While you want to effectively disbale some debugging features here? So if you want to follow that pattern then it would be something like vm_debug_disable=page_poisoning,$OTHER_FUTURE_DEBUG_OPTIONS why would you want to enable something when CONFIG_DEBUG_VM=y just enables everything? > > Why cannot you simply go with [no]vm_page_poison[=on/off]? > > That is what I had to begin with, but Dave Hansen and Dan Williams suggested > that I go with a slub_debug style interface so we could extend it in the > future. Please let's not over-engineer this. If you really need an umbrella parameter then make a list of things to disable. -- Michal Hocko SUSE Labs ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
On 9/26/2018 12:38 AM, Michal Hocko wrote: On Tue 25-09-18 13:20:12, 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 I agree with Dave that this is confusing as hell. So what does vm_debug (without any options means). I assume it's NOP and all debugging is enabled and that is the default. What if I want to disable _only_ the page struct poisoning. The weird lookcing `-' will disable all other options that we might gather in the future. With no options it works just like slub_debug and enables all available options. So in our case it is a NOP since we wanted the debugging enabled by default. Why cannot you simply go with [no]vm_page_poison[=on/off]? That is what I had to begin with, but Dave Hansen and Dan Williams suggested that I go with a slub_debug style interface so we could extend it in the future. It would probably make more sense if we had additional options added, but we only have one option for now so the only values we really have are 'P' and '-' for now. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
On Tue 25-09-18 13:20:12, 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 I agree with Dave that this is confusing as hell. So what does vm_debug (without any options means). I assume it's NOP and all debugging is enabled and that is the default. What if I want to disable _only_ the page struct poisoning. The weird lookcing `-' will disable all other options that we might gather in the future. Why cannot you simply go with [no]vm_page_poison[=on/off]? -- Michal Hocko SUSE Labs ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
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
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
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'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. :) >> 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, an> 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). ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
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. Either I can do this as a single parameter that supports on/off semantics, or I can support it as a slub_debug type parameter that does flags based on the input options. I would rather not muddy things by trying to do both. Also, 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". ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 2/4] mm: Provide kernel parameter to allow disabling page init poisoning
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. Also, we need to document the defaults. I think the default is "all debug options are enabled", but it would be nice to document that. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
