2020-04-29 03:30:40

by Joonsoo Kim

[permalink] [raw]
Subject: [PATCH v2 00/10] change the implementation of the PageHighMem()

From: Joonsoo Kim <[email protected]>

Changes on v2
- add "acked-by", "reviewed-by" tags
- replace PageHighMem() with use open-code, instead of using
new PageHighMemZone() macro. Related file is "include/linux/migrate.h"

Hello,

This patchset separates two use cases of PageHighMem() by introducing
PageHighMemZone() macro. And, it changes the implementation of
PageHighMem() to reflect the actual meaning of this macro. This patchset
is a preparation step for the patchset,
"mm/cma: manage the memory of the CMA area by using the ZONE_MOVABLE" [1].

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.

Until now, both the cases are the perfectly same thing. So, implementation
of the PageHighMem() uses the one case that checks if the zone of the page
is the highmem type zone or not.

"#define PageHighMem(__p) is_highmem_idx(page_zonenum(__p))"

ZONE_MOVABLE is special. It is considered as normal type zone on
!CONFIG_HIGHMEM, but, it is considered as highmem type zone
on CONFIG_HIGHMEM. Let's focus on later case. In later case, all pages
on the ZONE_MOVABLE has no direct mapping until now.

However, following patchset
"mm/cma: manage the memory of the CMA area by using the ZONE_MOVABLE"
, which is once merged and reverted, will be tried again and will break
this assumption that all pages on the ZONE_MOVABLE has no direct mapping.
Hence, the ZONE_MOVABLE which is considered as highmem type zone could
have the both types of pages, direct mapped and not. Since
the ZONE_MOVABLE could have both type of pages, __GFP_HIGHMEM is still
required to allocate the memory from it. And, we conservatively need to
consider the ZONE_MOVABLE as highmem type zone.

Even in this situation, PageHighMem() for the pages on the ZONE_MOVABLE
when it is called for checking the direct mapping should return correct
result. Current implementation of PageHighMem() just returns TRUE
if the zone of the page is on a highmem type zone. So, it could be wrong
if the page on the MOVABLE_ZONE is actually direct mapped.

To solve this potential problem, this patch introduces a new
PageHighMemZone() macro. In following patches, two use cases of
PageHighMem() are separated by calling proper macro, PageHighMem() and
PageHighMemZone(). Then, implementation of PageHighMem() will be changed
as just checking if the direct mapping exists or not, regardless of
the zone of the page.

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.

My final plan is to change the name, PageHighMem() to PageNoDirectMapped()
or something else in order to represent proper meaning.

This patchset is based on next-20200428 and you can find the full patchset on the
following link.

https://github.com/JoonsooKim/linux/tree/page_highmem-cleanup-v2.00-next-20200428

Thanks.

[1]: https://lore.kernel.org/linux-mm/[email protected]

Joonsoo Kim (10):
mm/page-flags: introduce PageHighMemZone()
drm/ttm: separate PageHighMem() and PageHighMemZone() use case
kexec: separate PageHighMem() and PageHighMemZone() use case
power: separate PageHighMem() and PageHighMemZone() use case
mm/gup: separate PageHighMem() and PageHighMemZone() use case
mm/hugetlb: separate PageHighMem() and PageHighMemZone() use case
mm: separate PageHighMem() and PageHighMemZone() use case
mm/page_alloc: correct the use of is_highmem_idx()
mm/migrate: replace PageHighMem() with open-code
mm/page-flags: change the implementation of the PageHighMem()

drivers/gpu/drm/ttm/ttm_memory.c | 4 ++--
drivers/gpu/drm/ttm/ttm_page_alloc.c | 2 +-
drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 2 +-
drivers/gpu/drm/ttm/ttm_tt.c | 2 +-
include/linux/migrate.h | 4 +++-
include/linux/page-flags.h | 10 +++++++++-
kernel/kexec_core.c | 2 +-
kernel/power/snapshot.c | 12 ++++++------
mm/gup.c | 2 +-
mm/hugetlb.c | 2 +-
mm/memory_hotplug.c | 2 +-
mm/page_alloc.c | 4 ++--
12 files changed, 29 insertions(+), 19 deletions(-)

--
2.7.4


2020-04-29 03:31:26

by Joonsoo Kim

