Skip to contentSkip to conversation

mm/page_owner: misc cleanups

PATCH Superseded Ye Liu 3 participants 23 messages Jun 26 – Jun 30

Subsystems (2)
Memory management Page allocator

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Ye Liu

author
#0 lore raw

v2 cover mm/page_owner: misc cleanups

v2:
- Add cover letter (no code changes).
- Link:https://lore.kernel.org/all/[email protected]/

This series collects a few cleanups for mm/page_owner.c that have been accumulated while reading through the file. There is no functional change -- the goal is to make the code easier to read and maintain.

Patch 1 consolidates three identical PageBuddy skip blocks into a single skip_buddy_pages() helper, eliminating the duplication and keeping the lockless-read comment in one place.

Patch 2 replaces the -1 magic number used for "never migrated" with a local MIGRATE_REASON_NONE define, making the intent explicit at every use site.

Patch 3 hoists the CONFIG_MEMCG guard out of print_page_owner_memcg()'s body so that the real implementation and the empty stub are two clearly separate definitions, the common kernel idiom.

Patch 4 adds a missing \n to the count_threshold debugfs attribute format string so that cat(1) output is properly terminated.

Patch 5 moves free_ts_nsec from the allocation summary line to the free section in __dump_page_owner(), grouping it with free_pid and free_tgid where it logically belongs. This also makes the dump output consistent with print_page_owner().

Patch 6 drops the redundant page_owner_ prefix from file-scoped static symbols (stack_fops, threshold_fops, etc.). Since they cannot collide across translation units, the prefix carries no information.

The series is based on v6.17-rc1 and has been compile-tested with and without CONFIG_MEMCG.

Ye Liu (6):
mm/page_owner: extract skip_buddy_pages() helper to unify buddy page
skipping
mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for
last_migrate_reason
mm/page_owner: hoist CONFIG_MEMCG to function level for
print_page_owner_memcg()
mm/page_owner: add missing newline to count_threshold format string mm/page_owner: move free_ts_nsec output to free section in

    __dump_page_owner()

mm/page_owner: drop redundant page_owner prefix from static symbols

 mm/page_owner.c | 121 +++++++++++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 54 deletions(-)
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v2 1/6 mm/page_owner: extract skip_buddy_pages() helper to unify buddy page skipping

Three places in page_owner.c duplicate the same pattern: check if a page is PageBuddy, read its order via buddy_order_unsafe(), advance the pfn past the buddy block if the order is valid, and continue.

Consolidate them into a single inline helper skip_buddy_pages(). The function returns true (skip) for any buddy page and advances @pfn past the block when the order is valid; returns false if the page is not a buddy page and should be processed normally.

The old init_pages_in_zone() variant used "order > 0" as an extra guard before advancing pfn, but the continue was unconditional and (1UL << 0) - 1 == 0, so the behaviour is identical. The comment about zone->lock is preserved in the helper's kernel-doc.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 52 ++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)
mm/page_owner.c +26 -26
index 2dddcb6510aa..342549891a8d 100644
@@ -422,6 +422,29 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
 	rcu_read_unlock();
 }
 
+/*
+ * Check if a page is a buddy page and advance @pfn past the entire buddy block.
+ * This safely reads the buddy order without the zone lock, which may cause us
+ * to skip less than the full buddy block, but that is acceptable for page owner
+ * iteration purposes.
+ *
+ * Return: true if the page was skipped (caller should continue its loop),
+ *         false if the page is not a buddy page and should be processed normally.
+ */
+static inline bool skip_buddy_pages(unsigned long *pfn, struct page *page)
+{
+	unsigned long order;
+
+	if (!PageBuddy(page))
+		return false;
+
+	order = buddy_order_unsafe(page);
+	if (order <= MAX_PAGE_ORDER)
+		*pfn += (1UL << order) - 1;
+
+	return true;
+}
+
 void pagetypeinfo_showmixedcount_print(struct seq_file *m,
 				       pg_data_t *pgdat, struct zone *zone)
 {

@@ -461,14 +484,8 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
 			if (page_zone(page) != zone)
 				continue;
 
-			if (PageBuddy(page)) {
-				unsigned long freepage_order;
-
-				freepage_order = buddy_order_unsafe(page);
-				if (freepage_order <= MAX_PAGE_ORDER)
-					pfn += (1UL << freepage_order) - 1;
+			if (skip_buddy_pages(&pfn, page))
 				continue;
-			}
 
 			if (PageReserved(page))
 				continue;

@@ -697,13 +714,8 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		}
 
 		page = pfn_to_page(pfn);
-		if (PageBuddy(page)) {
-			unsigned long freepage_order = buddy_order_unsafe(page);
-
-			if (freepage_order <= MAX_PAGE_ORDER)
-				pfn += (1UL << freepage_order) - 1;
+		if (skip_buddy_pages(&pfn, page))
 			continue;
-		}
 
 		page_ext = page_ext_get(page);
 		if (unlikely(!page_ext))

@@ -798,20 +810,8 @@ static void init_pages_in_zone(struct zone *zone)
 			if (page_zone(page) != zone)
 				continue;
 
-			/*
-			 * To avoid having to grab zone->lock, be a little
-			 * careful when reading buddy page order. The only
-			 * danger is that we skip too much and potentially miss
-			 * some early allocated pages, which is better than
-			 * heavy lock contention.
-			 */
-			if (PageBuddy(page)) {
-				unsigned long order = buddy_order_unsafe(page);
-
-				if (order > 0 && order <= MAX_PAGE_ORDER)
-					pfn += (1UL << order) - 1;
+			if (skip_buddy_pages(&pfn, page))
 				continue;
-			}
 
 			if (PageReserved(page))
 				continue;
Signature · 1 line
2.43.0
reviewer Reviewed-by on v2 1/6
#2 lore raw

Re: [PATCH v2 1/6] mm/page_owner: extract skip_buddy_pages() helper to unify buddy page skipping

Quoted 14 lines · Ye Liu

Three places in page_owner.c duplicate the same pattern: check if a page is PageBuddy, read its order via buddy_order_unsafe(), advance the pfn past the buddy block if the order is valid, and continue.

Consolidate them into a single inline helper skip_buddy_pages(). The function returns true (skip) for any buddy page and advances @pfn past the block when the order is valid; returns false if the page is not a buddy page and should be processed normally.

