Re: reply: [RFC] pin_user_pages_fast failure count increased

[Hyesoo Yu <hyesoo.yu@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4215 bytes --]

On Mon, May 26, 2025 at 04:05:16PM +0800, Zhaoyang Huang wrote:
> On Mon, May 26, 2025 at 3:50 PM Hyesoo Yu <hyesoo.yu@samsung.com> wrote:
> >
> > On Thu, May 22, 2025 at 07:52:41PM -0700, John Hubbard wrote:
> > > On 5/22/25 7:37 PM, 김재원 wrote:
> > > ...
> > > > I think this is what you meant, please let me know if you have an idea to make this nicer.
> > > > We may be to able to prepare the patch next week.
> > > >
> > > >  static long
> > > >  check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
> > > >  {
> > > > +       bool any_unpinnable;
> > > >         LIST_HEAD(movable_folio_list);
> > > >
> > > > -       collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
> > > > -       if (list_empty(&movable_folio_list))
> > > > -               return 0;
> > > > +       any_unpinnable = collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
> > > > +       if (list_empty(&movable_folio_list)) {
> > > > +               if (any_unpinnable)
> > > > +                       pofs_unpin(pofs);
> > >
> > > I think this is correct, although as I mentioned in the other thread,
> > > that implies that commit 1aaf8c122918 (which didn't add nor remove
> > > any pof unpinning) is probably not the true or only culprit, right?
> > >
> > > > +               return any_unpinnable ? -EAGAIN : 0;
> > >
> > > Ha, the "?" operator almost always does more harm than good.
> > >
> > > Here, for example, it has obscured from you the fact that any_unpinnable
> > > is being checked twice, when you could have merged those into a single "if".
> > >
> >
> > Hello,
> >
> > I was wondering if the original problem - an infinite loop when pages allocated by
> > cma_alloc() in vm_ops->fault are passed to GUP - still remains unresolved.
> > (To be honest, I'm not quite sure how such pages end up being pinned via GUP.
> >  Is that the expected behavior, or could it possibly indicate a bug ?)
> The original problem arises from applying CMA as guestOS's memory
> slots for kvm which use GUP to setup its 2nd stage mapping(HVA->PFN).
> You can check KVM code if you are interested.
> 

Thanks for the kind explanation. While I'm not deeply familiar with KVM, my understanding
is that there are cases where GUP is used on CMA.

So does that mean pinning memory from the CMA was actually intended to succeed ? 

In other words, with the commit 1aaf8c122918, it seems that pinning the CMA and no LRU pages
no longer results in an infinite loop and instead, the pinning is now allowed to proceed
successfully. Was this the intended behavior ?

I believe the pinning of CMA pages is something that should be properly handled,
and I understand the proposed code above is also intended to address this issue.

However if -EAGAIN is returned in cases where unpinnable pages are present,
it seems the infinite loop problem could reappear.

In general, we try to avoid pinning memory from CMA, so the fact that certain CMA regions
can still be pinned seems contradictory to that principle.
(folio_is_longterm_pinnable() function returns false for CMA.)

Is there a way to distinguish between CMA regions that must not be pinned and those that
might be allowed to be pinned like KVM ?

> > Would it be make sense to try calling __lru_add_drain_all(false) in collect_longterm_unpinnable_folios ?
> Please be noted that not all the pages allocated from vm_ops->fault
> will be added to LRU.

Yes, however, in our case, where the issue occurred due to pinning a cma page, I believe
there was likely contention around lru_add_drain_all(). 

Thanks,
Regards.

> > We could consider limiting the number of -EAGAIN retries to a fixed count (e.g., 5 attempts) to
> > avoid an infinite loop. Or perhaps in check_and_migrate_movable_pages_or_folio(),
> > if the list is empty but any_unpinnable is true, we should consider returning an error
> > instead of EAGAIN to explicitly prevent longterm pinning of CMA allocated memory.
> >
> > I'd be interested to hear what others think.
> >
> > Thanks,
> > Hyesoo Yu.
> >
> >
> > > > +       }
> > > >
> > > >         return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);
> > > >  }
> > > thanks,
> > > --
> > > John Hubbard
> > >
> > >
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]