Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Tue, May 03, 2022 at 07:27:06PM +0200, David Hildenbrand wrote:
> >> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> >> be migrated, which can fail if the page is temporarily unmovable.
> > 
> > Why is the page temporarily unmovable? The GUP didn't increase the
> > refcount in the case. If it's not migrabtable, that's not a fault
> > from the GUP but someone is already holding the temporal refcount.
> > It's not the scope this patchset would try to solve it.
> 
> You can have other references on the page that turn it temporarily
> unmovable, for example, via FOLL_GET, short-term FOLL_PIN.

Sure. However, user didn't passed the FOLL_LONGTERM. In that case,
the temporal page migration failure was expected.
What we want to guarantee for successful page migration is only
FOLL_LONGTERM.

If you are talking about the general problem(any GUP API without
FOLL_LONGTERM flag which is supposed to be short-term could turn
into long-term pinning by several reasons - I had struggled with
those issues - FOLL_LONGTERM is misnormer to me), yeah, I agree
we need to fix it but it's orthgonal issue.

> 
> > 
> >>
> >> See my point? We will try migrating in cases where we don't have to
> > 
> > Still not clear for me what you are concerning.
> > 
> >> migrate. I think what we would want to do is always reject pinning a CMA
> >> page, independent of the isolation status. but we don't have that
> > 
> > Always reject pinning a CMA page if it is *FOLL_LONGTERM*
> 
> Yes.
> 
> > 
> >> information available.
> > 
> > page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
> > for it?
> > 
> >>
> >> I raised in the past that we should look into preserving the migration
> >> type and turning MIGRATE_ISOLATE essentially into an additional flag.
> >>
> >>
> >> So I guess this patch is the right thing to do for now, but I wanted to
> >> spell out the implications.
> > 
> > I want but still don't understand what you want to write further
> > about the implication parts. If you make more clear, I am happy to
> > include it.
> 
> What I am essentially saying is that when rejecting to long-term
> FOLL_PIN something that is MIGRATE_ISOLATE now, we might now end up
> having to migrate pages that are actually fine to get pinned, because
> they are not actual CMA pages. And any such migration might fail when
> pages are temporarily unmovable.

Now I understand concern. Then how about introducing cma areas list
and use it instead of migrate type in is_pinnable_page

struct cma  {
  ..
  ..
  list_head list
};

bool is_cma_page(unsigned long pfn) {
    for cma in cma_list
      if (pfn >= cma->base_pfn && pfn < cma->base_pfn + count
        return true;

    return false;
}

Do you want to fix it at this moment or just write down the
possibility in the description and then we could fix once it
really happens later?

> 
> 
> > 
> >>
> >>>
> >>> A thing to get some attention is whether we need READ_ONCE or not
> >>> for the local variable mt.
> >>>
> >>
> >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> >> there is anything stopping the compiler from re-reading the value. But
> >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> >> something in between.
> > 
> > How about this?
> > 
> >      CPU A                                                      CPU B
> > 
> > is_pinnable_page
> >   ..
> >   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
> >   mt == MIGRATE_CMA
> >     get_pageblock_miratetype(page)
> >         returns MIGRATE_ISOLATE
> >   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
> >     get_pageblock_miratetype(page)
> >         returns MIGRATE_CMA
> >  
> > So both conditions fails to detect it.
> 
> I think you're right. That's nasty.

Ccing Paul to borrow expertise. :)

int mt = get_pageblock_migratetype(page);

if (mt == MIGRATE_CMA)
  return true;

if (mt == MIGRATE_ISOLATE)
  return true;

I'd like to keep use the local variable mt's value in folloing
conditions checks instead of refetching the value from
get_pageblock_migratetype.

What's the right way to achieve it?

Thanks in advance!