The old init_pages_in_zone() variant used "order > 0" as an extra guard before advancing pfn, but the continue was unconditional and (1UL << 0) - 1 == 0, so the behaviour is identical. The comment about zone->lock is preserved in the helper's kernel-doc.

Quoted 7 lines

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 52 ++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

LGTM.

Reviewed-by Zi Yan <[email protected]>
Signature · 2 lines
Best Regards,
Yan, Zi
maintainer Reviewed-by on v2 1/6
#3 lore raw

Re: [PATCH v2 1/6] mm/page_owner: extract skip_buddy_pages() helper to unify buddy page skipping

In reply to Ye Liu
Quoted 9 lines · Ye Liu

Three places in page_owner.c duplicate the same pattern: check if a page is PageBuddy, read its order via buddy_order_unsafe(), advance the pfn past the buddy block if the order is valid, and continue.

Consolidate them into a single inline helper skip_buddy_pages(). The function returns true (skip) for any buddy page and advances @pfn past the block when the order is valid; returns false if the page is not a buddy page and should be processed normally.

Quoted 8 lines

The old init_pages_in_zone() variant used "order > 0" as an extra guard before advancing pfn, but the continue was unconditional and (1UL << 0) - 1 == 0, so the behaviour is identical. The comment about zone->lock is preserved in the helper's kernel-doc.

No functional change.

Signed-off-by Ye Liu <[email protected]>
Reviewed-by Vlastimil Babka (SUSE) <[email protected]>
author
#4 lore raw

PATCH v2 2/6 mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason

In reply to Ye Liu

The last_migrate_reason field uses -1 as a sentinel value to mean "no migration has happened". Replace the four bare -1 occurrences with a local MIGRATE_REASON_NONE define so the intent is explicit and the magic number is eliminated.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
mm/page_owner.c +11 -4
index 342549891a8d..ebafa9d7ff07 100644
@@ -21,6 +21,13 @@
  */
 #define PAGE_OWNER_STACK_DEPTH (16)
 
+/*
+ * Used to indicate that a page has never been migrated, as the valid
+ * migrate_reason values are non-negative enum members (MR_* in
+ * include/linux/migrate_mode.h).
+ */
+#define MIGRATE_REASON_NONE (-1)
+
 struct page_owner {
 	unsigned short order;
 	short last_migrate_reason;

@@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
 	depot_stack_handle_t handle;
 
 	handle = save_stack(gfp_mask);
-	__update_page_owner_handle(page, handle, order, gfp_mask, -1,
+	__update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE,
 				   ts_nsec, current->pid, current->tgid,
 				   current->comm);
 	inc_stack_record_count(handle, gfp_mask, 1 << order);

@@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
-	if (page_owner->last_migrate_reason != -1) {
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) {
 		ret += scnprintf(kbuf + ret, count - ret,
 			"Page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);

@@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page)
 		stack_depot_print(handle);
 	}
 
-	if (page_owner->last_migrate_reason != -1)
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE)
 		pr_alert("page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);
 	page_ext_put(page_ext);

@@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone)
 
 			/* Found early allocated page */
 			__update_page_owner_handle(page, early_handle, 0, 0,
-						   -1, local_clock(), current->pid,
+						   MIGRATE_REASON_NONE, local_clock(), current->pid,
 						   current->tgid, current->comm);
 			count++;
 ext_put_continue:
Signature · 1 line
2.43.0
reviewer
#5 lore raw

Re: [PATCH v2 2/6] mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason

Quoted 17 lines · Ye Liu

The last_migrate_reason field uses -1 as a sentinel value to mean "no migration has happened". Replace the four bare -1 occurrences with a local MIGRATE_REASON_NONE define so the intent is explicit and the magic number is eliminated.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 342549891a8d..ebafa9d7ff07 100644

mm/page_owner.c line 27

Quoted 7 lines
mm/page_owner.c +4 -0
@@ -21,6 +21,13 @@
  */
 #define PAGE_OWNER_STACK_DEPTH (16)
 
+/*
+ * Used to indicate that a page has never been migrated, as the valid
+ * migrate_reason values are non-negative enum members (MR_* in
+ * include/linux/migrate_mode.h).

Why not change last_migrate_reason to enum migrate_reason and functions pass migrate reasons? MR_TYPES probably can be used to indicate no migration has happened if we do not want to add MR_NO_MIGRATION.

folio_set_owner_migrate_reason(), __folio_set_owner_migrate_reason(),
move_hugetlb_state(), and
__update_page_owner_handle() need to update their signatures for this.

Quoted 42 lines
mm/page_owner.c +7 -4
+ */
+#define MIGRATE_REASON_NONE (-1)
+
 struct page_owner {
 	unsigned short order;
 	short last_migrate_reason;

@@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
 	depot_stack_handle_t handle;
 
 	handle = save_stack(gfp_mask);
-	__update_page_owner_handle(page, handle, order, gfp_mask, -1,
+	__update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE,
 				   ts_nsec, current->pid, current->tgid,
 				   current->comm);
 	inc_stack_record_count(handle, gfp_mask, 1 << order);

@@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
-	if (page_owner->last_migrate_reason != -1) {
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) {
 		ret += scnprintf(kbuf + ret, count - ret,
 			"Page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);

@@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page)
 		stack_depot_print(handle);
 	}
 
-	if (page_owner->last_migrate_reason != -1)
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE)
 		pr_alert("page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);
 	page_ext_put(page_ext);

@@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone)
 
 			/* Found early allocated page */
 			__update_page_owner_handle(page, early_handle, 0, 0,
-						   -1, local_clock(), current->pid,
+						   MIGRATE_REASON_NONE, local_clock(), current->pid,
 						   current->tgid, current->comm);
 			count++;
 ext_put_continue:
Signature · 2 lines
Best Regards,
Yan, Zi
author
#6 lore raw

Re: [PATCH v2 2/6] mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason

Quoted 30 lines · Zi Yan
Quoted 17 lines · Ye Liu

The last_migrate_reason field uses -1 as a sentinel value to mean "no migration has happened". Replace the four bare -1 occurrences with a local MIGRATE_REASON_NONE define so the intent is explicit and the magic number is eliminated.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 342549891a8d..ebafa9d7ff07 100644