[permalink] [raw]
Subject: [PATCH v2 10/10] mm/page-flags: change the implementation of the PageHighMem()

From: Joonsoo Kim <[email protected]>

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.

Previous patches introduce PageHighMemZone() macro and separates both
cases strictly. So, now, PageHighMem() is used just for checking if
there is a direct mapping for this page or not.

In the following patchset, ZONE_MOVABLE which could be considered as
the highmem type zone in some configuration could have both types of
pages, direct mapped pages and unmapped pages. So, current implementation
of PageHighMem() that checks the zone rather than checks the page in order
to check if a direct mapping exists will be invalid. This patch prepares
that case by implementing PageHighMem() with the max_low_pfn.

Acked-by: Roman Gushchin <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
---
include/linux/page-flags.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index fca0cce..7ac5fc8 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -375,6 +375,8 @@ PAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
TESTCLEARFLAG(Readahead, reclaim, PF_NO_COMPOUND)

#ifdef CONFIG_HIGHMEM
+extern unsigned long max_low_pfn;
+
/*
* Must use a macro here due to header dependency issues. page_zone() is not
* available at this point.
@@ -383,7 +385,7 @@ PAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
* in order to predict previous gfp_flags or to count something for system
* memory management.
*/
-#define PageHighMem(__p) is_highmem_idx(page_zonenum(__p))
+#define PageHighMem(__p) (page_to_pfn(__p) >= max_low_pfn)
#define PageHighMemZone(__p) is_highmem_idx(page_zonenum(__p))
#else
PAGEFLAG_FALSE(HighMem)
--
2.7.4

2020-04-29 03:31:36

by Joonsoo Kim

[permalink] [raw]
Subject: [PATCH v2 06/10] mm/hugetlb: separate PageHighMem() and PageHighMemZone() use case

