Messages in this thread | | | Date | Thu, 16 Feb 2023 10:07:05 +0800 | Subject | Re: [PATCH v3 4/4] mm: change to return bool for isolate_movable_page() | From | Baolin Wang <> |
| |
On 2/15/2023 11:44 PM, Matthew Wilcox wrote: > On Wed, Feb 15, 2023 at 06:39:37PM +0800, Baolin Wang wrote: >> Now the isolate_movable_page() can only return 0 or -EBUSY, and no users >> will care about the negative return value, thus we can convert the >> isolate_movable_page() to return a boolean value to make the code more >> clear when checking the movable page isolation state. >> >> No functional changes intended. >> >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >> Acked-by: David Hildenbrand <david@redhat.com> > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > A couple of nits below, not worth respinning the patch series for: > >> diff --git a/include/linux/migrate.h b/include/linux/migrate.h >> index c88b96b48be7..6b252f519c86 100644 >> --- a/include/linux/migrate.h >> +++ b/include/linux/migrate.h >> @@ -71,7 +71,7 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, >> unsigned long private, enum migrate_mode mode, int reason, >> unsigned int *ret_succeeded); >> extern struct page *alloc_migration_target(struct page *page, unsigned long private); >> -extern int isolate_movable_page(struct page *page, isolate_mode_t mode); >> +extern bool isolate_movable_page(struct page *page, isolate_mode_t mode); > > You can drop the 'extern' here. > >> +++ b/mm/memory_hotplug.c >> @@ -1668,18 +1668,18 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) >> * We can skip free pages. And we can deal with pages on >> * LRU and non-lru movable pages. >> */ >> - if (PageLRU(page)) { >> + if (PageLRU(page)) >> isolated = isolate_lru_page(page); >> - ret = isolated ? 0 : -EBUSY; >> - } else >> - ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE); >> - if (!ret) { /* Success */ >> + else >> + isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE); >> + if (isolated) { /* Success */ > > I would have dropped the "/* Success */" here. Before, commenting > "!ret" is quite sensible, but "isolated" seems obviously success to me.
Right. Hope Andrew can help to drop this unnecessary comment:)
Thanks for reviewing.
| |