mm/page_owner.c line 27

Quoted 7 lines
mm/page_owner.c +4 -0
@@ -21,6 +21,13 @@
  */
 #define PAGE_OWNER_STACK_DEPTH (16)
 
+/*
+ * Used to indicate that a page has never been migrated, as the valid
+ * migrate_reason values are non-negative enum members (MR_* in
+ * include/linux/migrate_mode.h).

Why not change last_migrate_reason to enum migrate_reason and functions pass migrate reasons? MR_TYPES probably can be used to indicate no migration has happened if we do not want to add MR_NO_MIGRATION.

Quoted 5 lines

folio_set_owner_migrate_reason(), __folio_set_owner_migrate_reason(),
move_hugetlb_state(), and
__update_page_owner_handle() need to update their signatures for this.

Thanks for the review.

Using MR_TYPES as a sentinel is problematic - it's used kernel-wide as an array-size marker (migrate_reason_names[MR_TYPES]). Overloading it to also mean "never migrated" conflates two unrelated semantics. It also has a subtle out-of-bounds risk: if anyone ever uses the field as an array index without the guard check, MR_TYPES silently points past the end of the array, while -1 faults immediately.

Adding MR_NONE = -1 to the enum is the right long-term fix, but that needs proper enum definition changes and cascading signature updates across multiple headers — better done as a separate series.
The current patch is a minimal improvement: it kills the magic number and makes intent explicit without touching any header.

Quoted 47 lines
Quoted 43 lines
mm/page_owner.c +7 -4
+ */
+#define MIGRATE_REASON_NONE (-1)
+
 struct page_owner {
 	unsigned short order;
 	short last_migrate_reason;

@@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
 	depot_stack_handle_t handle;
 
 	handle = save_stack(gfp_mask);
-	__update_page_owner_handle(page, handle, order, gfp_mask, -1,
+	__update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE,
 				   ts_nsec, current->pid, current->tgid,
 				   current->comm);
 	inc_stack_record_count(handle, gfp_mask, 1 << order);

@@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
-	if (page_owner->last_migrate_reason != -1) {
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) {
 		ret += scnprintf(kbuf + ret, count - ret,
 			"Page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);

@@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page)
 		stack_depot_print(handle);
 	}
 
-	if (page_owner->last_migrate_reason != -1)
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE)
 		pr_alert("page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);
 	page_ext_put(page_ext);

@@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone)
 
 			/* Found early allocated page */
 			__update_page_owner_handle(page, early_handle, 0, 0,
-						   -1, local_clock(), current->pid,
+						   MIGRATE_REASON_NONE, local_clock(), current->pid,
 						   current->tgid, current->comm);
 			count++;
 ext_put_continue:
Signature · 2 lines
Thanks,
Ye Liu
maintainer
#7 lore raw

Re: [PATCH v2 2/6] mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason

Quoted 24 lines · Ye Liu
Quoted 21 lines · Zi Yan
Quoted 20 lines · Ye Liu

The last_migrate_reason field uses -1 as a sentinel value to mean "no migration has happened". Replace the four bare -1 occurrences with a local MIGRATE_REASON_NONE define so the intent is explicit and the magic number is eliminated.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 342549891a8d..ebafa9d7ff07 100644
@@ -21,6 +21,13 @@
  */
 #define PAGE_OWNER_STACK_DEPTH (16)
Quoted 8 lines
Quoted 8 lines

mm/page_owner.c line 27

Quoted 4 lines
mm/page_owner.c +4 -0
+/*
+ * Used to indicate that a page has never been migrated, as the valid
+ * migrate_reason values are non-negative enum members (MR_* in
+ * include/linux/migrate_mode.h).

Why not change last_migrate_reason to enum migrate_reason and functions pass migrate reasons? MR_TYPES probably can be used to indicate no migration has happened if we do not want to add MR_NO_MIGRATION.

Indeed. I'd just add a proper MR_NEVER option with a comment.

Quoted 4 lines
Quoted 4 lines

folio_set_owner_migrate_reason(), __folio_set_owner_migrate_reason(),
move_hugetlb_state(), and
__update_page_owner_handle() need to update their signatures for this.

They should ideally, but do they "need" as part of this series? But I guess it's not a big deal.

Quoted 5 lines

Thanks for the review.

Using MR_TYPES as a sentinel is problematic - it's used kernel-wide as an array-size marker (migrate_reason_names[MR_TYPES]). Overloading it to also mean "never migrated" conflates two unrelated semantics. It also has a

Well it's just for tracepoint and dumping page owner. Nothing prevents us assigning a value to MR_NEVER that's not -1 (just make MR_NEVER the first or last option of the enum) and a give it a name e.g. "never_migrated" in include/trace/events/migrate.h

Quoted 3 lines

subtle out-of-bounds risk: if anyone ever uses the field as an array index without the guard check, MR_TYPES silently points past the end of the array, while -1 faults immediately.

That wouldn't be a problem either with a proper value.

Quoted 5 lines

Adding MR_NONE = -1 to the enum is the right long-term fix, but that needs proper enum definition changes and cascading signature updates across multiple headers — better done as a separate series.
The current patch is a minimal improvement: it kills the magic number and makes intent explicit without touching any header.

Minimal fixes are sometimes necessary for actual bugs, for stable backport etc. Nothing prevents us doing it properly here.

Quoted 48 lines
Quoted 47 lines
Quoted 43 lines
mm/page_owner.c +7 -4
+ */
+#define MIGRATE_REASON_NONE (-1)
+
 struct page_owner {
 	unsigned short order;
 	short last_migrate_reason;

@@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
 	depot_stack_handle_t handle;
 
 	handle = save_stack(gfp_mask);
-	__update_page_owner_handle(page, handle, order, gfp_mask, -1,
+	__update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE,
 				   ts_nsec, current->pid, current->tgid,
 				   current->comm);
 	inc_stack_record_count(handle, gfp_mask, 1 << order);

@@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
-	if (page_owner->last_migrate_reason != -1) {
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) {
 		ret += scnprintf(kbuf + ret, count - ret,
 			"Page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);

@@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page)
 		stack_depot_print(handle);
 	}
 
-	if (page_owner->last_migrate_reason != -1)
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE)
 		pr_alert("page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);
 	page_ext_put(page_ext);

@@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone)
 
 			/* Found early allocated page */
 			__update_page_owner_handle(page, early_handle, 0, 0,
-						   -1, local_clock(), current->pid,
+						   MIGRATE_REASON_NONE, local_clock(), current->pid,
 						   current->tgid, current->comm);
 			count++;
 ext_put_continue:
