Message-ID: <87ftcfpzjn.fsf@x220.int.ebiederm.org>
Date:   Mon, 04 May 2020 09:03:56 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Joonsoo Kim <js1304@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Laura Abbott <labbott@...hat.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@...ux.ibm.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Michal Hocko <mhocko@...e.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Roman Gushchin <guro@...com>, Minchan Kim <minchan@...nel.org>,
        Rik van Riel <riel@...riel.com>,
        Christian Koenig <christian.koenig@....com>,
        Huang Rui <ray.huang@....com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, kernel-team@....com,
        Christoph Hellwig <hch@...radead.org>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Kexec Mailing List <kexec@...ts.infradead.org>
Subject: Re: [PATCH v2 03/10] kexec: separate PageHighMem() and PageHighMemZone() use case


I have added in the kexec mailling list.

Looking at the patch we are discussing it appears that the kexec code
could be doing much better in highmem situations today but is not.


Joonsoo Kim <js1304@...il.com> writes:

> 2020년 5월 1일 (금) 오후 11:06, Eric W. Biederman <ebiederm@...ssion.com>님이 작성:
>>
>> js1304@...il.com writes:
>>
>> > From: Joonsoo Kim <iamjoonsoo.kim@....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.
>
> AFAIK, deprecating highmem requires some more time and, before then,
> we need to support it.

But it at least makes sense to look at what we are doing with highmem
and ask if it makes sense.

>> 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.
>
> Good idea! I will do it.
>
>>
>> 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.
>
> Although I decide to make a helper as you described above, I don't
> understand why you think that a new code isn't maintainable. It is just
> the same thing with different name. Could you elaborate more why do
> you think so?

Because the current code is already wrong.  It does not handle
the general case of what it claims to handle.  When the only distinction
that needs to be drawn is highmem or not highmem that is likely fine.
But now you are making it possible to draw more distinctions.  At which
point I have no idea which distinction needs to be drawn.


The code and the logic is about 20 years old.  When it was written I
don't recally taking numa seriously and the kernel only had 3 zones
as I recall (DMA aka the now deprecated GFP_DMA, NORMAL, and HIGH).

The code attempts to work around limitations of those old zones amd play
nice in a highmem world by allocating memory HIGH memory and not using
it if the memory was above 4G ( on 32bit ).

Looking the kernel now has GFP_DMA32 so on 32bit with highmem we should
probably be using that, when allocating memory.




Further in dealing with this memory management situation we only
have two situations we call kimage_alloc_page.

For an indirect page which must have a valid page_address(page).
We could probably relax that if we cared to.

For a general kexec page to store the next kernel in until we switch.
The general pages can be in high memory.

In a highmem world all of those pages should be below 32bit.



Given that we fundamentally have two situations my sense is that we
should just refactor the code so that we never have to deal with:


			/* The old page I have found cannot be a
			 * destination page, so return it if it's
			 * gfp_flags honor the ones passed in.
			 */
			if (!(gfp_mask & __GFP_HIGHMEM) &&
			    PageHighMem(old_page)) {
				kimage_free_pages(old_page);
				continue;
			}

Either we teach kimage_add_entry how to work with high memory pages
(still 32bit accessible) or we teach kimage_alloc_page to notice it is
an indirect page allocation and to always skip trying to reuse the page
it found in that case.

That way the code does not need to know about forever changing mm internals.



We should probably investigate GFP_DMA32 at the same time, and switch to
that for 32bit rather than continuing to use GFP_HIGHUSER.

Eric

Powered by blists - more mailing lists