| Message ID | 20201026173358.14704-2-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | optimize handling of memory debugging parameters | expand |
On 26.10.20 18:33, Vlastimil Babka wrote: > Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces > a warning in dmesg that page_poison takes precendence. However, as these > warnings are printed in early_param handlers for init_on_alloc/free, they are > not printed if page_poison is enabled later on the command line (handlers are > called in the order of their parameters), or when init_on_alloc/free is always > enabled by the respective config option - before the page_poison early param > handler is called, it is not considered to be enabled. This is inconsistent. > > We can remove the dependency on order by making the init_on_* parameters only > set a boolean variable, and postponing the evaluation after all early params > have been processed. Introduce a new init_mem_debugging() function for that, > and move the related debug_pagealloc processing there as well. init_mem_debugging() is somewhat sub-optimal - init_on_alloc=1 or init_on_free=1 are rather security hardening mechanisms. ... I wondered if this could be the place to initialize any kind of mm parameters in the future. Like init_mem_params() or so. > > As a result init_mem_debugging() knows always accurately if init_on_* and/or > page_poison options were enabled. Thus we can also optimize want_init_on_alloc() > and want_init_on_free(). We don't need to check page_poisoning_enabled() there, > we can instead not enable the init_on_* tracepoint at all, if page poisoning is > enabled. This results in a simpler and more effective code. LGTM Reviewed-by: David Hildenbrand <david@redhat.com>
On 10/27/20 10:03 AM, David Hildenbrand wrote: > On 26.10.20 18:33, Vlastimil Babka wrote: >> Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces >> a warning in dmesg that page_poison takes precendence. However, as these >> warnings are printed in early_param handlers for init_on_alloc/free, they are >> not printed if page_poison is enabled later on the command line (handlers are >> called in the order of their parameters), or when init_on_alloc/free is always >> enabled by the respective config option - before the page_poison early param >> handler is called, it is not considered to be enabled. This is inconsistent. >> >> We can remove the dependency on order by making the init_on_* parameters only >> set a boolean variable, and postponing the evaluation after all early params >> have been processed. Introduce a new init_mem_debugging() function for that, >> and move the related debug_pagealloc processing there as well. > > init_mem_debugging() is somewhat sub-optimal - init_on_alloc=1 or > init_on_free=1 are rather security hardening mechanisms. Well yeah, init_mem_debugging_and_hardening()? > ... I wondered if this could be the place to initialize any kind of mm > parameters in the future. Like init_mem_params() or so. Maybe. In practice you often find out that different things have to be hooked in different points of the init process, and a single function might not be enough. I tried to group stuff that's really inter-related and can be initialized at the same time. >> >> As a result init_mem_debugging() knows always accurately if init_on_* and/or >> page_poison options were enabled. Thus we can also optimize want_init_on_alloc() >> and want_init_on_free(). We don't need to check page_poisoning_enabled() there, >> we can instead not enable the init_on_* tracepoint at all, if page poisoning is >> enabled. This results in a simpler and more effective code. > > LGTM > > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks!
On 27.10.20 10:58, Vlastimil Babka wrote: > On 10/27/20 10:03 AM, David Hildenbrand wrote: >> On 26.10.20 18:33, Vlastimil Babka wrote: >>> Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces >>> a warning in dmesg that page_poison takes precendence. However, as these >>> warnings are printed in early_param handlers for init_on_alloc/free, they are >>> not printed if page_poison is enabled later on the command line (handlers are >>> called in the order of their parameters), or when init_on_alloc/free is always >>> enabled by the respective config option - before the page_poison early param >>> handler is called, it is not considered to be enabled. This is inconsistent. >>> >>> We can remove the dependency on order by making the init_on_* parameters only >>> set a boolean variable, and postponing the evaluation after all early params >>> have been processed. Introduce a new init_mem_debugging() function for that, >>> and move the related debug_pagealloc processing there as well. >> >> init_mem_debugging() is somewhat sub-optimal - init_on_alloc=1 or >> init_on_free=1 are rather security hardening mechanisms. > > Well yeah, init_mem_debugging_and_hardening()? Would work for me.
On Mon, Oct 26, 2020 at 06:33:56PM +0100, Vlastimil Babka wrote: > Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces > a warning in dmesg that page_poison takes precendence. However, as these ^ precedence > warnings are printed in early_param handlers for init_on_alloc/free, they are > not printed if page_poison is enabled later on the command line (handlers are > called in the order of their parameters), or when init_on_alloc/free is always > enabled by the respective config option - before the page_poison early param > handler is called, it is not considered to be enabled. This is inconsistent. > > We can remove the dependency on order by making the init_on_* parameters only > set a boolean variable, and postponing the evaluation after all early params > have been processed. Introduce a new init_mem_debugging() function for that, > and move the related debug_pagealloc processing there as well. > > As a result init_mem_debugging() knows always accurately if init_on_* and/or > page_poison options were enabled. Thus we can also optimize want_init_on_alloc() > and want_init_on_free(). We don't need to check page_poisoning_enabled() there, > we can instead not enable the init_on_* tracepoint at all, if page poisoning is > enabled. This results in a simpler and more effective code. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> With two more nits below fixed Reviewed-by: Mike Rapoport <rppt@linux.ibm.com> > --- > include/linux/mm.h | 20 ++-------- > init/main.c | 2 +- > mm/page_alloc.c | 94 +++++++++++++++++++++++----------------------- > 3 files changed, 50 insertions(+), 66 deletions(-) > ... > @@ -792,6 +752,44 @@ static inline void clear_page_guard(struct zone *zone, struct page *page, > unsigned int order, int migratetype) {} > #endif > > +/* > + * Enable static keys related to various memory debugging and hardening options. > + * Some override others, and depend on early params that are evaluated in the > + * order of appearance. So we need to first gather the full picture of what was > + * enabled, and then make decisions. > + */ > +void init_mem_debugging() Shouldn't it be init_mem_debug(void)? Or whatever a new name would be :) > +{ > + if (_init_on_alloc_enabled_early) { > + if (page_poisoning_enabled()) { > + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, " > + "will take precedence over init_on_alloc\n"); > + } else { > + static_branch_enable(&init_on_alloc); > + } > + } > + if (_init_on_free_enabled_early) { > + if (page_poisoning_enabled()) { > + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, " > + "will take precedence over init_on_free\n"); > + } else { > + static_branch_enable(&init_on_free); > + } > + } I think the braces for the inner ifs are not required. > + > +#ifdef CONFIG_DEBUG_PAGEALLOC > + if (!debug_pagealloc_enabled()) > + return; > + > + static_branch_enable(&_debug_pagealloc_enabled); > + > + if (!debug_guardpage_minorder()) > + return; > + > + static_branch_enable(&_debug_guardpage_enabled); > +#endif > +} > + > static inline void set_buddy_order(struct page *page, unsigned int order) > { > set_page_private(page, order); > -- > 2.29.0 > >
diff --git a/include/linux/mm.h b/include/linux/mm.h index ef360fe70aaf..c6a0adccf2fe 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2865,6 +2865,7 @@ extern int apply_to_existing_page_range(struct mm_struct *mm, unsigned long address, unsigned long size, pte_fn_t fn, void *data); +extern void init_mem_debugging(void); #ifdef CONFIG_PAGE_POISONING extern bool page_poisoning_enabled(void); extern void kernel_poison_pages(struct page *page, int numpages, int enable); @@ -2874,35 +2875,20 @@ static inline void kernel_poison_pages(struct page *page, int numpages, int enable) { } #endif -#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON -DECLARE_STATIC_KEY_TRUE(init_on_alloc); -#else DECLARE_STATIC_KEY_FALSE(init_on_alloc); -#endif static inline bool want_init_on_alloc(gfp_t flags) { - if (static_branch_unlikely(&init_on_alloc) && - !page_poisoning_enabled()) + if (static_branch_unlikely(&init_on_alloc)) return true; return flags & __GFP_ZERO; } -#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON -DECLARE_STATIC_KEY_TRUE(init_on_free); -#else DECLARE_STATIC_KEY_FALSE(init_on_free); -#endif static inline bool want_init_on_free(void) { - return static_branch_unlikely(&init_on_free) && - !page_poisoning_enabled(); + return static_branch_unlikely(&init_on_free); } -#ifdef CONFIG_DEBUG_PAGEALLOC -extern void init_debug_pagealloc(void); -#else -static inline void init_debug_pagealloc(void) {} -#endif extern bool _debug_pagealloc_enabled_early; DECLARE_STATIC_KEY_FALSE(_debug_pagealloc_enabled); diff --git a/init/main.c b/init/main.c index 130376ec10ba..dca5b9093742 100644 --- a/init/main.c +++ b/init/main.c @@ -815,7 +815,7 @@ static void __init mm_init(void) * bigger than MAX_ORDER unless SPARSEMEM. */ page_ext_init_flatmem(); - init_debug_pagealloc(); + init_mem_debugging(); report_meminit(); mem_init(); kmem_cache_init(); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 23f5066bd4a5..b168c58ef337 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -165,53 +165,26 @@ unsigned long totalcma_pages __read_mostly; int percpu_pagelist_fraction; gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK; -#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON -DEFINE_STATIC_KEY_TRUE(init_on_alloc); -#else -DEFINE_STATIC_KEY_FALSE(init_on_alloc); -#endif +DEFINE_STATIC_KEY_FALSE_RO(init_on_alloc); EXPORT_SYMBOL(init_on_alloc); -#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON -DEFINE_STATIC_KEY_TRUE(init_on_free); -#else -DEFINE_STATIC_KEY_FALSE(init_on_free); -#endif +DEFINE_STATIC_KEY_FALSE_RO(init_on_free); EXPORT_SYMBOL(init_on_free); +static bool _init_on_alloc_enabled_early __read_mostly + = IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON); static int __init early_init_on_alloc(char *buf) { - int ret; - bool bool_result; - ret = kstrtobool(buf, &bool_result); - if (ret) - return ret; - if (bool_result && page_poisoning_enabled()) - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_alloc\n"); - if (bool_result) - static_branch_enable(&init_on_alloc); - else - static_branch_disable(&init_on_alloc); - return 0; + return kstrtobool(buf, &_init_on_alloc_enabled_early); } early_param("init_on_alloc", early_init_on_alloc); +static bool _init_on_free_enabled_early __read_mostly + = IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON); static int __init early_init_on_free(char *buf) { - int ret; - bool bool_result; - - ret = kstrtobool(buf, &bool_result); - if (ret) - return ret; - if (bool_result && page_poisoning_enabled()) - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_free\n"); - if (bool_result) - static_branch_enable(&init_on_free); - else - static_branch_disable(&init_on_free); - return 0; + return kstrtobool(buf, &_init_on_free_enabled_early); } early_param("init_on_free", early_init_on_free); @@ -728,19 +701,6 @@ static int __init early_debug_pagealloc(char *buf) } early_param("debug_pagealloc", early_debug_pagealloc); -void init_debug_pagealloc(void) -{ - if (!debug_pagealloc_enabled()) - return; - - static_branch_enable(&_debug_pagealloc_enabled); - - if (!debug_guardpage_minorder()) - return; - - static_branch_enable(&_debug_guardpage_enabled); -} - static int __init debug_guardpage_minorder_setup(char *buf) { unsigned long res; @@ -792,6 +752,44 @@ static inline void clear_page_guard(struct zone *zone, struct page *page, unsigned int order, int migratetype) {} #endif +/* + * Enable static keys related to various memory debugging and hardening options. + * Some override others, and depend on early params that are evaluated in the + * order of appearance. So we need to first gather the full picture of what was + * enabled, and then make decisions. + */ +void init_mem_debugging() +{ + if (_init_on_alloc_enabled_early) { + if (page_poisoning_enabled()) { + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, " + "will take precedence over init_on_alloc\n"); + } else { + static_branch_enable(&init_on_alloc); + } + } + if (_init_on_free_enabled_early) { + if (page_poisoning_enabled()) { + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, " + "will take precedence over init_on_free\n"); + } else { + static_branch_enable(&init_on_free); + } + } + +#ifdef CONFIG_DEBUG_PAGEALLOC + if (!debug_pagealloc_enabled()) + return; + + static_branch_enable(&_debug_pagealloc_enabled); + + if (!debug_guardpage_minorder()) + return; + + static_branch_enable(&_debug_guardpage_enabled); +#endif +} + static inline void set_buddy_order(struct page *page, unsigned int order) { set_page_private(page, order);
Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces a warning in dmesg that page_poison takes precendence. However, as these warnings are printed in early_param handlers for init_on_alloc/free, they are not printed if page_poison is enabled later on the command line (handlers are called in the order of their parameters), or when init_on_alloc/free is always enabled by the respective config option - before the page_poison early param handler is called, it is not considered to be enabled. This is inconsistent. We can remove the dependency on order by making the init_on_* parameters only set a boolean variable, and postponing the evaluation after all early params have been processed. Introduce a new init_mem_debugging() function for that, and move the related debug_pagealloc processing there as well. As a result init_mem_debugging() knows always accurately if init_on_* and/or page_poison options were enabled. Thus we can also optimize want_init_on_alloc() and want_init_on_free(). We don't need to check page_poisoning_enabled() there, we can instead not enable the init_on_* tracepoint at all, if page poisoning is enabled. This results in a simpler and more effective code. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- include/linux/mm.h | 20 ++-------- init/main.c | 2 +- mm/page_alloc.c | 94 +++++++++++++++++++++++----------------------- 3 files changed, 50 insertions(+), 66 deletions(-)