author
#8 lore raw

Re: [PATCH v2 2/6] mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason

Quoted 61 lines · Vlastimil Babka (SUSE)
Quoted 24 lines · Ye Liu
Quoted 21 lines · Zi Yan
Quoted 20 lines · Ye Liu

The last_migrate_reason field uses -1 as a sentinel value to mean "no migration has happened". Replace the four bare -1 occurrences with a local MIGRATE_REASON_NONE define so the intent is explicit and the magic number is eliminated.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 342549891a8d..ebafa9d7ff07 100644
@@ -21,6 +21,13 @@
  */
 #define PAGE_OWNER_STACK_DEPTH (16)
Quoted 8 lines
Quoted 8 lines

mm/page_owner.c line 27

Quoted 4 lines
mm/page_owner.c +4 -0
+/*
+ * Used to indicate that a page has never been migrated, as the valid
+ * migrate_reason values are non-negative enum members (MR_* in
+ * include/linux/migrate_mode.h).

Why not change last_migrate_reason to enum migrate_reason and functions pass migrate reasons? MR_TYPES probably can be used to indicate no migration has happened if we do not want to add MR_NO_MIGRATION.

Indeed. I'd just add a proper MR_NEVER option with a comment.

Quoted 4 lines
Quoted 4 lines

folio_set_owner_migrate_reason(), __folio_set_owner_migrate_reason(),
move_hugetlb_state(), and
__update_page_owner_handle() need to update their signatures for this.

They should ideally, but do they "need" as part of this series? But I guess it's not a big deal.

Quoted 5 lines

Thanks for the review.

Using MR_TYPES as a sentinel is problematic - it's used kernel-wide as an array-size marker (migrate_reason_names[MR_TYPES]). Overloading it to also mean "never migrated" conflates two unrelated semantics. It also has a

Well it's just for tracepoint and dumping page owner. Nothing prevents us assigning a value to MR_NEVER that's not -1 (just make MR_NEVER the first or last option of the enum) and a give it a name e.g. "never_migrated" in include/trace/events/migrate.h

Quoted 3 lines

subtle out-of-bounds risk: if anyone ever uses the field as an array index without the guard check, MR_TYPES silently points past the end of the array, while -1 faults immediately.

That wouldn't be a problem either with a proper value.

Quoted 9 lines
Quoted 5 lines

Adding MR_NONE = -1 to the enum is the right long-term fix, but that needs proper enum definition changes and cascading signature updates across multiple headers — better done as a separate series.
The current patch is a minimal improvement: it kills the magic number and makes intent explicit without touching any header.

Minimal fixes are sometimes necessary for actual bugs, for stable backport etc. Nothing prevents us doing it properly here.

Indeed, let me do it properly. In v3 I'll split this into two patches:

1. Add MR_NEVER (value 10) to enum migrate_reason in migrate_mode.h, add the corresponding "never_migrated" string in the MIGRATE_REASON trace macro, and replace the four bare -1 with MR_NEVER in page_owner.c. This removes the local MIGRATE_REASON_NONE define entirely.
2. A follow-up cleanup that converts all 'int reason' parameters in the callchain to 'enum migrate_reason'. The affected functions are:

2. page_owner: __folio_set_owner_migrate_reason(),
folio_set_owner_migrate_reason()
migrate: migrate_pages(), migrate_pages_sync(),
migrate_pages_batch(), migrate_folios_move(), migrate_hugetlbs(), unmap_and_move_huge_page()
hugetlb: move_hugetlb_state(), htlb_allow_alloc_fallback()
trace: mm_migrate_pages / mm_migrate_pages_start events

2. The 'short last_migrate_reason' struct field and the __update_page_owner_handle() parameter are intentionally left as 'short' since struct page_owner is per-page metadata and the size matters there.

Thanks for the review.

Quoted 49 lines
Quoted 48 lines
Quoted 47 lines
Quoted 43 lines
mm/page_owner.c +7 -4
+ */
+#define MIGRATE_REASON_NONE (-1)
+
 struct page_owner {
 	unsigned short order;
 	short last_migrate_reason;

@@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
 	depot_stack_handle_t handle;
 
 	handle = save_stack(gfp_mask);
-	__update_page_owner_handle(page, handle, order, gfp_mask, -1,
+	__update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE,
 				   ts_nsec, current->pid, current->tgid,
 				   current->comm);
 	inc_stack_record_count(handle, gfp_mask, 1 << order);

@@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
-	if (page_owner->last_migrate_reason != -1) {
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) {
 		ret += scnprintf(kbuf + ret, count - ret,
 			"Page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);

@@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page)
 		stack_depot_print(handle);
 	}
 
-	if (page_owner->last_migrate_reason != -1)
+	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE)
 		pr_alert("page has been migrated, last migrate reason: %s\n",
 			migrate_reason_names[page_owner->last_migrate_reason]);
 	page_ext_put(page_ext);

@@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone)
 
 			/* Found early allocated page */
 			__update_page_owner_handle(page, early_handle, 0, 0,
-						   -1, local_clock(), current->pid,
+						   MIGRATE_REASON_NONE, local_clock(), current->pid,
 						   current->tgid, current->comm);
 			count++;
 ext_put_continue:
Signature · 2 lines
Thanks,
Ye Liu
author
#9 lore raw

PATCH v2 3/6 mm/page_owner: hoist CONFIG_MEMCG to function level for print_page_owner_memcg()

In reply to Ye Liu

The print_page_owner_memcg() function has CONFIG_MEMCG guarding its entire body via #ifdef inside the function, which leaves a no-op { return ret; } when the config is disabled. Hoist the #ifdef to the top level so the real implementation and the empty stub are two clearly separated definitions.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
mm/page_owner.c +8 -2
index ebafa9d7ff07..5d8773c90cd9 100644
@@ -529,13 +529,13 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
 	seq_putc(m, '\n');
 }
 
