On 23 Jun 2025, at 11:47, David Hildenbrand wrote:

On 20.06.25 22:37, Zi Yan wrote:

quoted

On 18 Jun 2025, at 13:39, David Hildenbrand wrote:

quoted

Let's make it clearer that we are talking about movable_ops pages.

Signed-off-by: David Hildenbrand <redacted>
---
  include/linux/migrate.h    |  2 +-
  include/linux/page-flags.h |  2 +-
  mm/compaction.c            |  7 ++-----
  mm/memory-failure.c        |  4 ++--
  mm/memory_hotplug.c        |  8 +++-----
  mm/migrate.c               |  8 ++++----
  mm/page_alloc.c            |  2 +-
  mm/page_isolation.c        | 10 +++++-----
  8 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 204e89eac998f..c575778456f97 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h

@@ -115,7 +115,7 @@ static inline void __SetPageMovable(struct page *page,
  static inline
  const struct movable_operations *page_movable_ops(struct page *page)
  {
-	VM_BUG_ON(!__PageMovable(page));
+	VM_BUG_ON(!page_has_movable_ops(page));

  	return (const struct movable_operations *)
  		((unsigned long)page->mapping - PAGE_MAPPING_MOVABLE);

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 4fe5ee67535b2..c67163b73c5ec 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h

@@ -750,7 +750,7 @@ static __always_inline bool __folio_test_movable(const struct folio *folio)
  			PAGE_MAPPING_MOVABLE;
  }

-static __always_inline bool __PageMovable(const struct page *page)
+static __always_inline bool page_has_movable_ops(const struct page *page)
  {
  	return ((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) ==
  				PAGE_MAPPING_MOVABLE;

diff --git a/mm/compaction.c b/mm/compaction.c
index 5c37373017014..f8b7c09e2e48c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c

@@ -1056,11 +1056,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
  		 * Skip any other type of page
  		 */
  		if (!PageLRU(page)) {
-			/*
-			 * __PageMovable can return false positive so we need
-			 * to verify it under page_lock.
-			 */
-			if (unlikely(__PageMovable(page)) &&
+			/* Isolation will grab the page lock. */

I feel that the removed comment should stay, since the current comment
makes no sense when I read it alone.

Well, talking about the page lock is moot either way. The thing is, anything can change while we don't hold a page reference. So should we change the comment to

/* isolation code will deal with any races. */

Sounds good to me.

... or drop it completely?

quoted

In addition, why is __PageMovable() is renamed to page_has_movable_ops() but
__SetPageMovable() stays the same? page_has_movable_ops() and __SetPageMovable()
are functions for checking and setting PAGE_MAPPING_MOVABLE. The naming just
does not look symmetric.

See follow-up commits where __SetPageMovable() is cleaned up.

Right. It becomes clear at Patch 20.

Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi