From: Laura Abbott <labbott@redhat.com>
To: Vinayak Menon <vinmenon@codeaurora.org>,
iamjoonsoo.kim@lge.com, mhocko@suse.com,
akpm@linux-foundation.org
Cc: shashim@codeaurora.org, linux-mm@kvack.org
Subject: Re: [PATCH] mm: enable page poisoning early at boot
Date: Wed, 29 Mar 2017 10:04:47 -0700 [thread overview]
Message-ID: <d9e8b184-b2a9-1174-4a6b-17ae1d2d6444@redhat.com> (raw)
In-Reply-To: <1490358246-11001-1-git-send-email-vinmenon@codeaurora.org>
On 03/24/2017 05:24 AM, Vinayak Menon wrote:
> On SPARSEMEM systems page poisoning is enabled after buddy is up, because
> of the dependency on page extension init. This causes the pages released
> by free_all_bootmem not to be poisoned. This either delays or misses
> the identification of some issues because the pages have to undergo another
> cycle of alloc-free-alloc for any corruption to be detected.
> Enable page poisoning early by getting rid of the PAGE_EXT_DEBUG_POISON
> flag. Since all the free pages will now be poisoned, the flag need not be
> verified before checking the poison during an alloc.
>
> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
> ---
>
> An RFC was sent earlier (http://www.spinics.net/lists/linux-mm/msg123142.html)
> Not sure if there exists a code path that can free pages to buddy skipping
> kernel_poison_pages, making the flag PAGE_EXT_DEBUG_POISON a necessity. But
> the tests have not shown any issues. As per Laura's suggestion, the patch was
> tested with HIBERNATION enabled and no issues were seen.
>
I gave this a spin on some of my machines and it appears to be working
okay. I wish we had a bit more context about why it was necessary to track
the poison in the page itself.
This change means that we shouldn't need the "select PAGE_EXTENSION"
anymore so that can be dropped. If you do that, you can add
Acked-by: Laura Abbott <labbott@redhat.com>
> include/linux/mm.h | 1 -
> mm/page_alloc.c | 13 +++------
> mm/page_ext.c | 3 ---
> mm/page_poison.c | 77 +++++++++---------------------------------------------
> 4 files changed, 15 insertions(+), 79 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0d65dd7..b881966 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2473,7 +2473,6 @@ extern long copy_huge_page_from_user(struct page *dst_page,
> #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_HUGETLBFS */
>
> extern struct page_ext_operations debug_guardpage_ops;
> -extern struct page_ext_operations page_poisoning_ops;
>
> #ifdef CONFIG_DEBUG_PAGEALLOC
> extern unsigned int _debug_guardpage_minorder;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index fc5db1b..860b36f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1694,10 +1694,10 @@ static inline int check_new_page(struct page *page)
> return 1;
> }
>
> -static inline bool free_pages_prezeroed(bool poisoned)
> +static inline bool free_pages_prezeroed(void)
> {
> return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
> - page_poisoning_enabled() && poisoned;
> + page_poisoning_enabled();
> }
>
> #ifdef CONFIG_DEBUG_VM
> @@ -1751,17 +1751,10 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
> unsigned int alloc_flags)
> {
> int i;
> - bool poisoned = true;
> -
> - for (i = 0; i < (1 << order); i++) {
> - struct page *p = page + i;
> - if (poisoned)
> - poisoned &= page_is_poisoned(p);
> - }
>
> post_alloc_hook(page, order, gfp_flags);
>
> - if (!free_pages_prezeroed(poisoned) && (gfp_flags & __GFP_ZERO))
> + if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
> for (i = 0; i < (1 << order); i++)
> clear_highpage(page + i);
>
> diff --git a/mm/page_ext.c b/mm/page_ext.c
> index 121dcff..fc3e7ff 100644
> --- a/mm/page_ext.c
> +++ b/mm/page_ext.c
> @@ -59,9 +59,6 @@
>
> static struct page_ext_operations *page_ext_ops[] = {
> &debug_guardpage_ops,
> -#ifdef CONFIG_PAGE_POISONING
> - &page_poisoning_ops,
> -#endif
> #ifdef CONFIG_PAGE_OWNER
> &page_owner_ops,
> #endif
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 2e647c6..be19e98 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -6,7 +6,6 @@
> #include <linux/poison.h>
> #include <linux/ratelimit.h>
>
> -static bool __page_poisoning_enabled __read_mostly;
> static bool want_page_poisoning __read_mostly;
>
> static int early_page_poison_param(char *buf)
> @@ -19,74 +18,21 @@ static int early_page_poison_param(char *buf)
>
> bool page_poisoning_enabled(void)
> {
> - return __page_poisoning_enabled;
> -}
> -
> -static bool need_page_poisoning(void)
> -{
> - return want_page_poisoning;
> -}
> -
> -static void init_page_poisoning(void)
> -{
> /*
> - * page poisoning is debug page alloc for some arches. If either
> - * of those options are enabled, enable poisoning
> + * Assumes that debug_pagealloc_enabled is set before
> + * free_all_bootmem.
> + * Page poisoning is debug page alloc for some arches. If
> + * either of those options are enabled, enable poisoning.
> */
> - if (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC)) {
> - if (!want_page_poisoning && !debug_pagealloc_enabled())
> - return;
> - } else {
> - if (!want_page_poisoning)
> - return;
> - }
> -
> - __page_poisoning_enabled = true;
> -}
> -
> -struct page_ext_operations page_poisoning_ops = {
> - .need = need_page_poisoning,
> - .init = init_page_poisoning,
> -};
> -
> -static inline void set_page_poison(struct page *page)
> -{
> - struct page_ext *page_ext;
> -
> - page_ext = lookup_page_ext(page);
> - if (unlikely(!page_ext))
> - return;
> -
> - __set_bit(PAGE_EXT_DEBUG_POISON, &page_ext->flags);
> -}
> -
> -static inline void clear_page_poison(struct page *page)
> -{
> - struct page_ext *page_ext;
> -
> - page_ext = lookup_page_ext(page);
> - if (unlikely(!page_ext))
> - return;
> -
> - __clear_bit(PAGE_EXT_DEBUG_POISON, &page_ext->flags);
> -}
> -
> -bool page_is_poisoned(struct page *page)
> -{
> - struct page_ext *page_ext;
> -
> - page_ext = lookup_page_ext(page);
> - if (unlikely(!page_ext))
> - return false;
> -
> - return test_bit(PAGE_EXT_DEBUG_POISON, &page_ext->flags);
> + return (want_page_poisoning ||
> + (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> + debug_pagealloc_enabled()));
> }
>
> static void poison_page(struct page *page)
> {
> void *addr = kmap_atomic(page);
>
> - set_page_poison(page);
> memset(addr, PAGE_POISON, PAGE_SIZE);
> kunmap_atomic(addr);
> }
> @@ -140,12 +86,13 @@ static void unpoison_page(struct page *page)
> {
> void *addr;
>
> - if (!page_is_poisoned(page))
> - return;
> -
> addr = kmap_atomic(page);
> + /*
> + * Page poisoning when enabled poisons each and every page
> + * that is freed to buddy. Thus no extra check is done to
> + * see if a page was posioned.
> + */
> check_poison_mem(addr, PAGE_SIZE);
> - clear_page_poison(page);
> kunmap_atomic(addr);
> }
>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-03-29 17:04 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-24 12:24 [PATCH] mm: enable page poisoning early at boot Vinayak Menon
2017-03-29 17:04 ` Laura Abbott [this message]
2017-03-29 17:14 ` Laura Abbott
2017-03-30 12:45 ` Vinayak Menon
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=d9e8b184-b2a9-1174-4a6b-17ae1d2d6444@redhat.com \
--to=labbott@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=shashim@codeaurora.org \
--cc=vinmenon@codeaurora.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).