Re: [PATCH] mm: gup: fix infinite loop within __get_longterm_locked

[Zhaoyang Huang <huangzhaoyang@gmail.com>]

On Tue, Jan 21, 2025 at 4:14 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 20.01.25 10:26, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Infinite loop within __get_longterm_locked detected in an unique usage
> > of pin_user_pages where the VA's pages are all unpinnable(vm_ops->fault
> > function allocate pages via cma_alloc for hardware purpose and leave them
> > out of LRU) Fixing this by have 'collected' reflect the actual number> of pages in movable_folio_list.
>
> Maybe something like:
>
> "
> We can run into an infinite loop in __get_longterm_locked() when
> collect_longterm_unpinnable_folios() finds only folios that are isolated
> from the LRU or were never added to the LRU. This can happen when all
> folios to be pinned are never added to the LRU, for example when
> vm_ops->fault allocated pages using cma_alloc() and never added them to
> the LRU.
>
> We incorrectly update the "collected" variable even if nothing was
> collected. Fix it by incrementing "collected" only when we isolated a
> folio and added it to the list of folios to migrate.
> "
>
> I assume, long-term these things will not actually be folios, but pages,
> and we'll have to skip them in different code -- or assume they can be
> longterm pinned even on CMA because they are allocated by the CMA-owning
> driver.
Thanks for the commit message. will update them to v2
>
> >
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > ---
> >   mm/gup.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 3b75e631f369..2231ce7221f9 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2341,8 +2341,6 @@ static unsigned long collect_longterm_unpinnable_folios(
> >               if (folio_is_longterm_pinnable(folio))
> >                       continue;
> >
> > -             collected++;
> > -
> >               if (folio_is_device_coherent(folio))
> >                       continue;
> >
> > @@ -2359,6 +2357,8 @@ static unsigned long collect_longterm_unpinnable_folios(
> >               if (!folio_isolate_lru(folio))
> >                       continue;
> >
> > +             collected++;
> > +
> >               list_add_tail(&folio->lru, movable_folio_list);
> >               node_stat_mod_folio(folio,
> >                                   NR_ISOLATED_ANON + folio_is_file_lru(folio),
>
> What if folio_isolate_hugetlb() succeeded? The return value can tell us
> if it actually succeeded.
How about remove the variable 'collected' and change the criteria to
if(list_empty(&movable_folio_list))

>
> --
> Cheers,
>
> David / dhildenb
>