diff mbox series

[v2,04/10] power: separate PageHighMem() and PageHighMemZone() use case

Message ID 1588130803-20527-5-git-send-email-iamjoonsoo.kim@lge.com (mailing list archive)
State New, archived
Headers show
Series change the implementation of the PageHighMem() | expand

Commit Message

Joonsoo Kim April 29, 2020, 3:26 a.m. UTC 
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Until now, PageHighMem() is used for two different cases. One is to check
if there is a direct mapping for this page or not. The other is to check
the zone of this page, that is, weather it is the highmem type zone or not.

Now, we have separate functions, PageHighMem() and PageHighMemZone() for
each cases. Use appropriate one.

Note that there are some rules to determine the proper macro.

1. If PageHighMem() is called for checking if the direct mapping exists
or not, use PageHighMem().
2. If PageHighMem() is used to predict the previous gfp_flags for
this page, use PageHighMemZone(). The zone of the page is related to
the gfp_flags.
3. If purpose of calling PageHighMem() is to count highmem page and
to interact with the system by using this count, use PageHighMemZone().
This counter is usually used to calculate the available memory for an
kernel allocation and pages on the highmem zone cannot be available
for an kernel allocation.
4. Otherwise, use PageHighMemZone(). It's safe since it's implementation
is just copy of the previous PageHighMem() implementation and won't
be changed.

I apply the rule #3 for this patch.

Acked-by: Roman Gushchin <guro@fb.com>
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 kernel/power/snapshot.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig May 1, 2020, 12:22 p.m. UTC | #1 
On Wed, Apr 29, 2020 at 12:26:37PM +0900, js1304@gmail.com wrote:
> index 6598001..be759a6 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1227,7 +1227,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
>  	if (!page || page_zone(page) != zone)
>  		return NULL;
>  
> -	BUG_ON(!PageHighMem(page));
> +	BUG_ON(!PageHighMemZone(page));

The above check already checks for the highmem zone.  So if we want
to keep the BUG_ON it needs stay PageHighMem to make sense.  That being
said I'd rather remove it entirelẏ.

> -	BUG_ON(PageHighMem(page));
> +	BUG_ON(PageHighMemZone(page));

Same here.

> @@ -1529,7 +1529,7 @@ static unsigned long preallocate_image_pages(unsigned long nr_pages, gfp_t mask)
>  		if (!page)
>  			break;
>  		memory_bm_set_bit(&copy_bm, page_to_pfn(page));
> -		if (PageHighMem(page))
> +		if (PageHighMemZone(page))
>  			alloc_highmem++;
>  		else
>  			alloc_normal++;

I don't fully understand the log here.  Can Pavel or Rafael clarify
why swsupst would care about the exact zone?
Joonsoo Kim May 4, 2020, 3:01 a.m. UTC | #2 
2020년 5월 1일 (금) 오후 9:22, Christoph Hellwig <hch@infradead.org>님이 작성:
>
> On Wed, Apr 29, 2020 at 12:26:37PM +0900, js1304@gmail.com wrote:
> > index 6598001..be759a6 100644
> > --- a/kernel/power/snapshot.c
> > +++ b/kernel/power/snapshot.c
> > @@ -1227,7 +1227,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
> >       if (!page || page_zone(page) != zone)
> >               return NULL;
> >
> > -     BUG_ON(!PageHighMem(page));
> > +     BUG_ON(!PageHighMemZone(page));
>
> The above check already checks for the highmem zone.  So if we want
> to keep the BUG_ON it needs stay PageHighMem to make sense.  That being
> said I'd rather remove it entirelẏ.

Okay.

> > -     BUG_ON(PageHighMem(page));
> > +     BUG_ON(PageHighMemZone(page));
>
> Same here.

Okay.

Thanks.
diff mbox series

Patch

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 6598001..be759a6 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1227,7 +1227,7 @@  static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	if (!page || page_zone(page) != zone)
 		return NULL;
 
-	BUG_ON(!PageHighMem(page));
+	BUG_ON(!PageHighMemZone(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page))
 		return NULL;
@@ -1291,7 +1291,7 @@  static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 	if (!page || page_zone(page) != zone)
 		return NULL;
 
-	BUG_ON(PageHighMem(page));
+	BUG_ON(PageHighMemZone(page));
 
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
@@ -1529,7 +1529,7 @@  static unsigned long preallocate_image_pages(unsigned long nr_pages, gfp_t mask)
 		if (!page)
 			break;
 		memory_bm_set_bit(&copy_bm, page_to_pfn(page));
-		if (PageHighMem(page))
+		if (PageHighMemZone(page))
 			alloc_highmem++;
 		else
 			alloc_normal++;
@@ -1625,7 +1625,7 @@  static unsigned long free_unnecessary_pages(void)
 		unsigned long pfn = memory_bm_next_pfn(&copy_bm);
 		struct page *page = pfn_to_page(pfn);
 
-		if (PageHighMem(page)) {
+		if (PageHighMemZone(page)) {
 			if (!to_free_highmem)
 				continue;
 			to_free_highmem--;
@@ -2264,7 +2264,7 @@  static unsigned int count_highmem_image_pages(struct memory_bitmap *bm)
 	memory_bm_position_reset(bm);
 	pfn = memory_bm_next_pfn(bm);
 	while (pfn != BM_END_OF_MAP) {
-		if (PageHighMem(pfn_to_page(pfn)))
+		if (PageHighMemZone(pfn_to_page(pfn)))
 			cnt++;
 
 		pfn = memory_bm_next_pfn(bm);
@@ -2541,7 +2541,7 @@  static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
 		return ERR_PTR(-EFAULT);
 
 	page = pfn_to_page(pfn);
-	if (PageHighMem(page))
+	if (PageHighMemZone(page))
 		return get_highmem_page_buffer(page, ca);
 
 	if (swsusp_page_is_forbidden(page) && swsusp_page_is_free(page))