[PATCH v2 0/5] cleanup page poisoning

[PATCH v2 0/5] cleanup page poisoning

[Vlastimil Babka]

The first version was called "optimize handling of memory debugging parameters"
[1], changes are:

- apply review feedback
- drop former Patch 3
- add new patches 3-5, change name and cover letter of series

I have identified a number of issues and opportunities for cleanup with
CONFIG_PAGE_POISON and friends:
- interaction with init_on_alloc and init_on_free parameters depends on
  the order of parameters (Patch 1)
- the boot time enabling uses static key, but inefficienty (Patch 2)
- sanity checking is incompatible with hibernation (Patch 3)
- CONFIG_PAGE_POISONING_NO_SANITY can be removed now that we have init_on_free
  (Patch 4)
- CONFIG_PAGE_POISONING_ZERO can be most likely removed now that we have
  init_on_free (Patch 5)

[1] https://lore.kernel.org/r/20201026173358.14704-1-vbabka@suse.cz

Vlastimil Babka (5):
  mm, page_alloc: do not rely on the order of page_poison and
    init_on_alloc/free parameters
  mm, page_poison: use static key more efficiently
  kernel/power: allow hibernation with page_poison sanity checking
  mm, page_poison: remove CONFIG_PAGE_POISONING_NO_SANITY
  mm, page_poison: remove CONFIG_PAGE_POISONING_ZERO

 drivers/virtio/virtio_balloon.c |   4 +-
 include/linux/mm.h              |  43 ++++++------
 include/linux/poison.h          |   4 --
 init/main.c                     |   2 +-
 kernel/power/hibernate.c        |   2 +-
 kernel/power/power.h            |   2 +-
 kernel/power/snapshot.c         |  14 ++--
 mm/Kconfig.debug                |  28 ++------
 mm/page_alloc.c                 | 113 ++++++++++++++++----------------
 mm/page_poison.c                |  56 ++--------------
 tools/include/linux/poison.h    |   6 +-
 11 files changed, 106 insertions(+), 168 deletions(-)

-- 
2.29.1

[PATCH v2 3/5] kernel/power: allow hibernation with page_poison sanity checking

[Vlastimil Babka]

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>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: <linux-pm@vger.kernel.org>
---
 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
-- 
2.29.1

Re: [PATCH v2 3/5] kernel/power: allow hibernation with page_poison sanity checking

[Rafael J. Wysocki]

On Tue, Nov 3, 2020 at 4:24 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> 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>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: <linux-pm@vger.kernel.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the hibernation part and I'm assuming that this patch will be
routed through the mm tree.

> ---
>  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
> --
> 2.29.1
>

Re: [PATCH v2 3/5] kernel/power: allow hibernation with page_poison sanity checking

[David Hildenbrand]

On 03.11.20 16:22, Vlastimil Babka wrote:
> 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>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: <linux-pm@vger.kernel.org>
> ---
>   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);

^ empty line missing. And at least I prefer to declare all variables in 
the function header.

I'd even suggest to move this into a separate function like

clear_or_poison_free_page(struct page *page)


> +				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
> 

Unless I am missing something important, this should work! Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/5] kernel/power: allow hibernation with page_poison sanity checking

[Vlastimil Babka]

On 11/11/20 4:42 PM, David Hildenbrand wrote:
...
>> @@ -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);
> 
> ^ empty line missing. And at least I prefer to declare all variables in
> the function header.
> 
> I'd even suggest to move this into a separate function like
> 
> clear_or_poison_free_page(struct page *page)
> 
> 

Ok, fixup below.

----8<----
 From cae1e8ccfa57c28ed1b2f5f8a47319b86cbdcfbf Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 12 Nov 2020 15:33:07 +0100
Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity
  checking-fix

Adapt to __kernel_unpoison_pages fixup. Split out clear_or_poison_free_page()
per David Hildenbrand.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
  include/linux/mm.h      |  1 +
  kernel/power/snapshot.c | 18 ++++++++++--------
  2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 861b9392b5dc..d4cfb06a611e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2896,6 +2896,7 @@ static inline void kernel_unpoison_pages(struct page *page, int numpages)
  #else
  static inline bool page_poisoning_enabled(void) { return false; }
  static inline bool page_poisoning_enabled_static(void) { return false; }
+static inline void __kernel_poison_pages(struct page *page, int nunmpages) { }
  static inline void kernel_poison_pages(struct page *page, int numpages) { }
  static inline void kernel_unpoison_pages(struct page *page, int numpages) { }
  #endif
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 6b1c84afa891..a3491b29c5cc 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1144,6 +1144,14 @@ void free_basic_memory_bitmaps(void)
  	pr_debug("Basic memory bitmaps freed\n");
  }
  
+static void clear_or_poison_free_page(struct page *page)
+{
+	if (page_poisoning_enabled_static())
+		__kernel_poison_pages(page, 1);
+	else if (want_init_on_free())
+		clear_highpage(page);
+}
+
  void clear_or_poison_free_pages(void)
  {
  	struct memory_bitmap *bm = free_pages_map;
@@ -1156,14 +1164,8 @@ void clear_or_poison_free_pages(void)
  		memory_bm_position_reset(bm);
  		pfn = memory_bm_next_pfn(bm);
  		while (pfn != BM_END_OF_MAP) {
-			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);
-
-			}
+			if (pfn_valid(pfn))
+				clear_or_poison_free_page(pfn_to_page(pfn));
  
  			pfn = memory_bm_next_pfn(bm);
  		}