+#ifdef CONFIG_MEMCG
 /*
  * Looking for memcg information and print it out
  */
 static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
 					 struct page *page)
 {
-#ifdef CONFIG_MEMCG
 	unsigned long memcg_data;
 	struct mem_cgroup *memcg;
 	bool online;

@@ -563,10 +563,16 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
 			name);
 out_unlock:
 	rcu_read_unlock();
-#endif /* CONFIG_MEMCG */
 
 	return ret;
 }
+#else
+static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
+					 struct page *page)
+{
+	return ret;
+}
+#endif
 
 static ssize_t
 print_page_owner(char __user *buf, size_t count, unsigned long pfn,
Signature · 1 line
2.43.0
reviewer Reviewed-by on v2 3/6
#10 lore raw

Re: [PATCH v2 3/6] mm/page_owner: hoist CONFIG_MEMCG to function level for print_page_owner_memcg()

Quoted 2 lines · Ye Liu

The print_page_owner_memcg() function has CONFIG_MEMCG guarding its entire body via #ifdef inside the function, which leaves a no-op

no-op might not be accurate, since the purpose is not to change scnprintf() return values from prior invocations. Maybe just remove "no-op" in the commit message to avoid confusion.

Quoted 11 lines

{ return ret; } when the config is disabled. Hoist the #ifdef to the top level so the real implementation and the empty stub are two clearly separated definitions.

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

<snip>

mm/page_owner.c static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret, · line 573

Quoted 5 lines
mm/page_owner.c +5 -0
+#else
+static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
+					 struct page *page)
+{
+	return ret;

It is not obvious why we need to return ret here, until I find that ret cumulates scnprintf() return values.

mm/page_owner.c static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret, · line 578

Quoted 5 lines
mm/page_owner.c +2 -0
+}
+#endif
 
 static ssize_t
 print_page_owner(char __user *buf, size_t count, unsigned long pfn,

LGTM.

Reviewed-by Zi Yan <[email protected]>
Signature · 2 lines
Best Regards,
Yan, Zi
maintainer Reviewed-by on v2 3/6
#11 lore raw

Re: [PATCH v2 3/6] mm/page_owner: hoist CONFIG_MEMCG to function level for print_page_owner_memcg()

In reply to Ye Liu
Quoted 3 lines · Ye Liu

The print_page_owner_memcg() function has CONFIG_MEMCG guarding its entire body via #ifdef inside the function, which leaves a no-op { return ret; } when the config is disabled. Hoist the #ifdef to the

IMHO it's not a big deal for an internal function with the ifdef block being so straightforward. But whatever.

Quoted 6 lines

top level so the real implementation and the empty stub are two clearly separated definitions.

No functional change.

Signed-off-by Ye Liu <[email protected]>
Reviewed-by Vlastimil Babka (SUSE) <[email protected]>
author
#12 lore raw

PATCH v2 4/6 mm/page_owner: add missing newline to count_threshold format string

In reply to Ye Liu

The DEFINE_SIMPLE_ATTRIBUTE format string for page_owner_threshold_fops is missing a trailing \n. simple_attr_read() uses scnprintf() with the format string, which does not append a newline, so reading /sys/kernel/debug/page_owner_stacks/count_threshold produces output without a terminating newline. Add the missing \n to match the standard debugfs attribute convention.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/page_owner.c +1 -1
index 5d8773c90cd9..958151ccb587 100644
@@ -981,7 +981,7 @@ static int page_owner_threshold_set(void *data, u64 val)
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(page_owner_threshold_fops, &page_owner_threshold_get,
-			&page_owner_threshold_set, "%llu");
+			&page_owner_threshold_set, "%llu\n");
 
 
 static int __init pageowner_init(void)
Signature · 1 line
2.43.0
reviewer Reviewed-by on v2 4/6
#13 lore raw

Re: [PATCH v2 4/6] mm/page_owner: add missing newline to count_threshold format string

Quoted 7 lines · Ye Liu

The DEFINE_SIMPLE_ATTRIBUTE format string for page_owner_threshold_fops is missing a trailing \n. simple_attr_read() uses scnprintf() with the format string, which does not append a newline, so reading /sys/kernel/debug/page_owner_stacks/count_threshold produces output without a terminating newline. Add the missing \n to match the standard debugfs attribute convention.

Quoted 5 lines
Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

LGTM.

Reviewed-by Zi Yan <[email protected]>
Signature · 2 lines
Best Regards,
Yan, Zi
maintainer Reviewed-by on v2 4/6
#14 lore raw

Re: [PATCH v2 4/6] mm/page_owner: add missing newline to count_threshold format string

In reply to Ye Liu
Quoted 8 lines · Ye Liu

The DEFINE_SIMPLE_ATTRIBUTE format string for page_owner_threshold_fops is missing a trailing \n. simple_attr_read() uses scnprintf() with the format string, which does not append a newline, so reading /sys/kernel/debug/page_owner_stacks/count_threshold produces output without a terminating newline. Add the missing \n to match the standard debugfs attribute convention.

Signed-off-by Ye Liu <[email protected]>
Reviewed-by Vlastimil Babka (SUSE) <[email protected]>
Quoted 17 lines
---
 mm/page_owner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/page_owner.c +1 -1
index 5d8773c90cd9..958151ccb587 100644
@@ -981,7 +981,7 @@ static int page_owner_threshold_set(void *data, u64 val)
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(page_owner_threshold_fops, &page_owner_threshold_get,
-			&page_owner_threshold_set, "%llu");
+			&page_owner_threshold_set, "%llu\n");
 
 
 static int __init pageowner_init(void)
author
#15 lore raw

PATCH v2 5/6 mm/page_owner: move free_ts_nsec output to free section in __dump_page_owner()

In reply to Ye Liu

The free_ts_nsec field is a free-event timestamp, but it was printed in the allocation summary line alongside ts_nsec (allocation time). Move it to the free section where it logically belongs, together with free_pid and free_tgid. This also makes __dump_page_owner() consistent with print_page_owner(), which only prints ts_nsec in the allocation summary.

The output now groups all free-related information (pid, tgid, timestamp, stack trace) in one place.

