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

[Andrew Morton]

On Wed, 29 Apr 2020 12:26:33 +0900 js1304@xxxxxxxxx wrote:

> From: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> 
> 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.