Re: [PATCH v2 05/10] mm/gup: separate PageHighMem() and PageHighMemZone() use case

[Joonsoo Kim]

2020ë 5ì 1ì (ê) ìí 9:24, Christoph Hellwig <hch@xxxxxxxxxxxxx>ëì ìì:
>
> On Wed, Apr 29, 2020 at 12:26:38PM +0900, js1304@xxxxxxxxx wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> >
> > 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.
> >
> > Acked-by: Roman Gushchin <guro@xxxxxx>
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> > ---
> >  mm/gup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 11fda53..9652eed 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1608,7 +1608,7 @@ static struct page *new_non_cma_page(struct page *page, unsigned long private)
> >        */
> >       gfp_t gfp_mask = GFP_USER | __GFP_NOWARN;
> >
> > -     if (PageHighMem(page))
> > +     if (PageHighMemZone(page))
> >               gfp_mask |= __GFP_HIGHMEM;
>
> I think this wants to stay PageHighMem.  This migrates CMA pages to
> other places before doing a long term pin.  Anything that didn't have
> a direct mapping before won't need one for the new page, which could
> also include non-highmem zones without a highmem mapping.

What we want to do here is to guess allocation gfp flags of original
page in order to allocate
a new page with most relaxed gfp flag. It is depend on the zone where
the page belong to
rather than existence of direct mapping. Until now, existence of
direct mapping implies
the type of zone so there is no problem.

After my future CMA patchset, direct mapped CMA page will be on the
ZONE_MOVABLE.
And, a page on ZONE_MOVABLE should be allocated with __GFP_HIGHMEM |
__GFP_MOVABLE.
So, most relaxed gfp flag for this CMA page would include
__GFP_HIGHMEM. If PageHighMem()
is used here, __GFP_HIGHMEM would be lost since this CMA page has a
direct mapping.

Therefore, PageHighMemZone() is right one here.

Anyway, I saw Eric's comment in other e-mail that abstraction is
needed to guess gfp flags of
original page and I agree with it. This site can also get benefit of
such a change.

Thanks.