Re: [PATCH v2 03/10] kexec: separate PageHighMem() and PageHighMemZone() use case

[ebiederm@xmission.com (Eric W. Biederman)]

js1304@gmail.com writes:

> 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 #2 for this patch.

Hmm.

What happened to the notion of deprecating and reducing the usage of
highmem?  I know that we have some embedded architectures where it is
still important but this feels like it flies in the face of that.


This part of kexec would be much more maintainable if it had a proper
mm layer helper that tested to see if the page matched the passed in
gfp flags.  That way the mm layer could keep changing and doing weird
gyrations and this code would not care.


What would be really helpful is if there was a straight forward way to
allocate memory whose physical address fits in the native word size.


All I know for certain about this patch is that it takes a piece of code
that looked like it made sense, and transfroms it into something I can
not easily verify, and can not maintain.

As it makes the code unmaintainable.
Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>


Not to say that the code isn't questionable as it is, but this change just
pushes it over the edge into gobbledy gook.

Eric


> Acked-by: Roman Gushchin <guro@fb.com>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  kernel/kexec_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index ba1d91e..33097b7 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -766,7 +766,7 @@ static struct page *kimage_alloc_page(struct kimage *image,
>  			 * gfp_flags honor the ones passed in.
>  			 */
>  			if (!(gfp_mask & __GFP_HIGHMEM) &&
> -			    PageHighMem(old_page)) {
> +			    PageHighMemZone(old_page)) {
>  				kimage_free_pages(old_page);
>  				continue;
>  			}