From: Joonsoo Kim <[email protected]>

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 <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
---
mm/hugetlb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5548e88..56c9143 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2639,7 +2639,7 @@ static void try_to_free_low(struct hstate *h, unsigned long count,
list_for_each_entry_safe(page, next, freel, lru) {
if (count >= h->nr_huge_pages)
return;
- if (PageHighMem(page))
+ if (PageHighMemZone(page))
continue;
list_del(&page->lru);
update_and_free_page(h, page);
--
2.7.4

2020-04-29 03:32:45

by Joonsoo Kim

[permalink] [raw]
Subject: [PATCH v2 09/10] mm/migrate: replace PageHighMem() with open-code

From: Joonsoo Kim <[email protected]>

Implementation of PageHighMem() will be changed in following patches.
Before that, use open-code to avoid the side effect of implementation
change on PageHighMem().

Acked-by: Roman Gushchin <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
---
include/linux/migrate.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 3e546cb..a9cfd8e 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -37,6 +37,7 @@ static inline struct page *new_page_nodemask(struct page *page,
gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_RETRY_MAYFAIL;
unsigned int order = 0;
struct page *new_page = NULL;
+ int zidx;

if (PageHuge(page))
return alloc_huge_page_nodemask(page_hstate(compound_head(page)),
@@ -47,7 +48,8 @@ static inline struct page *new_page_nodemask(struct page *page,
order = HPAGE_PMD_ORDER;
}

- if (PageHighMem(page) || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
+ zidx = zone_idx(page_zone(page));
+ if (is_highmem_idx(zidx) || zidx == ZONE_MOVABLE)
gfp_mask |= __GFP_HIGHMEM;

new_page = __alloc_pages_nodemask(gfp_mask, order,
--
2.7.4

2020-04-30 01:49:56

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] change the implementation of the PageHighMem()

On Wed, 29 Apr 2020 12:26:33 +0900 [email protected] wrote:

> From: Joonsoo Kim <[email protected]>
>
> Changes on v2
> - add "acked-by", "reviewed-by" tags
> - replace PageHighMem() with use open-code, instead of using
> new PageHighMemZone() macro. Related file is "include/linux/migrate.h"
>
> Hello,
>
> This patchset separates two use cases of PageHighMem() by introducing
> PageHighMemZone() macro. And, it changes the implementation of
> PageHighMem() to reflect the actual meaning of this macro. This patchset
> is a preparation step for the patchset,
> "mm/cma: manage the memory of the CMA area by using the ZONE_MOVABLE" [1].
>
> 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.
>
> Until now, both the cases are the perfectly same thing. So, implementation
> of the PageHighMem() uses the one case that checks if the zone of the page
> is the highmem type zone or not.
>
> "#define PageHighMem(__p) is_highmem_idx(page_zonenum(__p))"
>
> ZONE_MOVABLE is special. It is considered as normal type zone on
> !CONFIG_HIGHMEM, but, it is considered as highmem type zone
> on CONFIG_HIGHMEM. Let's focus on later case. In later case, all pages
> on the ZONE_MOVABLE has no direct mapping until now.
>
> However, following patchset
> "mm/cma: manage the memory of the CMA area by using the ZONE_MOVABLE"
> , which is once merged and reverted, will be tried again and will break
> this assumption that all pages on the ZONE_MOVABLE has no direct mapping.
> Hence, the ZONE_MOVABLE which is considered as highmem type zone could
> have the both types of pages, direct mapped and not. Since
> the ZONE_MOVABLE could have both type of pages, __GFP_HIGHMEM is still
> required to allocate the memory from it. And, we conservatively need to
> consider the ZONE_MOVABLE as highmem type zone.
>
> Even in this situation, PageHighMem() for the pages on the ZONE_MOVABLE
> when it is called for checking the direct mapping should return correct
> result. Current implementation of PageHighMem() just returns TRUE
> if the zone of the page is on a highmem type zone. So, it could be wrong
> if the page on the MOVABLE_ZONE is actually direct mapped.
>
> To solve this potential problem, this patch introduces a new
> PageHighMemZone() macro. In following patches, two use cases of
> PageHighMem() are separated by calling proper macro, PageHighMem() and
> PageHighMemZone(). Then, implementation of PageHighMem() will be changed
> as just checking if the direct mapping exists or not, regardless of
> the zone of the page.
>
> 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.

hm, this won't improve maintainability :(

- Everyone will need to remember when to use PageHighMem() and when
to use PageHighMemZone(). If they get it wrong, they're unlikely to
notice any problem in their runtime testing, correct?

- New code will pop up which gets it wrong and nobody will notice for
a long time.

So I guess we need to be pretty confident that the series "mm/cma:
manage the memory of the CMA area by using the ZONE_MOVABLE" will be
useful and merged before proceeding with this, yes?

On the other hand, this whole series is a no-op until [10/10]
(correct?) so it can be effectively reverted with a single line change,
with later cleanups which revert the other 9 patches.

So I think I'd like to take another look at "mm/cma: manage the memory
of the CMA area by using the ZONE_MOVABLE" before figuring out what to
do here. Mainly to answer the question "is the new feature valuable
enough to justify the maintainability impact". So please do take some
care in explaining the end-user benefit when preparing the new version
of that patchset.

2020-05-01 10:54:37

by Joonsoo Kim

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] change the implementation of the PageHighMem()

2020년 4월 30일 (목) 오전 10:47, Andrew Morton <[email protected]>님이 작성:
>
> On Wed, 29 Apr 2020 12:26:33 +0900 [email protected] wrote:
>
> > From: Joonsoo Kim <[email protected]>
> >
> > Changes on v2
> > - add "acked-by", "reviewed-by" tags
> > - replace PageHighMem() with use open-code, instead of using
> > new PageHighMemZone() macro. Related file is "include/linux/migrate.h"
> >
> > Hello,
> >
> > This patchset separates two use cases of PageHighMem() by introducing
> > PageHighMemZone() macro. And, it changes the implementation of
> > PageHighMem() to reflect the actual meaning of this macro. This patchset
> > is a preparation step for the patchset,
> > "mm/cma: manage the memory of the CMA area by using the ZONE_MOVABLE" [1].
> >
> > 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.
> >
> > Until now, both the cases are the perfectly same thing. So, implementation
> > of the PageHighMem() uses the one case that checks if the zone of the page
> > is the highmem type zone or not.
> >
> > "#define PageHighMem(__p) is_highmem_idx(page_zonenum(__p))"
> >
> > ZONE_MOVABLE is special. It is considered as normal type zone on
> > !CONFIG_HIGHMEM, but, it is considered as highmem type zone
> > on CONFIG_HIGHMEM. Let's focus on later case. In later case, all pages
> > on the ZONE_MOVABLE has no direct mapping until now.
> >
> > However, following patchset
> > "mm/cma: manage the memory of the CMA area by using the ZONE_MOVABLE"
> > , which is once merged and reverted, will be tried again and will break
> > this assumption that all pages on the ZONE_MOVABLE has no direct mapping.
> > Hence, the ZONE_MOVABLE which is considered as highmem type zone could
> > have the both types of pages, direct mapped and not. Since
> > the ZONE_MOVABLE could have both type of pages, __GFP_HIGHMEM is still
> > required to allocate the memory from it. And, we conservatively need to
> > consider the ZONE_MOVABLE as highmem type zone.
> >
> > Even in this situation, PageHighMem() for the pages on the ZONE_MOVABLE
> > when it is called for checking the direct mapping should return correct
> > result. Current implementation of PageHighMem() just returns TRUE
> > if the zone of the page is on a highmem type zone. So, it could be wrong
> > if the page on the MOVABLE_ZONE is actually direct mapped.
> >
> > To solve this potential problem, this patch introduces a new
> > PageHighMemZone() macro. In following patches, two use cases of
> > PageHighMem() are separated by calling proper macro, PageHighMem() and
> > PageHighMemZone(). Then, implementation of PageHighMem() will be changed
> > as just checking if the direct mapping exists or not, regardless of
> > the zone of the page.
> >
> > 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.
>
> hm, this won't improve maintainability :(
>
> - Everyone will need to remember when to use PageHighMem() and when
> to use PageHighMemZone(). If they get it wrong, they're unlikely to
> notice any problem in their runtime testing, correct?
>
> - New code will pop up which gets it wrong and nobody will notice for
> a long time.

Hmm... I think that it's not that hard to decide correct macro. If we rename
PageHighMem() with PageDirectMapped(), they, PageDirectMapped() and
PageHighMemZone(), are self-explanation macro. There would be no
confusion to use.

> So I guess we need to be pretty confident that the series "mm/cma:
> manage the memory of the CMA area by using the ZONE_MOVABLE" will be
> useful and merged before proceeding with this, yes?

Yes and my assumption is that we (MM) have agreed with usefulness of
CMA series.

> On the other hand, this whole series is a no-op until [10/10]
> (correct?) so it can be effectively reverted with a single line change,

Correct!

> with later cleanups which revert the other 9 patches.
>
> So I think I'd like to take another look at "mm/cma: manage the memory
> of the CMA area by using the ZONE_MOVABLE" before figuring out what to
> do here. Mainly to answer the question "is the new feature valuable
> enough to justify the maintainability impact". So please do take some
> care in explaining the end-user benefit when preparing the new version
> of that patchset.

So, do you mean to send the new version of CMA patchset with more
explanation before merging this patchset? If yes, I can do. But, I'm not sure
that it's worth doing. Problems of CMA are still not solved although
the utilization problem will be partially solved by Roman's "mm,page_alloc,cma:
conditionally prefer cma pageblocks for movable allocations" patch
in this (v5.7) release. Rationale that we agree with CMA patchset is still
remained.

Anyway, if you mean that, I will send the CMA patchset with more explanation.

Thanks.

2020-05-01 10:58:21

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] change the implementation of the PageHighMem()

On Fri, May 01, 2020 at 07:52:35PM +0900, Joonsoo Kim wrote:
> > - New code will pop up which gets it wrong and nobody will notice for
> > a long time.
>
> Hmm... I think that it's not that hard to decide correct macro. If we rename
> PageHighMem() with PageDirectMapped(), they, PageDirectMapped() and
> PageHighMemZone(), are self-explanation macro. There would be no
> confusion to use.

What confuses me is why we even need PageHighMemZone - mostly code
should not care about particular zones. Maybe just open coding
PageHighMemZone makes more sense - it is a little more cumersome, but
at least it makes it explicit what we check for. I already sent you
an incremental diff for one obvious place, but maybe we need to look
through the remaining ones if we can kill them or open code them in an
obvious way.

2020-05-01 12:17:34

by Joonsoo Kim

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] change the implementation of the PageHighMem()

2020년 5월 1일 (금) 오후 7:55, Christoph Hellwig <[email protected]>님이 작성:
>
> On Fri, May 01, 2020 at 07:52:35PM +0900, Joonsoo Kim wrote:
> > > - New code will pop up which gets it wrong and nobody will notice for
> > > a long time.
> >
> > Hmm... I think that it's not that hard to decide correct macro. If we rename
> > PageHighMem() with PageDirectMapped(), they, PageDirectMapped() and
> > PageHighMemZone(), are self-explanation macro. There would be no
> > confusion to use.
>
> What confuses me is why we even need PageHighMemZone - mostly code
> should not care about particular zones. Maybe just open coding
> PageHighMemZone makes more sense - it is a little more cumersome, but
> at least it makes it explicit what we check for. I already sent you
> an incremental diff for one obvious place, but maybe we need to look
> through the remaining ones if we can kill them or open code them in an
> obvious way.

I think that PageHighMemZone() is long and complicated enough to have
a macro.

PageHighMemZone(page) = is_highmem_idx(zone_idx(page_zone(page))

Instead of open-code, how about changing the style of macro like as
page_from_highmem()? What PageHighMemZone() represent is derivated
attribute from the page so PageXXX() style may not be appropriate.

Thanks.

2020-05-01 12:29:19

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 06/10] mm/hugetlb: separate PageHighMem() and PageHighMemZone() use case

On Wed, Apr 29, 2020 at 12:26:39PM +0900, [email protected] wrote:
> From: Joonsoo Kim <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Joonsoo Kim <[email protected]>

Why do we care about the zone here? This only cares about having
kernel direct mapped pages as far as I can tell.

2020-05-01 12:37:50

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] change the implementation of the PageHighMem()

On Fri, May 01, 2020 at 09:15:30PM +0900, Joonsoo Kim wrote:
> I think that PageHighMemZone() is long and complicated enough to have
> a macro.

It is. But then again it also shouldn't really be used by anything
but MM internals.

>
> PageHighMemZone(page) = is_highmem_idx(zone_idx(page_zone(page))
>
> Instead of open-code, how about changing the style of macro like as
> page_from_highmem()? What PageHighMemZone() represent is derivated
> attribute from the page so PageXXX() style may not be appropriate.

Maybe page_is_highmem_zone() with a big kerneldoc comment explaining
the use case? Bonus points of killing enough users that it can be
in mm/internal.h.

2020-05-04 03:12:00

by Joonsoo Kim

[permalink] [raw]
Subject: Re: [PATCH v2 06/10] mm/hugetlb: separate PageHighMem() and PageHighMemZone() use case

2020년 5월 1일 (금) 오후 9:26, Christoph Hellwig <[email protected]>님이 작성:
>
> On Wed, Apr 29, 2020 at 12:26:39PM +0900, [email protected] wrote:
> > From: Joonsoo Kim <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Joonsoo Kim <[email protected]>
>
> Why do we care about the zone here? This only cares about having
> kernel direct mapped pages as far as I can tell.

My understaning is that what we want to do here is to first free memory
that can be used for kernel allocation. If direct mapped page is on the zone
that cannot be used for kernel allocation, there is no meaning to free
this page first. So, we need to take a care of the zone here.

Thanks.

2020-05-04 03:35:29

by Joonsoo Kim

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] change the implementation of the PageHighMem()

2020년 5월 1일 (금) 오후 9:34, Christoph Hellwig <[email protected]>님이 작성:
>
> On Fri, May 01, 2020 at 09:15:30PM +0900, Joonsoo Kim wrote:
> > I think that PageHighMemZone() is long and complicated enough to have
> > a macro.
>
> It is. But then again it also shouldn't really be used by anything
> but MM internals.

I'm not sure that we can make it MM internal but I will try.

> >
> > PageHighMemZone(page) = is_highmem_idx(zone_idx(page_zone(page))
> >
> > Instead of open-code, how about changing the style of macro like as
> > page_from_highmem()? What PageHighMemZone() represent is derivated
> > attribute from the page so PageXXX() style may not be appropriate.
>
> Maybe page_is_highmem_zone() with a big kerneldoc comment explaining
> the use case? Bonus points of killing enough users that it can be
> in mm/internal.h.

I will try to kill page_is_highmem_zone() as much as possible in next version.

Thanks.