No functional change except output formatting.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/page_owner.c +5 -4
index 958151ccb587..eaccff355cb4 100644
@@ -660,10 +660,10 @@ void __dump_page_owner(const struct page *page)
 	else
 		pr_alert("page_owner tracks the page as freed\n");
 
-	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu, free_ts %llu\n",
+	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu\n",
 		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
 		 page_owner->pid, page_owner->tgid, page_owner->comm,
-		 page_owner->ts_nsec, page_owner->free_ts_nsec);
+		 page_owner->ts_nsec);
 
 	handle = READ_ONCE(page_owner->handle);
 	if (!handle)

@@ -675,8 +675,9 @@ void __dump_page_owner(const struct page *page)
 	if (!handle) {
 		pr_alert("page_owner free stack trace missing\n");
 	} else {
-		pr_alert("page last free pid %d tgid %d stack trace:\n",
-			  page_owner->free_pid, page_owner->free_tgid);
+		pr_alert("page last free pid %d tgid %d ts %llu stack trace:\n",
+			  page_owner->free_pid, page_owner->free_tgid,
+			  page_owner->free_ts_nsec);
 		stack_depot_print(handle);
 	}
Signature · 1 line
2.43.0
reviewer
#16 lore raw

Re: [PATCH v2 5/6] mm/page_owner: move free_ts_nsec output to free section in __dump_page_owner()

Quoted 22 lines · Ye Liu

The free_ts_nsec field is a free-event timestamp, but it was printed in the allocation summary line alongside ts_nsec (allocation time). Move it to the free section where it logically belongs, together with free_pid and free_tgid. This also makes __dump_page_owner() consistent with print_page_owner(), which only prints ts_nsec in the allocation summary.

The output now groups all free-related information (pid, tgid, timestamp, stack trace) in one place.

No functional change except output formatting.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 958151ccb587..eaccff355cb4 100644

mm/page_owner.c void __dump_page_owner(const struct page *page) · line 663

Quoted 5 lines
mm/page_owner.c +1 -1
@@ -660,10 +660,10 @@ void __dump_page_owner(const struct page *page)
 	else
 		pr_alert("page_owner tracks the page as freed\n");
 
-	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu, free_ts %llu\n",
+	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu\n",

Is it going to break existing programs parsing this information? This is under debugfs, so probably does not matter?

Quoted 19 lines
mm/page_owner.c +4 -3
 		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
 		 page_owner->pid, page_owner->tgid, page_owner->comm,
-		 page_owner->ts_nsec, page_owner->free_ts_nsec);
+		 page_owner->ts_nsec);
 
 	handle = READ_ONCE(page_owner->handle);
 	if (!handle)

@@ -675,8 +675,9 @@ void __dump_page_owner(const struct page *page)
 	if (!handle) {
 		pr_alert("page_owner free stack trace missing\n");
 	} else {
-		pr_alert("page last free pid %d tgid %d stack trace:\n",
-			  page_owner->free_pid, page_owner->free_tgid);
+		pr_alert("page last free pid %d tgid %d ts %llu stack trace:\n",
+			  page_owner->free_pid, page_owner->free_tgid,
+			  page_owner->free_ts_nsec);
 		stack_depot_print(handle);
 	}
Signature · 2 lines
Best Regards,
Yan, Zi
author
#17 lore raw

Re: [PATCH v2 5/6] mm/page_owner: move free_ts_nsec output to free section in __dump_page_owner()

Quoted 26 lines · Zi Yan
Quoted 25 lines · Ye Liu

The free_ts_nsec field is a free-event timestamp, but it was printed in the allocation summary line alongside ts_nsec (allocation time). Move it to the free section where it logically belongs, together with free_pid and free_tgid. This also makes __dump_page_owner() consistent with print_page_owner(), which only prints ts_nsec in the allocation summary.

The output now groups all free-related information (pid, tgid, timestamp, stack trace) in one place.

No functional change except output formatting.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 958151ccb587..eaccff355cb4 100644
@@ -660,10 +660,10 @@ void __dump_page_owner(const struct page *page)
 	else
 		pr_alert("page_owner tracks the page as freed\n");
Quoted 5 lines

mm/page_owner.c void __dump_page_owner(const struct page *page) · line 663

Quoted 2 lines
mm/page_owner.c +1 -1
-	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu, free_ts %llu\n",
+	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu\n",

Is it going to break existing programs parsing this information? This is under debugfs, so probably does not matter?

This is debugfs output, not a stable ABI — debugfs format can change at any time. The field just moves position within the same function's output. Grouping all free-related info together makes __dump_page_owner() consistent with print_page_owner(), which already only prints ts_nsec in its allocation line. I'm not aware of any tools that parse this field, but if there are, I'm happy to update them as well.

Quoted 24 lines
Quoted 20 lines
mm/page_owner.c +4 -3
 		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
 		 page_owner->pid, page_owner->tgid, page_owner->comm,
-		 page_owner->ts_nsec, page_owner->free_ts_nsec);
+		 page_owner->ts_nsec);
 
 	handle = READ_ONCE(page_owner->handle);
 	if (!handle)

@@ -675,8 +675,9 @@ void __dump_page_owner(const struct page *page)
 	if (!handle) {
 		pr_alert("page_owner free stack trace missing\n");
 	} else {
-		pr_alert("page last free pid %d tgid %d stack trace:\n",
-			  page_owner->free_pid, page_owner->free_tgid);
+		pr_alert("page last free pid %d tgid %d ts %llu stack trace:\n",
+			  page_owner->free_pid, page_owner->free_tgid,
+			  page_owner->free_ts_nsec);
 		stack_depot_print(handle);
 	}
Signature · 2 lines
Thanks,
Ye Liu
reviewer Acked-by
#18 lore raw

Re: [PATCH v2 5/6] mm/page_owner: move free_ts_nsec output to free section in __dump_page_owner()

Quoted 35 lines · Ye Liu
Quoted 26 lines · Zi Yan
Quoted 25 lines · Ye Liu

The free_ts_nsec field is a free-event timestamp, but it was printed in the allocation summary line alongside ts_nsec (allocation time). Move it to the free section where it logically belongs, together with free_pid and free_tgid. This also makes __dump_page_owner() consistent with print_page_owner(), which only prints ts_nsec in the allocation summary.

