Re: [PATCH 2/6] mm, hwpoison: use __PageMovable() to detect non-lru movable pages

[HORIGUCHI NAOYA(堀口 直也)]

On Mon, Sep 05, 2022 at 02:53:41PM +0800, Miaohe Lin wrote:
> On 2022/9/5 13:22, HORIGUCHI NAOYA(堀口 直也) wrote:
> > Hi Miaohe,
> > 
> > On Tue, Aug 30, 2022 at 08:36:00PM +0800, Miaohe Lin wrote:
> >> It's more recommended to use __PageMovable() to detect non-lru movable
> >> pages. We can avoid bumping page refcnt via isolate_movable_page() for
> >> the isolated lru pages. Also if pages become PageLRU just after they're
> >> checked but before trying to isolate them, isolate_lru_page() will be
> >> called to do the right work.
> > 
> > Good point, non-lru movable page is currently handled by isolate_lru_page(),
> > which always fails.  This means that we lost the chance of soft-offlining
> > for any non-lru movable page.  So this patch improves the situation.
> 
> Non-lru movable page will still be handled by isolate_movable_page() before the code change
> as they don't have PageLRU set. The current situation is that the isolated LRU pages are
> passed to isolate_movable_page() uncorrectly. This might not hurt. But the chance that pages
> become un-isolated and thus available just after checking could be seized with this patch.

OK, thank you for correct me.

> 
> > 
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> >> ---
> >>  mm/memory-failure.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index a923a6dde871..3966fa6abe03 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -2404,7 +2404,7 @@ EXPORT_SYMBOL(unpoison_memory);
> >>  static bool isolate_page(struct page *page, struct list_head *pagelist)
> >>  {
> >>  	bool isolated = false;
> >> -	bool lru = PageLRU(page);
> >> +	bool lru = !__PageMovable(page);
> > 
> > It seems that PAGE_MAPPING_MOVABLE is not set for hugetlb pages, so
> > lru becomes true for them.  Then, if isolate_hugetlb() succeeds,
> > inc_node_page_state() is called for hugetlb pages, maybe that's not expected.
> 
> Yes, that's unexpected. Thanks for pointing this out.
> 
> > 
> >>  
> >>  	if (PageHuge(page)) {
> >>  		isolated = !isolate_hugetlb(page, pagelist);
> >         } else {
> >                 if (lru)
> >                         isolated = !isolate_lru_page(page);
> >                 else
> >                         isolated = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
> > 
> >                 if (isolated)
> >                         list_add(&page->lru, pagelist);
> >         }
> > 
> >         if (isolated && lru)
> >                 inc_node_page_state(page, NR_ISOLATED_ANON +
> >                                     page_is_file_lru(page));
> > 
> > so, how about moving this if block into the above else block?
> > Then, the automatic variable lru can be moved into the else block.
> 
> Do you mean something like below?
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index df3bf266eebf..48780f3a61d3 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2404,24 +2404,25 @@ EXPORT_SYMBOL(unpoison_memory);
>  static bool isolate_page(struct page *page, struct list_head *pagelist)
>  {
>         bool isolated = false;
> -       bool lru = !__PageMovable(page);
> 
>         if (PageHuge(page)) {
>                 isolated = !isolate_hugetlb(page, pagelist);
>         } else {
> +               bool lru = !__PageMovable(page);
> +
>                 if (lru)
>                         isolated = !isolate_lru_page(page);
>                 else
>                         isolated = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
> 
> -               if (isolated)
> +               if (isolated) {
>                         list_add(&page->lru, pagelist);
> +                       if (lru)
> +                               inc_node_page_state(page, NR_ISOLATED_ANON +
> +                                                   page_is_file_lru(page));
> +               }
>         }
> 
> -       if (isolated && lru)
> -               inc_node_page_state(page, NR_ISOLATED_ANON +
> -                                   page_is_file_lru(page));
> -
>         /*
>          * If we succeed to isolate the page, we grabbed another refcount on
>          * the page, so we can safely drop the one we got from get_any_pages().
> 

Yes, that's exactly what I thought of.

Thanks,
Naoya Horiguchi