| Message ID | 20181119101616.8901-9-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/8] mm: balloon: update comment about isolation/migration/compaction | expand |
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; >
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
>
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
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;