-- 
2.29.1

Re: [PATCH v2 3/5] kernel/power: allow hibernation with page_poison sanity checking

[Rafael J. Wysocki]

On Thu, Nov 12, 2020 at 3:42 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 11/11/20 4:42 PM, David Hildenbrand wrote:
> ...
> >> @@ -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);
> >
> > ^ empty line missing. And at least I prefer to declare all variables in
> > the function header.
> >
> > I'd even suggest to move this into a separate function like
> >
> > clear_or_poison_free_page(struct page *page)
> >
> >
>
> Ok, fixup below.
>
> ----8<----
>  From cae1e8ccfa57c28ed1b2f5f8a47319b86cbdcfbf Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 12 Nov 2020 15:33:07 +0100
> Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity
>   checking-fix
>
> Adapt to __kernel_unpoison_pages fixup. Split out clear_or_poison_free_page()
> per David Hildenbrand.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Still

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>   include/linux/mm.h      |  1 +
>   kernel/power/snapshot.c | 18 ++++++++++--------
>   2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 861b9392b5dc..d4cfb06a611e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2896,6 +2896,7 @@ static inline void kernel_unpoison_pages(struct page *page, int numpages)
>   #else
>   static inline bool page_poisoning_enabled(void) { return false; }
>   static inline bool page_poisoning_enabled_static(void) { return false; }
> +static inline void __kernel_poison_pages(struct page *page, int nunmpages) { }
>   static inline void kernel_poison_pages(struct page *page, int numpages) { }
>   static inline void kernel_unpoison_pages(struct page *page, int numpages) { }
>   #endif
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 6b1c84afa891..a3491b29c5cc 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1144,6 +1144,14 @@ void free_basic_memory_bitmaps(void)
>         pr_debug("Basic memory bitmaps freed\n");
>   }
>
> +static void clear_or_poison_free_page(struct page *page)
> +{
> +       if (page_poisoning_enabled_static())
> +               __kernel_poison_pages(page, 1);
> +       else if (want_init_on_free())
> +               clear_highpage(page);
> +}
> +
>   void clear_or_poison_free_pages(void)
>   {
>         struct memory_bitmap *bm = free_pages_map;
> @@ -1156,14 +1164,8 @@ void clear_or_poison_free_pages(void)
>                 memory_bm_position_reset(bm);
>                 pfn = memory_bm_next_pfn(bm);
>                 while (pfn != BM_END_OF_MAP) {
> -                       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);
> -
> -                       }
> +                       if (pfn_valid(pfn))
> +                               clear_or_poison_free_page(pfn_to_page(pfn));
>
>                         pfn = memory_bm_next_pfn(bm);
>                 }
> --
> 2.29.1
>
>

Re: [PATCH v2 3/5] kernel/power: allow hibernation with page_poison sanity checking

[David Hildenbrand]

On 12.11.20 15:39, Vlastimil Babka wrote:
> On 11/11/20 4:42 PM, David Hildenbrand wrote:
> ...
>>> @@ -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);
>>
>> ^ empty line missing. And at least I prefer to declare all variables in
>> the function header.
>>
>> I'd even suggest to move this into a separate function like
>>
>> clear_or_poison_free_page(struct page *page)
>>
>>
> 
> Ok, fixup below.
> 
> ----8<----
>   From cae1e8ccfa57c28ed1b2f5f8a47319b86cbdcfbf Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 12 Nov 2020 15:33:07 +0100
> Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity
>    checking-fix
> 
> Adapt to __kernel_unpoison_pages fixup. Split out clear_or_poison_free_page()
> per David Hildenbrand.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>    include/linux/mm.h      |  1 +
>    kernel/power/snapshot.c | 18 ++++++++++--------
>    2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 861b9392b5dc..d4cfb06a611e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2896,6 +2896,7 @@ static inline void kernel_unpoison_pages(struct page *page, int numpages)
>    #else
>    static inline bool page_poisoning_enabled(void) { return false; }
>    static inline bool page_poisoning_enabled_static(void) { return false; }
> +static inline void __kernel_poison_pages(struct page *page, int nunmpages) { }
>    static inline void kernel_poison_pages(struct page *page, int numpages) { }
>    static inline void kernel_unpoison_pages(struct page *page, int numpages) { }
>    #endif
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 6b1c84afa891..a3491b29c5cc 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1144,6 +1144,14 @@ void free_basic_memory_bitmaps(void)
>    	pr_debug("Basic memory bitmaps freed\n");
>    }
>    
> +static void clear_or_poison_free_page(struct page *page)
> +{
> +	if (page_poisoning_enabled_static())
> +		__kernel_poison_pages(page, 1);
> +	else if (want_init_on_free())
> +		clear_highpage(page);
> +}
> +
>    void clear_or_poison_free_pages(void)
>    {
>    	struct memory_bitmap *bm = free_pages_map;
> @@ -1156,14 +1164,8 @@ void clear_or_poison_free_pages(void)
>    		memory_bm_position_reset(bm);
>    		pfn = memory_bm_next_pfn(bm);
>    		while (pfn != BM_END_OF_MAP) {
> -			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);
> -
> -			}
> +			if (pfn_valid(pfn))
> +				clear_or_poison_free_page(pfn_to_page(pfn));
>    
>    			pfn = memory_bm_next_pfn(bm);
>    		}
> 

LGTM, thanks!

-- 
Thanks,

David / dhildenb