[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <56fe9ae8-e9bf-4627-a10a-ba1d3e84e8c8@redhat.com>
Date: Tue, 21 Jan 2025 09:13:26 +0100
From: David Hildenbrand <david@...hat.com>
To: "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Alistair Popple <apopple@...dia.com>, John Hubbard <jhubbard@...dia.com>,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Zhaoyang Huang <huangzhaoyang@...il.com>, steve.kang@...soc.com,
aijun.sun@...soc.com
Subject: Re: [PATCH v2] mm: gup: fix infinite loop within
__get_longterm_locked
On 21.01.25 03:01, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@...soc.com>
>
> 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.
Last sentence now has to be "Fix it by simply taking a look at the list
in the single caller, to see if anything was added.".
>
> Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()")
>
> Reviewed-by: John Hubbard <jhubbard@...dia.com>
> Suggested-by: David Hildenbrand <david@...hat.com>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@...soc.com>
> ---
> v2: judge if there are unpinnable pages via list_empty and update commit
> message
> ---
> ---
> mm/gup.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 3b75e631f369..37951fba929a 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2323,11 +2323,11 @@ static void pofs_unpin(struct pages_or_folios *pofs)
> /*
> * Returns the number of collected folios. Return value is always >= 0.
> */
> -static unsigned long collect_longterm_unpinnable_folios(
> +static void collect_longterm_unpinnable_folios(
> struct list_head *movable_folio_list,
> struct pages_or_folios *pofs)
> {
> - unsigned long i, collected = 0;
> + unsigned long i;
Nit: "reverse christmas tree" parameter format wants this to come after
the "drain_allow" variable now.
> struct folio *prev_folio = NULL;
> bool drain_allow = true;
>
> @@ -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;
>
> @@ -2364,8 +2362,6 @@ static unsigned long collect_longterm_unpinnable_folios(
> NR_ISOLATED_ANON + folio_is_file_lru(folio),
> folio_nr_pages(folio));
> }
> -
> - return collected;
> }
>
> /*
> @@ -2442,11 +2438,9 @@ static long
> check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
> {
> LIST_HEAD(movable_folio_list);
> - unsigned long collected;
>
> - collected = collect_longterm_unpinnable_folios(&movable_folio_list,
> - pofs);
> - if (!collected)
> + collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
> + if (list_empty(&movable_folio_list))
> return 0;
>
> return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);
Nice, exactly what I just recommended as reply to #1 after waking up :)
Reviewed-by: David Hildenbrand <david@...hat.com>
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists