| Message ID | 20201026173358.14704-4-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: > prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when > init_on_alloc is enabled, but will also always skip zeroing if the page was > already zeroed on free by init_on_free or page poisoning. > > The latter check implemented by free_pages_prezeroed() can involve two > different static keys. As prep_new_page() is really a hot path, let's introduce > a single static key free_pages_not_prezeroed for this purpose and initialize it > in init_mem_debugging(). Is this actually observable in practice? This smells like micro-optimization to me. Also, I thought the whole reason for static keys is to have basically no overhead at runtime, so I wonder if replacing two static key checks by a single one actually makes *some* difference.
On 10/27/20 10:10 AM, David Hildenbrand wrote: > On 26.10.20 18:33, Vlastimil Babka wrote: >> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when >> init_on_alloc is enabled, but will also always skip zeroing if the page was >> already zeroed on free by init_on_free or page poisoning. >> >> The latter check implemented by free_pages_prezeroed() can involve two >> different static keys. As prep_new_page() is really a hot path, let's introduce >> a single static key free_pages_not_prezeroed for this purpose and initialize it >> in init_mem_debugging(). > > Is this actually observable in practice? This smells like > micro-optimization to me. > > Also, I thought the whole reason for static keys is to have basically no > overhead at runtime, so I wonder if replacing two static key checks by a > single one actually makes *some* difference. You're right, the difference seems to be just a single NOP. The static key infrastructure seems to be working really well. (At least the asm inspection made me realize that kernel_poison_pages() is called unconditionally and the static key is checked inside, not inline so I'll be amending patch 2...) Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the code wrong. So unless others think it's a readability improvements, we can drop this patch. Or we can also reconsider this whole optimization. If the point is to be paranoid and enable both init_on_free and init_on_alloc, should we trust that nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex?
On 10/27/20 12:05 PM, Vlastimil Babka wrote: > On 10/27/20 10:10 AM, David Hildenbrand wrote: >> On 26.10.20 18:33, Vlastimil Babka wrote: >>> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when >>> init_on_alloc is enabled, but will also always skip zeroing if the page was >>> already zeroed on free by init_on_free or page poisoning. >>> >>> The latter check implemented by free_pages_prezeroed() can involve two >>> different static keys. As prep_new_page() is really a hot path, let's introduce >>> a single static key free_pages_not_prezeroed for this purpose and initialize it >>> in init_mem_debugging(). >> >> Is this actually observable in practice? This smells like >> micro-optimization to me. >> >> Also, I thought the whole reason for static keys is to have basically no >> overhead at runtime, so I wonder if replacing two static key checks by a >> single one actually makes *some* difference. > > You're right, the difference seems to be just a single NOP. The static key > infrastructure seems to be working really well. > (At least the asm inspection made me realize that kernel_poison_pages() is > called unconditionally and the static key is checked inside, not inline so I'll > be amending patch 2...) > > Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the > code wrong. So unless others think it's a readability improvements, we can drop > this patch. > > Or we can also reconsider this whole optimization. If the point is to be > paranoid and enable both init_on_free and init_on_alloc, should we trust that > nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex? More thoughts... PAGE_POISONING_NO_SANITY skips the check on "unpoisoning" whether poison was corrupted PAGE_POISONING_ZERO uses zero instead of 0xAA as poison pattern the point of enabling both of these seems to be moot now that init_on_free exists, as that zeroes pages that are being freed, without checking on alloc that they are still zeroed. What if only one is enabled? - PAGE_POISONING_NO_SANITY without PAGE_POISONING_ZERO - we poison with the 0xAA pattern but nobody checks it, so does it give us anything over init_on_free writing zeroes? I don't think so? - PAGE_POISONING_ZERO without PAGE_POISONING_NO_SANITY - we use zeroes (like init_on_free) but also check that it wasn't corrupted. We save some time on writing zeroes again on alloc, but the check is still expensive. And writing 0xAA would possibly detect more corruptions than writing zero (a stray write of NULL is more likely to happen than of 0xAA?). So my conclusion: - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with PAGE_POISONING_ZERO, and we can use init_on_free instead - We can also probably remove PAGE_POISONING_ZERO, because if we want to do the unpoisoning sanity check, then we also most likely want the 0xAA pattern and not zero. Thoughts?
On 10/27/20 2:32 PM, Vlastimil Babka wrote: > So my conclusion: > - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with > PAGE_POISONING_ZERO, and we can use init_on_free instead Note for this we first have to make sanity checking compatible with hibernation, but that should be easy as the zeroing variants already paved the way. The patch below will be added to the next version of the series: From 44474ee27c4f5248061ea2e5bbc2aeefc91bcdfc Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Tue, 27 Oct 2020 18:25:17 +0100 Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity checking Page poisoning used to be incompatible with hibernation, as the state of poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit 18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure free pages are cleared after resume. We can use the same mechanism to instead poison free pages with PAGE_POISON after resume. This covers both zero and 0xAA patterns. Thus we can remove the Kconfig restriction that disables page poison sanity checking when hibernation is enabled. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- kernel/power/hibernate.c | 2 +- kernel/power/power.h | 2 +- kernel/power/snapshot.c | 14 ++++++++++---- mm/Kconfig.debug | 1 - 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index 2fc7d509a34f..da0b41914177 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -326,7 +326,7 @@ static int create_image(int platform_mode) if (!in_suspend) { events_check_enabled = false; - clear_free_pages(); + clear_or_poison_free_pages(); } platform_leave(platform_mode); diff --git a/kernel/power/power.h b/kernel/power/power.h index 24f12d534515..778bf431ec02 100644 --- a/kernel/power/power.h +++ b/kernel/power/power.h @@ -106,7 +106,7 @@ extern int create_basic_memory_bitmaps(void); extern void free_basic_memory_bitmaps(void); extern int hibernate_preallocate_memory(void); -extern void clear_free_pages(void); +extern void clear_or_poison_free_pages(void); /** * Auxiliary structure used for reading the snapshot image data and diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index 46b1804c1ddf..6b1c84afa891 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -1144,7 +1144,7 @@ void free_basic_memory_bitmaps(void) pr_debug("Basic memory bitmaps freed\n"); } -void clear_free_pages(void) +void clear_or_poison_free_pages(void) { struct memory_bitmap *bm = free_pages_map; unsigned long pfn; @@ -1152,12 +1152,18 @@ void clear_free_pages(void) if (WARN_ON(!(free_pages_map))) return; - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { + if (page_poisoning_enabled() || want_init_on_free()) { memory_bm_position_reset(bm); pfn = memory_bm_next_pfn(bm); while (pfn != BM_END_OF_MAP) { - if (pfn_valid(pfn)) - clear_highpage(pfn_to_page(pfn)); + if (pfn_valid(pfn)) { + struct page *page = pfn_to_page(pfn); + if (page_poisoning_enabled_static()) + kernel_poison_pages(page, 1); + else if (want_init_on_free()) + clear_highpage(page); + + } pfn = memory_bm_next_pfn(bm); } diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug index 864f129f1937..c57786ad5be9 100644 --- a/mm/Kconfig.debug +++ b/mm/Kconfig.debug @@ -64,7 +64,6 @@ config PAGE_OWNER config PAGE_POISONING bool "Poison pages after freeing" - select PAGE_POISONING_NO_SANITY if HIBERNATION help Fill the pages with poison patterns after free_pages() and verify the patterns before alloc_pages. The filling of the memory helps
On 27.10.20 18:41, Vlastimil Babka wrote: > On 10/27/20 2:32 PM, Vlastimil Babka wrote: >> So my conclusion: >> - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with >> PAGE_POISONING_ZERO, and we can use init_on_free instead > > Note for this we first have to make sanity checking compatible with > hibernation, but that should be easy as the zeroing variants already > paved the way. The patch below will be added to the next version of > the series: > > From 44474ee27c4f5248061ea2e5bbc2aeefc91bcdfc Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Tue, 27 Oct 2020 18:25:17 +0100 > Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity > checking > > Page poisoning used to be incompatible with hibernation, as the state of > poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces > CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes > variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter > restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow > hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit > 18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure > free pages are cleared after resume. > > We can use the same mechanism to instead poison free pages with PAGE_POISON > after resume. This covers both zero and 0xAA patterns. Thus we can remove the > Kconfig restriction that disables page poison sanity checking when hibernation > is enabled. I haven't fully dived into the details, but the idea it sounds sane to me.
On Tue, Oct 27, 2020 at 2:32 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 10/27/20 12:05 PM, Vlastimil Babka wrote: > > On 10/27/20 10:10 AM, David Hildenbrand wrote: > >> On 26.10.20 18:33, Vlastimil Babka wrote: > >>> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when > >>> init_on_alloc is enabled, but will also always skip zeroing if the page was > >>> already zeroed on free by init_on_free or page poisoning. > >>> > >>> The latter check implemented by free_pages_prezeroed() can involve two > >>> different static keys. As prep_new_page() is really a hot path, let's introduce > >>> a single static key free_pages_not_prezeroed for this purpose and initialize it > >>> in init_mem_debugging(). > >> > >> Is this actually observable in practice? This smells like > >> micro-optimization to me. > >> > >> Also, I thought the whole reason for static keys is to have basically no > >> overhead at runtime, so I wonder if replacing two static key checks by a > >> single one actually makes *some* difference. > > > > You're right, the difference seems to be just a single NOP. The static key > > infrastructure seems to be working really well. > > (At least the asm inspection made me realize that kernel_poison_pages() is > > called unconditionally and the static key is checked inside, not inline so I'll > > be amending patch 2...) > > > > Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the > > code wrong. So unless others think it's a readability improvements, we can drop > > this patch. I agree with David that replacing two static keys with one is probably a micro-optimization. Also, if someone is enabling both init_on_alloc and init_on_free, they are already paying so much that no one is going to notice an extra static key. > > Or we can also reconsider this whole optimization. If the point is to be > > paranoid and enable both init_on_free and init_on_alloc, should we trust that > > nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex? I think we must trust the kernel to not overwrite zeroed pages. After all, this could theoretically happen at any time, not only while the memory chunk is freed. > More thoughts... > > PAGE_POISONING_NO_SANITY skips the check on "unpoisoning" whether poison was > corrupted > PAGE_POISONING_ZERO uses zero instead of 0xAA as poison pattern > > the point of enabling both of these seems to be moot now that init_on_free > exists, as that zeroes pages that are being freed, without checking on alloc > that they are still zeroed. > > What if only one is enabled? > - PAGE_POISONING_NO_SANITY without PAGE_POISONING_ZERO - we poison with the 0xAA > pattern but nobody checks it, so does it give us anything over init_on_free > writing zeroes? I don't think so? > > - PAGE_POISONING_ZERO without PAGE_POISONING_NO_SANITY - we use zeroes (like > init_on_free) but also check that it wasn't corrupted. We save some time on > writing zeroes again on alloc, but the check is still expensive. And writing > 0xAA would possibly detect more corruptions than writing zero (a stray write of > NULL is more likely to happen than of 0xAA?). > > So my conclusion: > - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with > PAGE_POISONING_ZERO, and we can use init_on_free instead Agreed. > - We can also probably remove PAGE_POISONING_ZERO, because if we want to do the > unpoisoning sanity check, then we also most likely want the 0xAA pattern and not > zero. Agreed. It might also make sense to somehow merge page poisoning and init_on_free together and have one config dimension instead of two (providing something similar to the INIT_STACK_NONE/INIT_STACK_ALL_ZERO/INIT_STACK_ALL_PATTERN configs) > Thoughts? >
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 2a1be197649d..980780f5f242 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -171,6 +171,8 @@ EXPORT_SYMBOL(init_on_alloc); DEFINE_STATIC_KEY_FALSE_RO(init_on_free); EXPORT_SYMBOL(init_on_free); +static DEFINE_STATIC_KEY_TRUE_RO(free_pages_not_prezeroed); + 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) @@ -777,6 +779,16 @@ void init_mem_debugging() } } + /* + * We have a special static key that controls whether prep_new_page will + * never need to zero the page. This mode is enabled when page is + * already zeroed by init_on_free or page_poisoning zero mode. + */ + if (_init_on_free_enabled_early || + (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) + && page_poisoning_enabled())) + static_branch_disable(&free_pages_not_prezeroed); + #ifdef CONFIG_PAGE_POISONING /* * Page poisoning is debug page alloc for some arches. If @@ -2216,12 +2228,6 @@ static inline int check_new_page(struct page *page) return 1; } -static inline bool free_pages_prezeroed(void) -{ - return (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) && - page_poisoning_enabled_static()) || want_init_on_free(); -} - #ifdef CONFIG_DEBUG_VM /* * With DEBUG_VM enabled, order-0 pages are checked for expected state when @@ -2291,7 +2297,8 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags { post_alloc_hook(page, order, gfp_flags); - if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags)) + if (static_branch_likely(&free_pages_not_prezeroed) + && want_init_on_alloc(gfp_flags)) kernel_init_free_pages(page, 1 << order); if (order && (gfp_flags & __GFP_COMP))
prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when init_on_alloc is enabled, but will also always skip zeroing if the page was already zeroed on free by init_on_free or page poisoning. The latter check implemented by free_pages_prezeroed() can involve two different static keys. As prep_new_page() is really a hot path, let's introduce a single static key free_pages_not_prezeroed for this purpose and initialize it in init_mem_debugging(). Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/page_alloc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)