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!
