Re: [PATCH next] mm/vmscan: __isolate_lru_page_prepare clean up

From: Matthew Wilcox
Date: Sun Nov 22 2020 - 07:42:10 EST


On Sun, Nov 22, 2020 at 08:00:19PM +0800, Alex Shi wrote:
> mm/compaction.c | 2 +-
> mm/vmscan.c | 69 +++++++++++++++++++++++--------------------------
> 2 files changed, 34 insertions(+), 37 deletions(-)

How is it possible you're changing the signature of a function without
touching a header file? Surely __isolate_lru_page_prepare() must be declared
in mm/internal.h ?

> +++ b/mm/vmscan.c
> @@ -1536,19 +1536,17 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
> * page: page to consider
> * mode: one of the LRU isolation modes defined above
> *
> - * returns 0 on success, -ve errno on failure.
> + * returns ture on success, false on failure.

"true".

> @@ -1674,35 +1672,34 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> * only when the page is being freed somewhere else.
> */
> scan += nr_pages;
> - switch (__isolate_lru_page_prepare(page, mode)) {
> - case 0:
> + if (!__isolate_lru_page_prepare(page, mode)) {
> + /* else it is being freed elsewhere */

I don't think the word "else" helps here. Just
/* It is being freed elsewhere */

> + if (!TestClearPageLRU(page)) {
> /*
> + * This page may in other isolation path,
> + * but we still hold lru_lock.
> */

I don't think this comment helps me understand what's going on here.
Maybe:

/* Another thread is already isolating this page */

> + put_page(page);
> list_move(&page->lru, src);
> + continue;
> }