The output now groups all free-related information (pid, tgid, timestamp, stack trace) in one place.

No functional change except output formatting.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/page_owner.c +0 -0
index 958151ccb587..eaccff355cb4 100644
@@ -660,10 +660,10 @@ void __dump_page_owner(const struct page *page)
 	else
 		pr_alert("page_owner tracks the page as freed\n");
Quoted 5 lines

mm/page_owner.c void __dump_page_owner(const struct page *page) · line 663

Quoted 2 lines
mm/page_owner.c +1 -1
-	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu, free_ts %llu\n",
+	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu\n",

Is it going to break existing programs parsing this information? This is under debugfs, so probably does not matter?

Quoted 6 lines

This is debugfs output, not a stable ABI — debugfs format can change at any time. The field just moves position within the same function's output. Grouping all free-related info together makes __dump_page_owner() consistent with print_page_owner(), which already only prints ts_nsec in its allocation line. I'm not aware of any tools that parse this field, but if there are, I'm happy to update them as well.

OK, feel free to add

Acked-by Zi Yan <[email protected]>
Quoted 25 lines
Quoted 25 lines
Quoted 21 lines
mm/page_owner.c +4 -3
 		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
 		 page_owner->pid, page_owner->tgid, page_owner->comm,
-		 page_owner->ts_nsec, page_owner->free_ts_nsec);
+		 page_owner->ts_nsec);
 
 	handle = READ_ONCE(page_owner->handle);
 	if (!handle)

@@ -675,8 +675,9 @@ void __dump_page_owner(const struct page *page)
 	if (!handle) {
 		pr_alert("page_owner free stack trace missing\n");
 	} else {
-		pr_alert("page last free pid %d tgid %d stack trace:\n",
-			  page_owner->free_pid, page_owner->free_tgid);
+		pr_alert("page last free pid %d tgid %d ts %llu stack trace:\n",
+			  page_owner->free_pid, page_owner->free_tgid,
+			  page_owner->free_ts_nsec);
 		stack_depot_print(handle);
 	}
Signature · 2 lines
Best Regards,
Yan, Zi
maintainer Reviewed-by on v2 5/6
#19 lore raw

Re: [PATCH v2 5/6] mm/page_owner: move free_ts_nsec output to free section in __dump_page_owner()

In reply to Ye Liu
Quoted 7 lines · Ye Liu

The free_ts_nsec field is a free-event timestamp, but it was printed in the allocation summary line alongside ts_nsec (allocation time). Move it to the free section where it logically belongs, together with free_pid and free_tgid. This also makes __dump_page_owner() consistent with print_page_owner(), which only prints ts_nsec in the allocation summary.

Quoted 6 lines

The output now groups all free-related information (pid, tgid, timestamp, stack trace) in one place.

No functional change except output formatting.

Signed-off-by Ye Liu <[email protected]>
Reviewed-by Vlastimil Babka (SUSE) <[email protected]>

If it breaks anything and they complain, we'll deal with that, but the risk is low in this case so it shouldn't stop us from trying.

Quoted 33 lines
---
 mm/page_owner.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/page_owner.c +5 -4
index 958151ccb587..eaccff355cb4 100644
@@ -660,10 +660,10 @@ void __dump_page_owner(const struct page *page)
 	else
 		pr_alert("page_owner tracks the page as freed\n");
 
-	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu, free_ts %llu\n",
+	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu\n",
 		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
 		 page_owner->pid, page_owner->tgid, page_owner->comm,
-		 page_owner->ts_nsec, page_owner->free_ts_nsec);
+		 page_owner->ts_nsec);
 
 	handle = READ_ONCE(page_owner->handle);
 	if (!handle)

@@ -675,8 +675,9 @@ void __dump_page_owner(const struct page *page)
 	if (!handle) {
 		pr_alert("page_owner free stack trace missing\n");
 	} else {
-		pr_alert("page last free pid %d tgid %d stack trace:\n",
-			  page_owner->free_pid, page_owner->free_tgid);
+		pr_alert("page last free pid %d tgid %d ts %llu stack trace:\n",
+			  page_owner->free_pid, page_owner->free_tgid,
+			  page_owner->free_ts_nsec);
 		stack_depot_print(handle);
 	}
author
#20 lore raw

PATCH v2 6/6 mm/page_owner: drop redundant page_owner prefix from static symbols

In reply to Ye Liu

All of these symbols are file-scoped (static) in page_owner.c, so the page_owner_ prefix is pure noise. Rename them to shorter, still-clear names:

  page_owner_stack_op      -> stack_op
  page_owner_stack_open    -> stack_open
  page_owner_stack_fops    -> stack_fops

page_owner_pages_threshold -> pages_threshold
page_owner_threshold_get -> threshold_get
page_owner_threshold_set -> threshold_set
page_owner_threshold_fops -> threshold_fops

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)
mm/page_owner.c +17 -18
index eaccff355cb4..ec9600025127 100644
@@ -901,7 +901,7 @@ static void *stack_next(struct seq_file *m, void *v, loff_t *ppos)
 	return stack;
 }
 
