From: David Hildenbrand <david@redhat.com>
To: Vlastimil Babka <vbabka@suse.cz>,
Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Alexander Potapenko <glider@google.com>,
Kees Cook <keescook@chromium.org>,
Michal Hocko <mhocko@kernel.org>,
Mateusz Nosek <mateusznosek0@gmail.com>,
Laura Abbott <labbott@kernel.org>
Subject: Re: [PATCH v3 2/5] mm, page_poison: use static key more efficiently
Date: Fri, 13 Nov 2020 13:08:50 +0100 [thread overview]
Message-ID: <4fea3200-70f8-620a-ec87-735882a530a5@redhat.com> (raw)
In-Reply-To: <20201113104033.22907-3-vbabka@suse.cz>
On 13.11.20 11:40, Vlastimil Babka wrote:
> Commit 11c9c7edae06 ("mm/page_poison.c: replace bool variable with static key")
> changed page_poisoning_enabled() to a static key check. However, the function
> is not inlined, so each check still involves a function call with overhead not
> eliminated when page poisoning is disabled.
>
> Analogically to how debug_pagealloc is handled, this patch converts
> page_poisoning_enabled() back to boolean check, and introduces
> page_poisoning_enabled_static() for fast paths. Both functions are inlined.
>
> The function kernel_poison_pages() is also called unconditionally and does
> the static key check inside. Remove it from there and put it to callers. Also
> split it to two functions kernel_poison_pages() and kernel_unpoison_pages()
> instead of the confusing bool parameter.
>
> Also optimize the check that enables page poisoning instead of debug_pagealloc
> for architectures without proper debug_pagealloc support. Move the check to
> init_mem_debugging_and_hardening() to enable a single static key instead of
> having two static branches in page_poisoning_enabled_static().
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> drivers/virtio/virtio_balloon.c | 2 +-
> include/linux/mm.h | 33 +++++++++++++++++---
> mm/page_alloc.c | 18 +++++++++--
> mm/page_poison.c | 53 +++++----------------------------
> 4 files changed, 52 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 481611c09dae..e53faed6ba93 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1116,7 +1116,7 @@ static int virtballoon_validate(struct virtio_device *vdev)
> */
> if (!want_init_on_free() &&
> (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> - !page_poisoning_enabled()))
> + !page_poisoning_enabled_static()))
> __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON))
> __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 82ab5c894d94..5ab5358be2b3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2862,12 +2862,37 @@ extern int apply_to_existing_page_range(struct mm_struct *mm,
>
> extern void init_mem_debugging_and_hardening(void);
> #ifdef CONFIG_PAGE_POISONING
> -extern bool page_poisoning_enabled(void);
> -extern void kernel_poison_pages(struct page *page, int numpages, int enable);
> +extern void __kernel_poison_pages(struct page *page, int numpages);
> +extern void __kernel_unpoison_pages(struct page *page, int numpages);
> +extern bool _page_poisoning_enabled_early;
> +DECLARE_STATIC_KEY_FALSE(_page_poisoning_enabled);
> +static inline bool page_poisoning_enabled(void)
> +{
> + return _page_poisoning_enabled_early;
> +}
> +/*
> + * For use in fast paths after init_mem_debugging() has run, or when a
> + * false negative result is not harmful when called too early.
> + */
> +static inline bool page_poisoning_enabled_static(void)
> +{
> + return static_branch_unlikely(&_page_poisoning_enabled);
> +}
> +static inline void kernel_poison_pages(struct page *page, int numpages)
> +{
> + if (page_poisoning_enabled_static())
> + __kernel_poison_pages(page, numpages);
> +}
> +static inline void kernel_unpoison_pages(struct page *page, int numpages)
> +{
> + if (page_poisoning_enabled_static())
> + __kernel_unpoison_pages(page, numpages);
> +}
> #else
> static inline bool page_poisoning_enabled(void) { return false; }
> -static inline void kernel_poison_pages(struct page *page, int numpages,
> - int enable) { }
> +static inline bool page_poisoning_enabled_static(void) { return false; }
> +static inline void kernel_poison_pages(struct page *page, int numpages) { }
> +static inline void kernel_unpoison_pages(struct page *page, int numpages) { }
> #endif
>
> DECLARE_STATIC_KEY_FALSE(init_on_alloc);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 567060c2ad83..cd966829bed3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -775,6 +775,17 @@ void init_mem_debugging_and_hardening(void)
> static_branch_enable(&init_on_free);
> }
>
> +#ifdef CONFIG_PAGE_POISONING
> + /*
> + * Page poisoning is debug page alloc for some arches. If
> + * either of those options are enabled, enable poisoning.
> + */
> + if (page_poisoning_enabled() ||
> + (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> + debug_pagealloc_enabled()))
> + static_branch_enable(&_page_poisoning_enabled);
> +#endif
> +
> #ifdef CONFIG_DEBUG_PAGEALLOC
> if (!debug_pagealloc_enabled())
> return;
> @@ -1262,7 +1273,8 @@ static __always_inline bool free_pages_prepare(struct page *page,
> if (want_init_on_free())
> kernel_init_free_pages(page, 1 << order);
>
> - kernel_poison_pages(page, 1 << order, 0);
> + kernel_poison_pages(page, 1 << order);
> +
> /*
> * arch_free_page() can make the page's contents inaccessible. s390
> * does this. So nothing which can access the page's contents should
> @@ -2217,7 +2229,7 @@ static inline int check_new_page(struct page *page)
> static inline bool free_pages_prezeroed(void)
> {
> return (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
> - page_poisoning_enabled()) || want_init_on_free();
> + page_poisoning_enabled_static()) || want_init_on_free();
> }
>
> #ifdef CONFIG_DEBUG_VM
> @@ -2279,7 +2291,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> arch_alloc_page(page, order);
> debug_pagealloc_map_pages(page, 1 << order);
> kasan_alloc_pages(page, order);
> - kernel_poison_pages(page, 1 << order, 1);
> + kernel_unpoison_pages(page, 1 << order);
> set_page_owner(page, order, gfp_flags);
> }
>
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index e6c994af7518..0d899a01d107 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -8,45 +8,17 @@
> #include <linux/ratelimit.h>
> #include <linux/kasan.h>
>
> -static DEFINE_STATIC_KEY_FALSE_RO(want_page_poisoning);
> +bool _page_poisoning_enabled_early;
> +EXPORT_SYMBOL(_page_poisoning_enabled_early);
> +DEFINE_STATIC_KEY_FALSE(_page_poisoning_enabled);
> +EXPORT_SYMBOL(_page_poisoning_enabled);
>
> static int __init early_page_poison_param(char *buf)
> {
> - int ret;
> - bool tmp;
> -
> - ret = strtobool(buf, &tmp);
> - if (ret)
> - return ret;
> -
> - if (tmp)
> - static_branch_enable(&want_page_poisoning);
> - else
> - static_branch_disable(&want_page_poisoning);
> -
> - return 0;
> + return kstrtobool(buf, &_page_poisoning_enabled_early);
> }
> early_param("page_poison", early_page_poison_param);
>
> -/**
> - * page_poisoning_enabled - check if page poisoning is enabled
> - *
> - * Return true if page poisoning is enabled, or false if not.
> - */
> -bool page_poisoning_enabled(void)
> -{
> - /*
> - * Assumes that debug_pagealloc_enabled is set before
> - * memblock_free_all.
> - * Page poisoning is debug page alloc for some arches. If
> - * either of those options are enabled, enable poisoning.
> - */
> - return (static_branch_unlikely(&want_page_poisoning) ||
> - (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> - debug_pagealloc_enabled()));
> -}
> -EXPORT_SYMBOL_GPL(page_poisoning_enabled);
> -
> static void poison_page(struct page *page)
> {
> void *addr = kmap_atomic(page);
> @@ -58,7 +30,7 @@ static void poison_page(struct page *page)
> kunmap_atomic(addr);
> }
>
> -static void poison_pages(struct page *page, int n)
> +void __kernel_poison_pages(struct page *page, int n)
> {
> int i;
>
> @@ -117,7 +89,7 @@ static void unpoison_page(struct page *page)
> kunmap_atomic(addr);
> }
>
> -static void unpoison_pages(struct page *page, int n)
> +void __kernel_unpoison_pages(struct page *page, int n)
> {
> int i;
>
> @@ -125,17 +97,6 @@ static void unpoison_pages(struct page *page, int n)
> unpoison_page(page + i);
> }
>
> -void kernel_poison_pages(struct page *page, int numpages, int enable)
> -{
> - if (!page_poisoning_enabled())
> - return;
> -
> - if (enable)
> - unpoison_pages(page, numpages);
> - else
> - poison_pages(page, numpages);
> -}
> -
> #ifndef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
> void __kernel_map_pages(struct page *page, int numpages, int enable)
> {
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2020-11-13 12:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-13 10:40 [PATCH v3 0/5] cleanup page poisoning Vlastimil Babka
2020-11-13 10:40 ` [PATCH v3 1/5] mm, page_alloc: do not rely on the order of page_poison and init_on_alloc/free parameters Vlastimil Babka
2020-11-13 10:40 ` [PATCH v3 2/5] mm, page_poison: use static key more efficiently Vlastimil Babka
2020-11-13 12:08 ` David Hildenbrand [this message]
2020-11-13 10:40 ` [PATCH v3 3/5] kernel/power: allow hibernation with page_poison sanity checking Vlastimil Babka
2020-11-13 12:10 ` David Hildenbrand
2020-11-13 12:15 ` Vlastimil Babka
2020-11-13 12:17 ` David Hildenbrand
2020-11-13 10:40 ` [PATCH v3 4/5] mm, page_poison: remove CONFIG_PAGE_POISONING_NO_SANITY Vlastimil Babka
2020-11-13 10:40 ` [PATCH v3 5/5] mm, page_poison: remove CONFIG_PAGE_POISONING_ZERO Vlastimil Babka
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=4fea3200-70f8-620a-ec87-735882a530a5@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=glider@google.com \
--cc=keescook@chromium.org \
--cc=labbott@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mateusznosek0@gmail.com \
--cc=mhocko@kernel.org \
--cc=vbabka@suse.cz \
/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.