[v1,8/8] PM / Hibernate: exclude all PageOffline() pages
diff mbox series

Message ID 20181119101616.8901-9-david@redhat.com
State In Next
Commit bb354d7008b1f9bd41fddd619f89f78581c33a23
Headers show
Series
  • [v1,1/8] mm: balloon: update comment about isolation/migration/compaction
Related show

Commit Message

David Hildenbrand Nov. 19, 2018, 10:16 a.m. UTC
The content of pages that are marked PG_offline is not of interest
(e.g. inflated by a balloon driver), let's skip these pages.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 kernel/power/snapshot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki Nov. 20, 2018, 9:22 p.m. UTC | #1
On Monday, November 19, 2018 11:16:16 AM CET David Hildenbrand wrote:
> The content of pages that are marked PG_offline is not of interest
> (e.g. inflated by a balloon driver), let's skip these pages.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  kernel/power/snapshot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 87e6dd57819f..8d7b4d458842 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
>  	BUG_ON(!PageHighMem(page));
>  
>  	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
> -	    PageReserved(page))
> +	    PageReserved(page) || PageOffline(page))
>  		return NULL;
>  
>  	if (page_is_guard(page))
> @@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
>  	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
>  		return NULL;
>  
> +	if (PageOffline(page))
> +		return NULL;
> +
>  	if (PageReserved(page)
>  	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
>  		return NULL;
>
William Kucharski Nov. 21, 2018, 11:35 a.m. UTC | #2
If you are adding PageOffline(page) to the condition list of the already existing if in
saveable_highmem_page(), why explicitly add it as a separate statement in saveable_page()?

It would seem more consistent to make the second check:

-	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
+	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page) ||
+		PageOffline(page))

instead.

It's admittedly a nit but it just seems cleaner to either do that or, if your intention
was to separate the Page checks from the swsusp checks, to break the calls to
PageReserved() and PageOffline() into their own check in saveable_highmem_page().

Thanks!
    -- Bill
     

> On Nov 19, 2018, at 3:16 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
> 	BUG_ON(!PageHighMem(page));
> 
> 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
> -	    PageReserved(page))
> +	    PageReserved(page) || PageOffline(page))
> 		return NULL;
> 
> 	if (page_is_guard(page))
> @@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
> 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
> 		return NULL;
> 
> +	if (PageOffline(page))
> +		return NULL;
> +
> 	if (PageReserved(page)
> 	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
> 		return NULL;
> -- 
> 2.17.2
>
David Hildenbrand Nov. 21, 2018, 12:21 p.m. UTC | #3
On 21.11.18 12:35, William Kucharski wrote:
> If you are adding PageOffline(page) to the condition list of the already existing if in
> saveable_highmem_page(), why explicitly add it as a separate statement in saveable_page()?
> 
> It would seem more consistent to make the second check:
> 
> -	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
> +	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page) ||
> +		PageOffline(page))
> 
> instead.
> 
> It's admittedly a nit but it just seems cleaner to either do that or, if your intention
> was to separate the Page checks from the swsusp checks, to break the calls to
> PageReserved() and PageOffline() into their own check in saveable_highmem_page().

I'll split PageReserved() and PageOffline() off from the swsusp checks,
thanks for your comment!

> 
> Thanks!
>     -- Bill

Patch
diff mbox series

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 87e6dd57819f..8d7b4d458842 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1222,7 +1222,7 @@  static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))
@@ -1286,6 +1286,9 @@  static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
 
+	if (PageOffline(page))
+		return NULL;
+
 	if (PageReserved(page)
 	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
 		return NULL;