-static unsigned long page_owner_pages_threshold;
+static unsigned long pages_threshold;
 
 static int stack_print(struct seq_file *m, void *v)
 {

@@ -918,7 +918,7 @@ static int stack_print(struct seq_file *m, void *v)
 	nr_base_pages = refcount_read(&stack_record->count) - 1;
 
 	if (ctx->flags & STACK_PRINT_FLAG_PAGES &&
-	    (nr_base_pages < 1 || nr_base_pages < page_owner_pages_threshold))
+	    (nr_base_pages < 1 || nr_base_pages < pages_threshold))
 		return 0;
 
 	if (ctx->flags & STACK_PRINT_FLAG_STACK) {

@@ -940,16 +940,16 @@ static void stack_stop(struct seq_file *m, void *v)
 {
 }
 
-static const struct seq_operations page_owner_stack_op = {
+static const struct seq_operations stack_op = {
 	.start	= stack_start,
 	.next	= stack_next,
 	.stop	= stack_stop,
 	.show	= stack_print
 };
 
-static int page_owner_stack_open(struct inode *inode, struct file *file)
+static int stack_open(struct inode *inode, struct file *file)
 {
-	int ret = seq_open_private(file, &page_owner_stack_op,
+	int ret = seq_open_private(file, &stack_op,
 				   sizeof(struct stack_print_ctx));
 
 	if (!ret) {

@@ -962,28 +962,27 @@ static int page_owner_stack_open(struct inode *inode, struct file *file)
 	return ret;
 }
 
-static const struct file_operations page_owner_stack_fops = {
-	.open		= page_owner_stack_open,
+static const struct file_operations stack_fops = {
+	.open		= stack_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
 	.release	= seq_release_private,
 };
 
-static int page_owner_threshold_get(void *data, u64 *val)
+static int threshold_get(void *data, u64 *val)
 {
-	*val = READ_ONCE(page_owner_pages_threshold);
+	*val = READ_ONCE(pages_threshold);
 	return 0;
 }
 
-static int page_owner_threshold_set(void *data, u64 val)
+static int threshold_set(void *data, u64 val)
 {
-	WRITE_ONCE(page_owner_pages_threshold, val);
+	WRITE_ONCE(pages_threshold, val);
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(page_owner_threshold_fops, &page_owner_threshold_get,
-			&page_owner_threshold_set, "%llu\n");
-
+DEFINE_SIMPLE_ATTRIBUTE(threshold_fops, &threshold_get,
+			&threshold_set, "%llu\n");
 
 static int __init pageowner_init(void)
 {

@@ -999,17 +998,17 @@ static int __init pageowner_init(void)
 	debugfs_create_file("show_stacks", 0400, dir,
 			    (void *)(STACK_PRINT_FLAG_STACK |
 				     STACK_PRINT_FLAG_PAGES),
-			     &page_owner_stack_fops);
+			     &stack_fops);
 	debugfs_create_file("show_handles", 0400, dir,
 			    (void *)(STACK_PRINT_FLAG_HANDLE |
 				     STACK_PRINT_FLAG_PAGES),
-			    &page_owner_stack_fops);
+			    &stack_fops);
 	debugfs_create_file("show_stacks_handles", 0400, dir,
 			    (void *)(STACK_PRINT_FLAG_STACK |
 				     STACK_PRINT_FLAG_HANDLE),
-			    &page_owner_stack_fops);
+			    &stack_fops);
 	debugfs_create_file("count_threshold", 0600, dir, NULL,
-			    &page_owner_threshold_fops);
+			    &threshold_fops);
 	return 0;
 }
 late_initcall(pageowner_init)
Signature · 1 line
2.43.0
reviewer Acked-by on v2 6/6
#21 lore raw

Re: [PATCH v2 6/6] mm/page_owner: drop redundant page_owner prefix from static symbols

Quoted 12 lines · Ye Liu

All of these symbols are file-scoped (static) in page_owner.c, so the page_owner_ prefix is pure noise. Rename them to shorter, still-clear names:

  page_owner_stack_op      -> stack_op
  page_owner_stack_open    -> stack_open
  page_owner_stack_fops    -> stack_fops

page_owner_pages_threshold -> pages_threshold
page_owner_threshold_get -> threshold_get
page_owner_threshold_set -> threshold_set
page_owner_threshold_fops -> threshold_fops

Quoted 7 lines

No functional change.

Signed-off-by Ye Liu <[email protected]>
---
 mm/page_owner.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

The rename makes sense.

Acked-by Zi Yan <[email protected]>
Signature · 2 lines
Best Regards,
Yan, Zi
maintainer Reviewed-by on v2 6/6
#22 lore raw

Re: [PATCH v2 6/6] mm/page_owner: drop redundant page_owner prefix from static symbols

In reply to Ye Liu
Quoted 7 lines · Ye Liu

All of these symbols are file-scoped (static) in page_owner.c, so the page_owner_ prefix is pure noise. Rename them to shorter, still-clear names:

  page_owner_stack_op      -> stack_op
  page_owner_stack_open    -> stack_open
  page_owner_stack_fops    -> stack_fops
Quoted 8 lines

page_owner_pages_threshold -> pages_threshold
page_owner_threshold_get -> threshold_get
page_owner_threshold_set -> threshold_set
page_owner_threshold_fops -> threshold_fops

No functional change.

Signed-off-by Ye Liu <[email protected]>
Reviewed-by Vlastimil Babka (SUSE) <[email protected]>

Nit:

mm/page_owner.c static int page_owner_stack_open(struct inode *inode, struct file *file) · line 985

Quoted 13 lines
mm/page_owner.c +4 -5
-static int page_owner_threshold_set(void *data, u64 val)
+static int threshold_set(void *data, u64 val)
 {
-	WRITE_ONCE(page_owner_pages_threshold, val);
+	WRITE_ONCE(pages_threshold, val);
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(page_owner_threshold_fops, &page_owner_threshold_get,
-			&page_owner_threshold_set, "%llu\n");
-
+DEFINE_SIMPLE_ATTRIBUTE(threshold_fops, &threshold_get,
+			&threshold_set, "%llu\n");

We can now do make it a single line? 82 characters are fine unless hch sees this. checkpatch now warns on 100 chars

Quoted 25 lines

static int __init pageowner_init(void)
{

mm/page_owner.c +4 -4
@@ -999,17 +998,17 @@ static int __init pageowner_init(void)
 	debugfs_create_file("show_stacks", 0400, dir,
 			    (void *)(STACK_PRINT_FLAG_STACK |
 				     STACK_PRINT_FLAG_PAGES),
-			     &page_owner_stack_fops);
+			     &stack_fops);
 	debugfs_create_file("show_handles", 0400, dir,
 			    (void *)(STACK_PRINT_FLAG_HANDLE |
 				     STACK_PRINT_FLAG_PAGES),
-			    &page_owner_stack_fops);
+			    &stack_fops);
 	debugfs_create_file("show_stacks_handles", 0400, dir,
 			    (void *)(STACK_PRINT_FLAG_STACK |
 				     STACK_PRINT_FLAG_HANDLE),
-			    &page_owner_stack_fops);
+			    &stack_fops);
 	debugfs_create_file("count_threshold", 0600, dir, NULL,
-			    &page_owner_threshold_fops);
+			    &threshold_fops);
 	return 0;
 }
 late_initcall(pageowner_init)