| Message ID | 20230131063206.28820-1-Kuan-Ying.Lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: Add folio to list when folio_isolate_lru() succeed | expand |
On Tue, 31 Jan 2023 14:32:06 +0800 Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> wrote: > If we call folio_isolate_lru() successfully, we will get > return value 0. We need to add this folio to the > movable_pages_list. > > Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()") > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> > > ... > > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1914,7 +1914,7 @@ static unsigned long collect_longterm_unpinnable_pages( > drain_allow = false; > } > > - if (!folio_isolate_lru(folio)) > + if (folio_isolate_lru(folio)) > continue; > > list_add_tail(&folio->lru, movable_page_list); Thanks. What are the user-visible effects of this bug?
On 1/31/2023 2:32 PM, Kuan-Ying Lee wrote: > If we call folio_isolate_lru() successfully, we will get > return value 0. We need to add this folio to the > movable_pages_list. > > Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()") > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> Good catch. Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index f3d2cccb89f8..918c364d01ac 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1914,7 +1914,7 @@ static unsigned long collect_longterm_unpinnable_pages( > drain_allow = false; > } > > - if (!folio_isolate_lru(folio)) > + if (folio_isolate_lru(folio)) > continue; > > list_add_tail(&folio->lru, movable_page_list);
On Tue, 2023-01-31 at 15:17 -0800, Andrew Morton wrote: > On Tue, 31 Jan 2023 14:32:06 +0800 Kuan-Ying Lee < > Kuan-Ying.Lee@mediatek.com> wrote: > > > If we call folio_isolate_lru() successfully, we will get > > return value 0. We need to add this folio to the > > movable_pages_list. > > > > Fixes: 67e139b02d99 ("mm/gup.c: refactor > > check_and_migrate_movable_pages()") > > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> > > > > ... > > > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -1914,7 +1914,7 @@ static unsigned long > > collect_longterm_unpinnable_pages( > > drain_allow = false; > > } > > > > - if (!folio_isolate_lru(folio)) > > + if (folio_isolate_lru(folio)) > > continue; > > > > list_add_tail(&folio->lru, movable_page_list); > > Thanks. What are the user-visible effects of this bug? Hi Andrew, I didn't hit bug on real devices. I observe this issue by tracing get_user_pages() call flow. Thanks.
Andrew Morton <akpm@linux-foundation.org> writes: > On Tue, 31 Jan 2023 14:32:06 +0800 Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> wrote: > >> If we call folio_isolate_lru() successfully, we will get >> return value 0. We need to add this folio to the >> movable_pages_list. Ugh, thanks for catching this: Reviewed-by: Alistair Popple <apopple@nvidia.com> >> Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()") >> Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> >> >> ... >> >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -1914,7 +1914,7 @@ static unsigned long collect_longterm_unpinnable_pages( >> drain_allow = false; >> } >> >> - if (!folio_isolate_lru(folio)) >> + if (folio_isolate_lru(folio)) >> continue; >> >> list_add_tail(&folio->lru, movable_page_list); > > Thanks. What are the user-visible effects of this bug? In the common case none other than an extra loop through collect_longterm_unpinnable_pages(): 1. First call to collect_longterm_unpinnable_pages() will increment collected and isolate the page but not add it to movable_page_list. 2. migrate_longterm_unpinnable_pages() will return -EAGAIN and unpin all the pages but they will remain LRU isolated. 3. The next spin around __gup_longterm_locked() will re-pin the pages and re-call collect_longterm_unpinnable_pages(). As the page has already been isolated folio_isolate_lru() will return -EBUSY which will add the page to movable_page_list and complete processing as intended. However this assumes the page table still points to the same page when __get_user_pages_locked() is called the second time. That may not be the case in which case we would leave the page LRU isolated forever effectively leaving an unmovable page in a movable zone which is what we were trying to avoid in the first place. So I think Cc: stable is warranted. - Alistair
On 31.01.23 07:32, Kuan-Ying Lee wrote: > If we call folio_isolate_lru() successfully, we will get > return value 0. We need to add this folio to the > movable_pages_list. > > Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()") > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index f3d2cccb89f8..918c364d01ac 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1914,7 +1914,7 @@ static unsigned long collect_longterm_unpinnable_pages( > drain_allow = false; > } > > - if (!folio_isolate_lru(folio)) > + if (folio_isolate_lru(folio)) > continue; > > list_add_tail(&folio->lru, movable_page_list); Agreed that this deserves cc:stable Acked-by: David Hildenbrand <david@redhat.com>
diff --git a/mm/gup.c b/mm/gup.c index f3d2cccb89f8..918c364d01ac 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1914,7 +1914,7 @@ static unsigned long collect_longterm_unpinnable_pages( drain_allow = false; } - if (!folio_isolate_lru(folio)) + if (folio_isolate_lru(folio)) continue; list_add_tail(&folio->lru, movable_page_list);
If we call folio_isolate_lru() successfully, we will get return value 0. We need to add this folio to the movable_pages_list. Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()") Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> --- mm/gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)