Skip to contentSkip to conversation

mm: zone lock tracepoint instrumentation

PATCH Superseded Dmitry Ilvokhin 4 participants 18 messages Feb 11 – Feb 26

Subsystems (8)
Memory management MM core Tracing Page allocator +4 more subsystems

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Dmitry Ilvokhin

author
#0 lore raw

cover mm: zone lock tracepoint instrumentation

Zone lock contention can significantly impact allocation and reclaim latency, as it is a central synchronization point in the page allocator and reclaim paths. Improved visibility into its behavior is therefore important for diagnosing performance issues in memory-intensive workloads.

On some production workloads at Meta, we have observed noticeable zone lock contention. Deeper analysis of lock holders and waiters is currently difficult with existing instrumentation.

While generic lock contention_begin/contention_end tracepoints cover the slow path, they do not provide sufficient visibility into lock hold times. In particular, the lack of a release-side event makes it difficult to identify long lock holders and correlate them with waiters. As a result, distinguishing between short bursts of contention and pathological long hold times requires additional instrumentation.

This patch series adds dedicated tracepoint instrumentation to zone lock, following the existing mmap_lock tracing model.

The goal is to enable detailed holder/waiter analysis and lock hold time measurements without affecting the fast path when tracing is disabled.

The series is structured as follows:

1. Introduce zone lock wrappers.
2. Mechanically convert zone lock users to the wrappers.
3. Convert compaction to use the wrappers (requires minor restructuring of compact_lock_irqsave()).
4. Add zone lock tracepoints.

The tracepoints are added via lightweight inline helpers in the wrappers. When tracing is disabled, the fast path remains unchanged.

The compaction changes required abstracting compact_lock_irqsave() away from raw spinlock_t. I chose a small tagged struct to handle both zone and LRU locks uniformly. If there is a preferred alternative (e.g. splitting helpers or using a different abstraction), I would appreciate feedback.

Dmitry Ilvokhin (4):
mm: introduce zone lock wrappers
mm: convert zone lock users to wrappers
mm: convert compaction to zone lock wrappers
mm: add tracepoints for zone lock

 MAINTAINERS                      |   3 +
 include/linux/zone_lock.h        | 100 ++++++++++++++++++++++++++++
 include/trace/events/zone_lock.h |  64 ++++++++++++++++++
 mm/Makefile                      |   2 +-
 mm/compaction.c                  | 108 +++++++++++++++++++++++++------
 mm/memory_hotplug.c              |   9 +--
 mm/mm_init.c                     |   3 +-
 mm/page_alloc.c                  |  73 ++++++++++-----------
 mm/page_isolation.c              |  19 +++---
 mm/page_reporting.c              |  13 ++--
 mm/show_mem.c                    |   5 +-
 mm/vmscan.c                      |   5 +-
 mm/vmstat.c                      |   9 +--
 mm/zone_lock.c                   |  31 +++++++++
 14 files changed, 360 insertions(+), 84 deletions(-)
 create mode 100644 include/linux/zone_lock.h
 create mode 100644 include/trace/events/zone_lock.h
 create mode 100644 mm/zone_lock.c
Signature · 1 line
2.47.3
author
#1 lore raw

PATCH 1/4 mm: introduce zone lock wrappers

Add thin wrappers around zone lock acquire/release operations. This prepares the code for future tracepoint instrumentation without modifying individual call sites.

Centralizing zone lock operations behind wrappers allows future instrumentation or debugging hooks to be added without touching all users.

No functional change intended. The wrappers are introduced in preparation for subsequent patches and are not yet used.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 MAINTAINERS               |  1 +
 include/linux/zone_lock.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 include/linux/zone_lock.h
MAINTAINERS +1 -0
index b4088f7290be..680c9ae02d7e 100644
@@ -16498,6 +16498,7 @@ F:	include/linux/pgtable.h
 F:	include/linux/ptdump.h
 F:	include/linux/vmpressure.h
 F:	include/linux/vmstat.h
+F:	include/linux/zone_lock.h
 F:	kernel/fork.c
 F:	mm/Kconfig
 F:	mm/debug.c
include/linux/zone_lock.h new file +38 -0
new file mode 100644
index 000000000000..c531e26280e6
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ZONE_LOCK_H
+#define _LINUX_ZONE_LOCK_H
+
+#include <linux/mmzone.h>
+#include <linux/spinlock.h>
+
+static inline void zone_lock_init(struct zone *zone)
+{
+	spin_lock_init(&zone->lock);
+}
+
+#define zone_lock_irqsave(zone, flags)				\
+do {								\
+	spin_lock_irqsave(&(zone)->lock, flags);		\
+} while (0)
+
+#define zone_trylock_irqsave(zone, flags)			\
+({								\
+	spin_trylock_irqsave(&(zone)->lock, flags);		\
+})
+
+static inline void zone_unlock_irqrestore(struct zone *zone, unsigned long flags)
+{
+	spin_unlock_irqrestore(&zone->lock, flags);
+}
+
+static inline void zone_lock_irq(struct zone *zone)
+{
+	spin_lock_irq(&zone->lock);
+}
+
+static inline void zone_unlock_irq(struct zone *zone)
+{
+	spin_unlock_irq(&zone->lock);
+}
+
+#endif /* _LINUX_ZONE_LOCK_H */
Signature · 1 line
2.47.3
reviewer
#2 lore raw

Re: [PATCH 1/4] mm: introduce zone lock wrappers

Quoted 47 lines · Dmitry Ilvokhin

Add thin wrappers around zone lock acquire/release operations. This prepares the code for future tracepoint instrumentation without modifying individual call sites.

Centralizing zone lock operations behind wrappers allows future instrumentation or debugging hooks to be added without touching all users.

No functional change intended. The wrappers are introduced in preparation for subsequent patches and are not yet used.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 MAINTAINERS               |  1 +
 include/linux/zone_lock.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 include/linux/zone_lock.h
MAINTAINERS +1 -0
index b4088f7290be..680c9ae02d7e 100644
@@ -16498,6 +16498,7 @@ F:	include/linux/pgtable.h
 F:	include/linux/ptdump.h
 F:	include/linux/vmpressure.h
 F:	include/linux/vmstat.h
+F:	include/linux/zone_lock.h
 F:	kernel/fork.c
 F:	mm/Kconfig
 F:	mm/debug.c
include/linux/zone_lock.h new file +11 -0
new file mode 100644
index 000000000000..c531e26280e6
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ZONE_LOCK_H
+#define _LINUX_ZONE_LOCK_H
+
+#include <linux/mmzone.h>
+#include <linux/spinlock.h>
+
+static inline void zone_lock_init(struct zone *zone)
+{
+	spin_lock_init(&zone->lock);
+}

include/linux/zone_lock.h line 21

Quoted 10 lines
include/linux/zone_lock.h new file +10 -0
+
+#define zone_lock_irqsave(zone, flags)				\
+do {								\
+	spin_lock_irqsave(&(zone)->lock, flags);		\
+} while (0)
+
+#define zone_trylock_irqsave(zone, flags)			\
+({								\
+	spin_trylock_irqsave(&(zone)->lock, flags);		\
+})

Any reason you used macros for above two and inlined functions for remaining?

Quoted 20 lines
include/linux/zone_lock.h new file +17 -0
+
+static inline void zone_unlock_irqrestore(struct zone *zone, unsigned long flags)
+{
+	spin_unlock_irqrestore(&zone->lock, flags);
+}
+
+static inline void zone_lock_irq(struct zone *zone)
+{
+	spin_lock_irq(&zone->lock);
+}
+
+static inline void zone_unlock_irq(struct zone *zone)
+{
+	spin_unlock_irq(&zone->lock);
+}
+
+#endif /* _LINUX_ZONE_LOCK_H */
Signature · 1 line
2.47.3
author
#3 lore raw

Re: [PATCH 1/4] mm: introduce zone lock wrappers

Quoted 52 lines · Shakeel Butt
Quoted 51 lines · Dmitry Ilvokhin

Add thin wrappers around zone lock acquire/release operations. This prepares the code for future tracepoint instrumentation without modifying individual call sites.

Centralizing zone lock operations behind wrappers allows future instrumentation or debugging hooks to be added without touching all users.

No functional change intended. The wrappers are introduced in preparation for subsequent patches and are not yet used.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 MAINTAINERS               |  1 +
 include/linux/zone_lock.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 include/linux/zone_lock.h
MAINTAINERS +1 -0
index b4088f7290be..680c9ae02d7e 100644
@@ -16498,6 +16498,7 @@ F:	include/linux/pgtable.h
 F:	include/linux/ptdump.h
 F:	include/linux/vmpressure.h
 F:	include/linux/vmstat.h
+F:	include/linux/zone_lock.h
 F:	kernel/fork.c
 F:	mm/Kconfig
 F:	mm/debug.c
include/linux/zone_lock.h new file +15 -0
new file mode 100644
index 000000000000..c531e26280e6
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ZONE_LOCK_H
+#define _LINUX_ZONE_LOCK_H
+
+#include <linux/mmzone.h>
+#include <linux/spinlock.h>
+
+static inline void zone_lock_init(struct zone *zone)
+{
+	spin_lock_init(&zone->lock);
+}
+
+#define zone_lock_irqsave(zone, flags)				\
+do {								\
+	spin_lock_irqsave(&(zone)->lock, flags);		\
Quoted 9 lines

include/linux/zone_lock.h line 21

Quoted 6 lines
include/linux/zone_lock.h new file +6 -0
+} while (0)
+
+#define zone_trylock_irqsave(zone, flags)			\
+({								\
+	spin_trylock_irqsave(&(zone)->lock, flags);		\
+})

Any reason you used macros for above two and inlined functions for remaining?

The reason for using macros in those two cases is that they need to modify the flags variable passed by the caller, just like spin_lock_irqsave() and spin_trylock_irqsave() do. I followed the same convention here.

If we used normal inline functions instead, we would need to pass a pointer to flags, which would change the call sites and diverge from the existing *_irqsave() locking pattern.

There is also a difference between zone_lock_irqsave() and zone_trylock_irqsave() implementations: the former is implemented as a do { } while (0) macro since it does not return a value, while the latter uses a GCC extension in order to return the trylock result. This matches spin_lock_* convention as well.

Quoted 20 lines
Quoted 20 lines
include/linux/zone_lock.h new file +17 -0
+
+static inline void zone_unlock_irqrestore(struct zone *zone, unsigned long flags)
+{
+	spin_unlock_irqrestore(&zone->lock, flags);
+}
+
+static inline void zone_lock_irq(struct zone *zone)
+{
+	spin_lock_irq(&zone->lock);
+}
+
+static inline void zone_unlock_irq(struct zone *zone)
+{
+	spin_unlock_irq(&zone->lock);
+}
+
+#endif /* _LINUX_ZONE_LOCK_H */
Signature · 1 line
2.47.3
reviewer
#4 lore raw

Re: [PATCH 1/4] mm: introduce zone lock wrappers

Quoted 72 lines · Dmitry Ilvokhin
Quoted 52 lines · Shakeel Butt
Quoted 51 lines · Dmitry Ilvokhin

Add thin wrappers around zone lock acquire/release operations. This prepares the code for future tracepoint instrumentation without modifying individual call sites.

Centralizing zone lock operations behind wrappers allows future instrumentation or debugging hooks to be added without touching all users.

No functional change intended. The wrappers are introduced in preparation for subsequent patches and are not yet used.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 MAINTAINERS               |  1 +
 include/linux/zone_lock.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 include/linux/zone_lock.h
MAINTAINERS +1 -0
index b4088f7290be..680c9ae02d7e 100644
@@ -16498,6 +16498,7 @@ F:	include/linux/pgtable.h
 F:	include/linux/ptdump.h
 F:	include/linux/vmpressure.h
 F:	include/linux/vmstat.h
+F:	include/linux/zone_lock.h
 F:	kernel/fork.c
 F:	mm/Kconfig
 F:	mm/debug.c
include/linux/zone_lock.h new file +15 -0
new file mode 100644
index 000000000000..c531e26280e6
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ZONE_LOCK_H
+#define _LINUX_ZONE_LOCK_H
+
+#include <linux/mmzone.h>
+#include <linux/spinlock.h>
+
+static inline void zone_lock_init(struct zone *zone)
+{
+	spin_lock_init(&zone->lock);
+}
+
+#define zone_lock_irqsave(zone, flags)				\
+do {								\
+	spin_lock_irqsave(&(zone)->lock, flags);		\
Quoted 9 lines

include/linux/zone_lock.h line 21

Quoted 6 lines
include/linux/zone_lock.h new file +6 -0
+} while (0)
+
+#define zone_trylock_irqsave(zone, flags)			\
+({								\
+	spin_trylock_irqsave(&(zone)->lock, flags);		\
+})

Any reason you used macros for above two and inlined functions for remaining?

The reason for using macros in those two cases is that they need to modify the flags variable passed by the caller, just like spin_lock_irqsave() and spin_trylock_irqsave() do. I followed the same convention here.

If we used normal inline functions instead, we would need to pass a pointer to flags, which would change the call sites and diverge from the existing *_irqsave() locking pattern.

Quoted 6 lines

There is also a difference between zone_lock_irqsave() and zone_trylock_irqsave() implementations: the former is implemented as a do { } while (0) macro since it does not return a value, while the latter uses a GCC extension in order to return the trylock result. This matches spin_lock_* convention as well.

Cool, thanks for the explanation.

reviewer Acked-by on v1 1/4
#5 lore raw

Re: [PATCH 1/4] mm: introduce zone lock wrappers

In reply to Dmitry Ilvokhin
Quoted 4 lines · Dmitry Ilvokhin

Add thin wrappers around zone lock acquire/release operations. This prepares the code for future tracepoint instrumentation without modifying individual call sites.

Quoted 8 lines

Centralizing zone lock operations behind wrappers allows future instrumentation or debugging hooks to be added without touching all users.

No functional change intended. The wrappers are introduced in preparation for subsequent patches and are not yet used.

Signed-off-by Dmitry Ilvokhin <[email protected]>
Acked-by Shakeel Butt <[email protected]>
maintainer
#6 lore raw

Re: [PATCH 1/4] mm: introduce zone lock wrappers

In reply to Dmitry Ilvokhin
Quoted 34 lines · On Wed, 11 Feb 2026 15:22:13 +0000 Dmitry Ilvokhin <[email protected]> wrote:
include/linux/zone_lock.h new file +28 -0
new file mode 100644
index 000000000000..c531e26280e6
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ZONE_LOCK_H
+#define _LINUX_ZONE_LOCK_H
+
+#include <linux/mmzone.h>
+#include <linux/spinlock.h>
+
+static inline void zone_lock_init(struct zone *zone)
+{
+	spin_lock_init(&zone->lock);
+}
+
+#define zone_lock_irqsave(zone, flags)				\
+do {								\
+	spin_lock_irqsave(&(zone)->lock, flags);		\
+} while (0)
+
+#define zone_trylock_irqsave(zone, flags)			\
+({								\
+	spin_trylock_irqsave(&(zone)->lock, flags);		\
+})
+
+static inline void zone_unlock_irqrestore(struct zone *zone, unsigned long flags)
+{
+	spin_unlock_irqrestore(&zone->lock, flags);
+}
+
+static inline void zone_lock_irq(struct zone *zone)

include/linux/zone_lock.h line 38

Quoted 10 lines
include/linux/zone_lock.h new file +10 -0
+{
+	spin_lock_irq(&zone->lock);
+}
+
+static inline void zone_unlock_irq(struct zone *zone)
+{
+	spin_unlock_irq(&zone->lock);
+}
+
+#endif /* _LINUX_ZONE_LOCK_H */

Have you thought about adding guards as well. It could make the code simpler:

(Not tested)

#include <linux/cleanup.h>
[..]

DEFINE_LOCK_GUARD_1(zonelock_irqsave, struct zone *,

		    zone_lock_irqsave(_T->lock, _T->flags),
		    zone_unlock_irqrestore(_T->lock, _T->flags),
		    unsigned long flags)

DECLARE_LOCK_GUARD_1_ATTRS(zonelock_irqsave, __acquires(_T), __releases(*(struct zone ***)_T)) #define class_zonelock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(zonelock_irqsave, _T)

DEFINE_LOCK_GUARD_1(zonelock_irq, struct zone *,

		    zone_lock_irq(_T->lock),
		    zone_unlock_irq(_T->lock))

DECLARE_LOCK_GUARD_1_ATTRS(zonelock_irq, __acquires(_T), __releases(*(struct zone ***)_T)) #define class_zonelock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(zonelock_irq, _T)

Then you could even remove the "flags" variables from the C code, and some goto unlocks.

-- Steve

author
#7 lore raw

Re: [PATCH 1/4] mm: introduce zone lock wrappers

Quoted 63 lines · Steven Rostedt
Quoted 34 lines · On Wed, 11 Feb 2026 15:22:13 +0000 Dmitry Ilvokhin <[email protected]> wrote:
include/linux/zone_lock.h new file +28 -0
new file mode 100644
index 000000000000..c531e26280e6
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ZONE_LOCK_H
+#define _LINUX_ZONE_LOCK_H
+
+#include <linux/mmzone.h>
+#include <linux/spinlock.h>
+
+static inline void zone_lock_init(struct zone *zone)
+{
+	spin_lock_init(&zone->lock);
+}
+
+#define zone_lock_irqsave(zone, flags)				\
+do {								\
+	spin_lock_irqsave(&(zone)->lock, flags);		\
+} while (0)
+
+#define zone_trylock_irqsave(zone, flags)			\
+({								\
+	spin_trylock_irqsave(&(zone)->lock, flags);		\
+})
+
+static inline void zone_unlock_irqrestore(struct zone *zone, unsigned long flags)
+{
+	spin_unlock_irqrestore(&zone->lock, flags);
+}
+
+static inline void zone_lock_irq(struct zone *zone)

include/linux/zone_lock.h line 38

Quoted 10 lines
include/linux/zone_lock.h new file +10 -0
+{
+	spin_lock_irq(&zone->lock);
+}
+
+static inline void zone_unlock_irq(struct zone *zone)
+{
+	spin_unlock_irq(&zone->lock);
+}
+
+#endif /* _LINUX_ZONE_LOCK_H */

Have you thought about adding guards as well. It could make the code simpler:

(Not tested)

#include <linux/cleanup.h>
[..]

DEFINE_LOCK_GUARD_1(zonelock_irqsave, struct zone *,

		    zone_lock_irqsave(_T->lock, _T->flags),
		    zone_unlock_irqrestore(_T->lock, _T->flags),
		    unsigned long flags)

DECLARE_LOCK_GUARD_1_ATTRS(zonelock_irqsave, __acquires(_T), __releases(*(struct zone ***)_T)) #define class_zonelock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(zonelock_irqsave, _T)

Quoted 8 lines

DEFINE_LOCK_GUARD_1(zonelock_irq, struct zone *,

		    zone_lock_irq(_T->lock),
		    zone_unlock_irq(_T->lock))

DECLARE_LOCK_GUARD_1_ATTRS(zonelock_irq, __acquires(_T), __releases(*(struct zone ***)_T)) #define class_zonelock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(zonelock_irq, _T)

Then you could even remove the "flags" variables from the C code, and some goto unlocks.

Thanks, Steve.

I like the idea: guards could indeed simplify parts of the locking and reduce some of the explicit flags handling.

For this series, though, I'd prefer to keep the changes mostly mechanical and focused on introducing the wrappers and tracepoints. Converting to guards would make the transformation less mechanical and potentially harder to review.

I'd be happy to follow up with a separate patch to explore adding guards for zone locks and see whether we can simplify the existing logic further.

Quoted 1 line

-- Steve

author
#8 lore raw

PATCH 2/4 mm: convert zone lock users to wrappers

In reply to Dmitry Ilvokhin

Replace direct zone lock acquire/release operations with the newly introduced wrappers.

The changes are purely mechanical substitutions. No functional change intended. Locking semantics and ordering remain unchanged.

The compaction path is left unchanged for now and will be handled separately in the following patch due to additional non-trivial modifications.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 mm/memory_hotplug.c |  9 +++---
 mm/mm_init.c        |  3 +-
 mm/page_alloc.c     | 73 +++++++++++++++++++++++----------------------
 mm/page_isolation.c | 19 ++++++------
 mm/page_reporting.c | 13 ++++----
 mm/show_mem.c       |  5 ++--
 mm/vmscan.c         |  5 ++--
 mm/vmstat.c         |  9 +++---
 8 files changed, 72 insertions(+), 64 deletions(-)
mm/memory_hotplug.c +5 -4
index bc805029da51..cfc0103fa50e 100644
@@ -36,6 +36,7 @@
 #include <linux/rmap.h>
 #include <linux/module.h>
 #include <linux/node.h>
+#include <linux/zone_lock.h>
 
 #include <asm/tlbflush.h>
 

@@ -1190,9 +1191,9 @@ int online_pages(unsigned long pfn, unsigned long nr_pages,
 	 * Fixup the number of isolated pageblocks before marking the sections
 	 * onlining, such that undo_isolate_page_range() works correctly.
 	 */
-	spin_lock_irqsave(&zone->lock, flags);
+	zone_lock_irqsave(zone, flags);
 	zone->nr_isolate_pageblock += nr_pages / pageblock_nr_pages;
-	spin_unlock_irqrestore(&zone->lock, flags);
+	zone_unlock_irqrestore(zone, flags);
 
 	/*
 	 * If this zone is not populated, then it is not in zonelist.

@@ -2041,9 +2042,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
 	 * effectively stale; nobody should be touching them. Fixup the number
 	 * of isolated pageblocks, memory onlining will properly revert this.
 	 */
-	spin_lock_irqsave(&zone->lock, flags);
+	zone_lock_irqsave(zone, flags);
 	zone->nr_isolate_pageblock -= nr_pages / pageblock_nr_pages;
-	spin_unlock_irqrestore(&zone->lock, flags);
+	zone_unlock_irqrestore(zone, flags);
 
 	lru_cache_enable();
 	zone_pcp_enable(zone);
mm/mm_init.c +2 -1
index 1a29a719af58..426e5a0256f9 100644
@@ -32,6 +32,7 @@
 #include <linux/vmstat.h>
 #include <linux/kexec_handover.h>
 #include <linux/hugetlb.h>
+#include <linux/zone_lock.h>
 #include "internal.h"
 #include "slab.h"
 #include "shuffle.h"

@@ -1425,7 +1426,7 @@ static void __meminit zone_init_internals(struct zone *zone, enum zone_type idx,
 	zone_set_nid(zone, nid);
 	zone->name = zone_names[idx];
 	zone->zone_pgdat = NODE_DATA(nid);
-	spin_lock_init(&zone->lock);
+	zone_lock_init(zone);
 	zone_seqlock_init(zone);
 	zone_pcp_init(zone);
 }
mm/page_alloc.c +37 -36

266 lines not on this page · raw message

mm/page_isolation.c +10 -9
index c48ff5c00244..56a272f38b66 100644
@@ -10,6 +10,7 @@
 #include <linux/hugetlb.h>
 #include <linux/page_owner.h>
 #include <linux/migrate.h>
+#include <linux/zone_lock.h>
 #include "internal.h"
 
 #define CREATE_TRACE_POINTS

@@ -173,7 +174,7 @@ static int set_migratetype_isolate(struct page *page, enum pb_isolate_mode mode,
 	if (PageUnaccepted(page))
 		accept_page(page);
 
-	spin_lock_irqsave(&zone->lock, flags);
+	zone_lock_irqsave(zone, flags);
 
 	/*
 	 * We assume the caller intended to SET migrate type to isolate.

@@ -181,7 +182,7 @@ static int set_migratetype_isolate(struct page *page, enum pb_isolate_mode mode,
 	 * set it before us.
 	 */
 	if (is_migrate_isolate_page(page)) {
-		spin_unlock_irqrestore(&zone->lock, flags);
+		zone_unlock_irqrestore(zone, flags);
 		return -EBUSY;
 	}
 

@@ -200,15 +201,15 @@ static int set_migratetype_isolate(struct page *page, enum pb_isolate_mode mode,
 			mode);
 	if (!unmovable) {
 		if (!pageblock_isolate_and_move_free_pages(zone, page)) {
-			spin_unlock_irqrestore(&zone->lock, flags);
+			zone_unlock_irqrestore(zone, flags);
 			return -EBUSY;
 		}
 		zone->nr_isolate_pageblock++;
-		spin_unlock_irqrestore(&zone->lock, flags);
+		zone_unlock_irqrestore(zone, flags);
 		return 0;
 	}
 
-	spin_unlock_irqrestore(&zone->lock, flags);
+	zone_unlock_irqrestore(zone, flags);
 	if (mode == PB_ISOLATE_MODE_MEM_OFFLINE) {
 		/*
 		 * printk() with zone->lock held will likely trigger a

@@ -229,7 +230,7 @@ static void unset_migratetype_isolate(struct page *page)
 	struct page *buddy;
 
 	zone = page_zone(page);
-	spin_lock_irqsave(&zone->lock, flags);
+	zone_lock_irqsave(zone, flags);
 	if (!is_migrate_isolate_page(page))
 		goto out;
 

@@ -280,7 +281,7 @@ static void unset_migratetype_isolate(struct page *page)
 	}
 	zone->nr_isolate_pageblock--;
 out:
-	spin_unlock_irqrestore(&zone->lock, flags);
+	zone_unlock_irqrestore(zone, flags);
 }
 
 static inline struct page *

@@ -641,9 +642,9 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn,
 
 	/* Check all pages are free or marked as ISOLATED */
 	zone = page_zone(page);
-	spin_lock_irqsave(&zone->lock, flags);
+	zone_lock_irqsave(zone, flags);
 	pfn = __test_page_isolated_in_pageblock(start_pfn, end_pfn, mode);
-	spin_unlock_irqrestore(&zone->lock, flags);
+	zone_unlock_irqrestore(zone, flags);
 
 	ret = pfn < end_pfn ? -EBUSY : 0;
mm/page_reporting.c +7 -6
index 8a03effda749..ac2ac8fd0487 100644
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/scatterlist.h>
+#include <linux/zone_lock.h>
 
 #include "page_reporting.h"
 #include "internal.h"

@@ -161,7 +162,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
 	if (list_empty(list))
 		return err;
 
-	spin_lock_irq(&zone->lock);
+	zone_lock_irq(zone);
 
 	/*
 	 * Limit how many calls we will be making to the page reporting

@@ -219,7 +220,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
 			list_rotate_to_front(&page->lru, list);
 
 		/* release lock before waiting on report processing */
-		spin_unlock_irq(&zone->lock);
+		zone_unlock_irq(zone);
 
 		/* begin processing pages in local list */
 		err = prdev->report(prdev, sgl, PAGE_REPORTING_CAPACITY);

@@ -231,7 +232,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
 		budget--;
 
 		/* reacquire zone lock and resume processing */
-		spin_lock_irq(&zone->lock);
+		zone_lock_irq(zone);
 
 		/* flush reported pages from the sg list */
 		page_reporting_drain(prdev, sgl, PAGE_REPORTING_CAPACITY, !err);

@@ -251,7 +252,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
 	if (!list_entry_is_head(next, list, lru) && !list_is_first(&next->lru, list))
 		list_rotate_to_front(&next->lru, list);
 
-	spin_unlock_irq(&zone->lock);
+	zone_unlock_irq(zone);
 
 	return err;
 }

@@ -296,9 +297,9 @@ page_reporting_process_zone(struct page_reporting_dev_info *prdev,
 		err = prdev->report(prdev, sgl, leftover);
 
 		/* flush any remaining pages out from the last report */
-		spin_lock_irq(&zone->lock);
+		zone_lock_irq(zone);
 		page_reporting_drain(prdev, sgl, leftover, !err);
-		spin_unlock_irq(&zone->lock);
+		zone_unlock_irq(zone);
 	}
 
 	return err;
mm/show_mem.c +3 -2
index 24078ac3e6bc..245beca127af 100644
@@ -14,6 +14,7 @@
 #include <linux/mmzone.h>
 #include <linux/swap.h>
 #include <linux/vmstat.h>
+#include <linux/zone_lock.h>
 
 #include "internal.h"
 #include "swap.h"

@@ -363,7 +364,7 @@ static void show_free_areas(unsigned int filter, nodemask_t *nodemask, int max_z
 		show_node(zone);
 		printk(KERN_CONT "%s: ", zone->name);
 
-		spin_lock_irqsave(&zone->lock, flags);
+		zone_lock_irqsave(zone, flags);
 		for (order = 0; order < NR_PAGE_ORDERS; order++) {
 			struct free_area *area = &zone->free_area[order];
 			int type;

@@ -377,7 +378,7 @@ static void show_free_areas(unsigned int filter, nodemask_t *nodemask, int max_z
 					types[order] |= 1 << type;
 			}
 		}
-		spin_unlock_irqrestore(&zone->lock, flags);
+		zone_unlock_irqrestore(zone, flags);
 		for (order = 0; order < NR_PAGE_ORDERS; order++) {
 			printk(KERN_CONT "%lu*%lukB ",
 			       nr[order], K(1UL) << order);
mm/vmscan.c +3 -2
index 973ffb9813ea..9fe5c41e0e0a 100644
@@ -58,6 +58,7 @@
 #include <linux/random.h>
 #include <linux/mmu_notifier.h>
 #include <linux/parser.h>
+#include <linux/zone_lock.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>

@@ -7129,9 +7130,9 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int highest_zoneidx)
 
 			/* Increments are under the zone lock */
 			zone = pgdat->node_zones + i;
-			spin_lock_irqsave(&zone->lock, flags);
+			zone_lock_irqsave(zone, flags);
 			zone->watermark_boost -= min(zone->watermark_boost, zone_boosts[i]);
-			spin_unlock_irqrestore(&zone->lock, flags);
+			zone_unlock_irqrestore(zone, flags);
 		}
 
 		/*
mm/vmstat.c +5 -4
index 99270713e0c1..06b27255a626 100644
@@ -28,6 +28,7 @@
 #include <linux/mm_inline.h>
 #include <linux/page_owner.h>
 #include <linux/sched/isolation.h>
+#include <linux/zone_lock.h>
 
 #include "internal.h"
 

@@ -1535,10 +1536,10 @@ static void walk_zones_in_node(struct seq_file *m, pg_data_t *pgdat,
 			continue;
 
 		if (!nolock)
-			spin_lock_irqsave(&zone->lock, flags);
+			zone_lock_irqsave(zone, flags);
 		print(m, pgdat, zone);
 		if (!nolock)
-			spin_unlock_irqrestore(&zone->lock, flags);
+			zone_unlock_irqrestore(zone, flags);
 	}
 }
 #endif

@@ -1603,9 +1604,9 @@ static void pagetypeinfo_showfree_print(struct seq_file *m,
 				}
 			}
 			seq_printf(m, "%s%6lu ", overflow ? ">" : "", freecount);
-			spin_unlock_irq(&zone->lock);
+			zone_unlock_irq(zone);
 			cond_resched();
-			spin_lock_irq(&zone->lock);
+			zone_lock_irq(zone);
 		}
 		seq_putc(m, '\n');
 	}
Signature · 1 line
2.47.3
reviewer Acked-by on v1 2/4
#9 lore raw

Re: [PATCH 2/4] mm: convert zone lock users to wrappers

Quoted 11 lines · Dmitry Ilvokhin

Replace direct zone lock acquire/release operations with the newly introduced wrappers.

The changes are purely mechanical substitutions. No functional change intended. Locking semantics and ordering remain unchanged.

The compaction path is left unchanged for now and will be handled separately in the following patch due to additional non-trivial modifications.

Signed-off-by Dmitry Ilvokhin <[email protected]>
Acked-by Shakeel Butt <[email protected]>
author
#10 lore raw

PATCH 3/4 mm: convert compaction to zone lock wrappers

In reply to Dmitry Ilvokhin

Compaction uses compact_lock_irqsave(), which currently operates on a raw spinlock_t pointer so that it can be used for both zone->lock and lru_lock. Since zone lock operations are now wrapped, compact_lock_irqsave() can no longer operate directly on a spinlock_t when the lock belongs to a zone.

Introduce struct compact_lock to abstract the underlying lock type. The structure carries a lock type enum and a union holding either a zone pointer or a raw spinlock_t pointer, and dispatches to the appropriate lock/unlock helper.

No functional change intended.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 mm/compaction.c | 108 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 89 insertions(+), 19 deletions(-)
mm/compaction.c +89 -19
index 1e8f8eca318c..1b000d2b95b2 100644
@@ -24,6 +24,7 @@
 #include <linux/page_owner.h>
 #include <linux/psi.h>
 #include <linux/cpuset.h>
+#include <linux/zone_lock.h>
 #include "internal.h"
 
 #ifdef CONFIG_COMPACTION

@@ -493,6 +494,65 @@ static bool test_and_set_skip(struct compact_control *cc, struct page *page)
 }
 #endif /* CONFIG_COMPACTION */
 
+enum compact_lock_type {
+	COMPACT_LOCK_ZONE,
+	COMPACT_LOCK_RAW_SPINLOCK,
+};
+
+struct compact_lock {
+	enum compact_lock_type type;
+	union {
+		struct zone *zone;
+		spinlock_t *lock; /* Reference to lru lock */
+	};
+};
+
+static bool compact_do_zone_trylock_irqsave(struct zone *zone,
+					    unsigned long *flags)
+{
+	return zone_trylock_irqsave(zone, *flags);
+}
+
+static bool compact_do_raw_trylock_irqsave(spinlock_t *lock,
+					   unsigned long *flags)
+{
+	return spin_trylock_irqsave(lock, *flags);
+}
+
+static bool compact_do_trylock_irqsave(struct compact_lock lock,
+				       unsigned long *flags)
+{
+	if (lock.type == COMPACT_LOCK_ZONE)
+		return compact_do_zone_trylock_irqsave(lock.zone, flags);
+
+	return compact_do_raw_trylock_irqsave(lock.lock, flags);
+}
+
+static void compact_do_zone_lock_irqsave(struct zone *zone,
+					 unsigned long *flags)
+__acquires(zone->lock)
+{
+	zone_lock_irqsave(zone, *flags);
+}
+
+static void compact_do_raw_lock_irqsave(spinlock_t *lock,
+					unsigned long *flags)
+__acquires(lock)
+{
+	spin_lock_irqsave(lock, *flags);
+}
+
+static void compact_do_lock_irqsave(struct compact_lock lock,
+				    unsigned long *flags)
+{
+	if (lock.type == COMPACT_LOCK_ZONE) {
+		compact_do_zone_lock_irqsave(lock.zone, flags);
+		return;
+	}
+
+	return compact_do_raw_lock_irqsave(lock.lock, flags);
+}
+
 /*
  * Compaction requires the taking of some coarse locks that are potentially
  * very heavily contended. For async compaction, trylock and record if the

@@ -502,19 +562,19 @@ static bool test_and_set_skip(struct compact_control *cc, struct page *page)
  *
  * Always returns true which makes it easier to track lock state in callers.
  */
-static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
-						struct compact_control *cc)
-	__acquires(lock)
+static bool compact_lock_irqsave(struct compact_lock lock,
+				 unsigned long *flags,
+				 struct compact_control *cc)
 {
 	/* Track if the lock is contended in async mode */
 	if (cc->mode == MIGRATE_ASYNC && !cc->contended) {
-		if (spin_trylock_irqsave(lock, *flags))
+		if (compact_do_trylock_irqsave(lock, flags))
 			return true;
 
 		cc->contended = true;
 	}
 
-	spin_lock_irqsave(lock, *flags);
+	compact_do_lock_irqsave(lock, flags);
 	return true;
 }
 

@@ -530,11 +590,13 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
  * Returns true if compaction should abort due to fatal signal pending.
  * Returns false when compaction can continue.
  */
-static bool compact_unlock_should_abort(spinlock_t *lock,
-		unsigned long flags, bool *locked, struct compact_control *cc)
+static bool compact_unlock_should_abort(struct zone *zone,
+					unsigned long flags,
+					bool *locked,
+					struct compact_control *cc)
 {
 	if (*locked) {
-		spin_unlock_irqrestore(lock, flags);
+		zone_unlock_irqrestore(zone, flags);
 		*locked = false;
 	}
 

@@ -582,9 +644,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 		 * contention, to give chance to IRQs. Abort if fatal signal
 		 * pending.
 		 */
-		if (!(blockpfn % COMPACT_CLUSTER_MAX)
-		    && compact_unlock_should_abort(&cc->zone->lock, flags,
-								&locked, cc))
+		if (!(blockpfn % COMPACT_CLUSTER_MAX) &&
+		    compact_unlock_should_abort(cc->zone, flags, &locked, cc))
 			break;
 
 		nr_scanned++;

@@ -613,8 +674,12 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 
 		/* If we already hold the lock, we can skip some rechecking. */
 		if (!locked) {
-			locked = compact_lock_irqsave(&cc->zone->lock,
-								&flags, cc);
+			struct compact_lock zol = {
+				.type = COMPACT_LOCK_ZONE,
+				.zone = cc->zone,
+			};
+
+			locked = compact_lock_irqsave(zol, &flags, cc);
 
 			/* Recheck this is a buddy page under lock */
 			if (!PageBuddy(page))

@@ -649,7 +714,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 	}
 
 	if (locked)
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 
 	/*
 	 * Be careful to not go outside of the pageblock.

@@ -1157,10 +1222,15 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* If we already hold the lock, we can skip some rechecking */
 		if (lruvec != locked) {
+			struct compact_lock zol = {
+				.type = COMPACT_LOCK_RAW_SPINLOCK,
+				.lock = &lruvec->lru_lock,
+			};
+
 			if (locked)
 				unlock_page_lruvec_irqrestore(locked, flags);
 
-			compact_lock_irqsave(&lruvec->lru_lock, &flags, cc);
+			compact_lock_irqsave(zol, &flags, cc);
 			locked = lruvec;
 
 			lruvec_memcg_debug(lruvec, folio);

@@ -1555,7 +1625,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 		if (!area->nr_free)
 			continue;
 
-		spin_lock_irqsave(&cc->zone->lock, flags);
+		zone_lock_irqsave(cc->zone, flags);
 		freelist = &area->free_list[MIGRATE_MOVABLE];
 		list_for_each_entry_reverse(freepage, freelist, buddy_list) {
 			unsigned long pfn;

@@ -1614,7 +1684,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 			}
 		}
 
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 
 		/* Skip fast search if enough freepages isolated */
 		if (cc->nr_freepages >= cc->nr_migratepages)

@@ -1988,7 +2058,7 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 		if (!area->nr_free)
 			continue;
 
-		spin_lock_irqsave(&cc->zone->lock, flags);
+		zone_lock_irqsave(cc->zone, flags);
 		freelist = &area->free_list[MIGRATE_MOVABLE];
 		list_for_each_entry(freepage, freelist, buddy_list) {
 			unsigned long free_pfn;

@@ -2021,7 +2091,7 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 				break;
 			}
 		}
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 	}
 
 	cc->total_migrate_scanned += nr_scanned;
Signature · 1 line
2.47.3
#11 lore raw

Re: [PATCH 3/4] mm: convert compaction to zone lock wrappers

Quoted 57 lines · Dmitry Ilvokhin

Compaction uses compact_lock_irqsave(), which currently operates on a raw spinlock_t pointer so that it can be used for both zone->lock and lru_lock. Since zone lock operations are now wrapped, compact_lock_irqsave() can no longer operate directly on a spinlock_t when the lock belongs to a zone.

Introduce struct compact_lock to abstract the underlying lock type. The structure carries a lock type enum and a union holding either a zone pointer or a raw spinlock_t pointer, and dispatches to the appropriate lock/unlock helper.

No functional change intended.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 mm/compaction.c | 108 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 89 insertions(+), 19 deletions(-)
mm/compaction.c +24 -0
index 1e8f8eca318c..1b000d2b95b2 100644
@@ -24,6 +24,7 @@
 #include <linux/page_owner.h>
 #include <linux/psi.h>
 #include <linux/cpuset.h>
+#include <linux/zone_lock.h>
 #include "internal.h"
 
 #ifdef CONFIG_COMPACTION

@@ -493,6 +494,65 @@ static bool test_and_set_skip(struct compact_control *cc, struct page *page)
 }
 #endif /* CONFIG_COMPACTION */
 
+enum compact_lock_type {
+	COMPACT_LOCK_ZONE,
+	COMPACT_LOCK_RAW_SPINLOCK,
+};
+
+struct compact_lock {
+	enum compact_lock_type type;
+	union {
+		struct zone *zone;
+		spinlock_t *lock; /* Reference to lru lock */
+	};
+};
+
+static bool compact_do_zone_trylock_irqsave(struct zone *zone,
+					    unsigned long *flags)
+{
+	return zone_trylock_irqsave(zone, *flags);
+}
+
+static bool compact_do_raw_trylock_irqsave(spinlock_t *lock,
+					   unsigned long *flags)
+{
+	return spin_trylock_irqsave(lock, *flags);

mm/compaction.c static bool test_and_set_skip(struct compact_control *cc, struct page *page) · line 529

Quoted 10 lines
mm/compaction.c +10 -0
+}
+
+static bool compact_do_trylock_irqsave(struct compact_lock lock,
+				       unsigned long *flags)
+{
+	if (lock.type == COMPACT_LOCK_ZONE)
+		return compact_do_zone_trylock_irqsave(lock.zone, flags);
+
+	return compact_do_raw_trylock_irqsave(lock.lock, flags);
+}

Nit: You could remove the helpers above and just do the calls directly in this function, though it would remove the parity with the compact helpers. compact_do_lock_irqsave() helpers can stay since they have the __acquires() annotations.

Quoted 14 lines
mm/compaction.c +14 -0
+
+static void compact_do_zone_lock_irqsave(struct zone *zone,
+					 unsigned long *flags)
+__acquires(zone->lock)
+{
+	zone_lock_irqsave(zone, *flags);
+}
+
+static void compact_do_raw_lock_irqsave(spinlock_t *lock,
+					unsigned long *flags)
+__acquires(lock)
+{
+	spin_lock_irqsave(lock, *flags);
+}

mm/compaction.c static bool test_and_set_skip(struct compact_control *cc, struct page *page) · line 553

Quoted 10 lines
mm/compaction.c +10 -0
+
+static void compact_do_lock_irqsave(struct compact_lock lock,
+				    unsigned long *flags)
+{
+	if (lock.type == COMPACT_LOCK_ZONE) {
+		compact_do_zone_lock_irqsave(lock.zone, flags);
+		return;
+	}
+
+	return compact_do_raw_lock_irqsave(lock.lock, flags);

You don't need the return statement here (and you shouldn't be returning a value at all).

It may be cleaner to just do an if-else statement here instead.

Quoted 34 lines
mm/compaction.c +7 -5
+}
+
 /*
  * Compaction requires the taking of some coarse locks that are potentially
  * very heavily contended. For async compaction, trylock and record if the

@@ -502,19 +562,19 @@ static bool test_and_set_skip(struct compact_control *cc, struct page *page)
  *
  * Always returns true which makes it easier to track lock state in callers.
  */
-static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
-						struct compact_control *cc)
-	__acquires(lock)
+static bool compact_lock_irqsave(struct compact_lock lock,
+				 unsigned long *flags,
+				 struct compact_control *cc)
 {
 	/* Track if the lock is contended in async mode */
 	if (cc->mode == MIGRATE_ASYNC && !cc->contended) {
-		if (spin_trylock_irqsave(lock, *flags))
+		if (compact_do_trylock_irqsave(lock, flags))
 			return true;
 
 		cc->contended = true;
 	}
 
-	spin_lock_irqsave(lock, *flags);
+	compact_do_lock_irqsave(lock, flags);
 	return true;
 }
 

@@ -530,11 +590,13 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
  * Returns true if compaction should abort due to fatal signal pending.
  * Returns false when compaction can continue.
  */

mm/compaction.c static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags, · line 599

Quoted 10 lines
mm/compaction.c +5 -3
-static bool compact_unlock_should_abort(spinlock_t *lock,
-		unsigned long flags, bool *locked, struct compact_control *cc)
+static bool compact_unlock_should_abort(struct zone *zone,
+					unsigned long flags,
+					bool *locked,
+					struct compact_control *cc)
 {
 	if (*locked) {
-		spin_unlock_irqrestore(lock, flags);
+		zone_unlock_irqrestore(zone, flags);

I would move this (and other wrapper changes below that don't use compact_*) to the last patch. I understand you didn't change it due to location but I would argue it isn't really relevant to what's being added in this patch and fits better in the last.

Quoted 92 lines
mm/compaction.c +19 -11
 		*locked = false;
 	}
 

@@ -582,9 +644,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 		 * contention, to give chance to IRQs. Abort if fatal signal
 		 * pending.
 		 */
-		if (!(blockpfn % COMPACT_CLUSTER_MAX)
-		    && compact_unlock_should_abort(&cc->zone->lock, flags,
-								&locked, cc))
+		if (!(blockpfn % COMPACT_CLUSTER_MAX) &&
+		    compact_unlock_should_abort(cc->zone, flags, &locked, cc))
 			break;
 
 		nr_scanned++;

@@ -613,8 +674,12 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 
 		/* If we already hold the lock, we can skip some rechecking. */
 		if (!locked) {
-			locked = compact_lock_irqsave(&cc->zone->lock,
-								&flags, cc);
+			struct compact_lock zol = {
+				.type = COMPACT_LOCK_ZONE,
+				.zone = cc->zone,
+			};
+
+			locked = compact_lock_irqsave(zol, &flags, cc);
 
 			/* Recheck this is a buddy page under lock */
 			if (!PageBuddy(page))

@@ -649,7 +714,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 	}
 
 	if (locked)
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 
 	/*
 	 * Be careful to not go outside of the pageblock.

@@ -1157,10 +1222,15 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* If we already hold the lock, we can skip some rechecking */
 		if (lruvec != locked) {
+			struct compact_lock zol = {
+				.type = COMPACT_LOCK_RAW_SPINLOCK,
+				.lock = &lruvec->lru_lock,
+			};
+
 			if (locked)
 				unlock_page_lruvec_irqrestore(locked, flags);
 
-			compact_lock_irqsave(&lruvec->lru_lock, &flags, cc);
+			compact_lock_irqsave(zol, &flags, cc);
 			locked = lruvec;
 
 			lruvec_memcg_debug(lruvec, folio);

@@ -1555,7 +1625,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 		if (!area->nr_free)
 			continue;
 
-		spin_lock_irqsave(&cc->zone->lock, flags);
+		zone_lock_irqsave(cc->zone, flags);
 		freelist = &area->free_list[MIGRATE_MOVABLE];
 		list_for_each_entry_reverse(freepage, freelist, buddy_list) {
 			unsigned long pfn;

@@ -1614,7 +1684,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 			}
 		}
 
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 
 		/* Skip fast search if enough freepages isolated */
 		if (cc->nr_freepages >= cc->nr_migratepages)

@@ -1988,7 +2058,7 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 		if (!area->nr_free)
 			continue;
 
-		spin_lock_irqsave(&cc->zone->lock, flags);
+		zone_lock_irqsave(cc->zone, flags);
 		freelist = &area->free_list[MIGRATE_MOVABLE];
 		list_for_each_entry(freepage, freelist, buddy_list) {
 			unsigned long free_pfn;

@@ -2021,7 +2091,7 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 				break;
 			}
 		}
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 	}
 
 	cc->total_migrate_scanned += nr_scanned;
author
#12 lore raw

Re: [PATCH 3/4] mm: convert compaction to zone lock wrappers

Quoted 64 lines · Benjamin
Quoted 63 lines · Dmitry Ilvokhin

Compaction uses compact_lock_irqsave(), which currently operates on a raw spinlock_t pointer so that it can be used for both zone->lock and lru_lock. Since zone lock operations are now wrapped, compact_lock_irqsave() can no longer operate directly on a spinlock_t when the lock belongs to a zone.

Introduce struct compact_lock to abstract the underlying lock type. The structure carries a lock type enum and a union holding either a zone pointer or a raw spinlock_t pointer, and dispatches to the appropriate lock/unlock helper.

No functional change intended.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 mm/compaction.c | 108 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 89 insertions(+), 19 deletions(-)
mm/compaction.c +30 -0
index 1e8f8eca318c..1b000d2b95b2 100644
@@ -24,6 +24,7 @@
 #include <linux/page_owner.h>
 #include <linux/psi.h>
 #include <linux/cpuset.h>
+#include <linux/zone_lock.h>
 #include "internal.h"
 
 #ifdef CONFIG_COMPACTION

@@ -493,6 +494,65 @@ static bool test_and_set_skip(struct compact_control *cc, struct page *page)
 }
 #endif /* CONFIG_COMPACTION */
 
+enum compact_lock_type {
+	COMPACT_LOCK_ZONE,
+	COMPACT_LOCK_RAW_SPINLOCK,
+};
+
+struct compact_lock {
+	enum compact_lock_type type;
+	union {
+		struct zone *zone;
+		spinlock_t *lock; /* Reference to lru lock */
+	};
+};
+
+static bool compact_do_zone_trylock_irqsave(struct zone *zone,
+					    unsigned long *flags)
+{
+	return zone_trylock_irqsave(zone, *flags);
+}
+
+static bool compact_do_raw_trylock_irqsave(spinlock_t *lock,
+					   unsigned long *flags)
+{
+	return spin_trylock_irqsave(lock, *flags);
+}
+
+static bool compact_do_trylock_irqsave(struct compact_lock lock,
+				       unsigned long *flags)
+{
+	if (lock.type == COMPACT_LOCK_ZONE)
Quoted 8 lines

mm/compaction.c static bool test_and_set_skip(struct compact_control *cc, struct page *page) · line 529

Quoted 4 lines
mm/compaction.c +4 -0
+		return compact_do_zone_trylock_irqsave(lock.zone, flags);
+
+	return compact_do_raw_trylock_irqsave(lock.lock, flags);
+}

Nit: You could remove the helpers above and just do the calls directly in this function, though it would remove the parity with the compact helpers. compact_do_lock_irqsave() helpers can stay since they have the __acquires() annotations.

Yes, I agree, there is no much value in this wrappers, will remove them, thanks!

Quoted 18 lines
Quoted 18 lines
mm/compaction.c +18 -0
+
+static void compact_do_zone_lock_irqsave(struct zone *zone,
+					 unsigned long *flags)
+__acquires(zone->lock)
+{
+	zone_lock_irqsave(zone, *flags);
+}
+
+static void compact_do_raw_lock_irqsave(spinlock_t *lock,
+					unsigned long *flags)
+__acquires(lock)
+{
+	spin_lock_irqsave(lock, *flags);
+}
+
+static void compact_do_lock_irqsave(struct compact_lock lock,
+				    unsigned long *flags)
+{
Quoted 8 lines

mm/compaction.c static bool test_and_set_skip(struct compact_control *cc, struct page *page) · line 553

Quoted 6 lines
mm/compaction.c +6 -0
+	if (lock.type == COMPACT_LOCK_ZONE) {
+		compact_do_zone_lock_irqsave(lock.zone, flags);
+		return;
+	}
+
+	return compact_do_raw_lock_irqsave(lock.lock, flags);

You don't need the return statement here (and you shouldn't be returning a value at all).

Yes, agree, will fix in v2.

Quoted 43 lines

It may be cleaner to just do an if-else statement here instead.

Quoted 40 lines
mm/compaction.c +11 -7
+}
+
 /*
  * Compaction requires the taking of some coarse locks that are potentially
  * very heavily contended. For async compaction, trylock and record if the

@@ -502,19 +562,19 @@ static bool test_and_set_skip(struct compact_control *cc, struct page *page)
  *
  * Always returns true which makes it easier to track lock state in callers.
  */
-static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
-						struct compact_control *cc)
-	__acquires(lock)
+static bool compact_lock_irqsave(struct compact_lock lock,
+				 unsigned long *flags,
+				 struct compact_control *cc)
 {
 	/* Track if the lock is contended in async mode */
 	if (cc->mode == MIGRATE_ASYNC && !cc->contended) {
-		if (spin_trylock_irqsave(lock, *flags))
+		if (compact_do_trylock_irqsave(lock, flags))
 			return true;
 
 		cc->contended = true;
 	}
 
-	spin_lock_irqsave(lock, *flags);
+	compact_do_lock_irqsave(lock, flags);
 	return true;
 }
 

@@ -530,11 +590,13 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
  * Returns true if compaction should abort due to fatal signal pending.
  * Returns false when compaction can continue.
  */
-static bool compact_unlock_should_abort(spinlock_t *lock,
-		unsigned long flags, bool *locked, struct compact_control *cc)
+static bool compact_unlock_should_abort(struct zone *zone,
+					unsigned long flags,
+					bool *locked,
+					struct compact_control *cc)
Quoted 8 lines

mm/compaction.c static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags, · line 599

Quoted 4 lines
mm/compaction.c +1 -1
 {
 	if (*locked) {
-		spin_unlock_irqrestore(lock, flags);
+		zone_unlock_irqrestore(zone, flags);

I would move this (and other wrapper changes below that don't use compact_*) to the last patch. I understand you didn't change it due to location but I would argue it isn't really relevant to what's being added in this patch and fits better in the last.

Thanks for the suggestion. Totally makes sense to me, will do in v2 as well.

Quoted 94 lines
Quoted 93 lines
mm/compaction.c +19 -11
 		*locked = false;
 	}
 

@@ -582,9 +644,8 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 		 * contention, to give chance to IRQs. Abort if fatal signal
 		 * pending.
 		 */
-		if (!(blockpfn % COMPACT_CLUSTER_MAX)
-		    && compact_unlock_should_abort(&cc->zone->lock, flags,
-								&locked, cc))
+		if (!(blockpfn % COMPACT_CLUSTER_MAX) &&
+		    compact_unlock_should_abort(cc->zone, flags, &locked, cc))
 			break;
 
 		nr_scanned++;

@@ -613,8 +674,12 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 
 		/* If we already hold the lock, we can skip some rechecking. */
 		if (!locked) {
-			locked = compact_lock_irqsave(&cc->zone->lock,
-								&flags, cc);
+			struct compact_lock zol = {
+				.type = COMPACT_LOCK_ZONE,
+				.zone = cc->zone,
+			};
+
+			locked = compact_lock_irqsave(zol, &flags, cc);
 
 			/* Recheck this is a buddy page under lock */
 			if (!PageBuddy(page))

@@ -649,7 +714,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 	}
 
 	if (locked)
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 
 	/*
 	 * Be careful to not go outside of the pageblock.

@@ -1157,10 +1222,15 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* If we already hold the lock, we can skip some rechecking */
 		if (lruvec != locked) {
+			struct compact_lock zol = {
+				.type = COMPACT_LOCK_RAW_SPINLOCK,
+				.lock = &lruvec->lru_lock,
+			};
+
 			if (locked)
 				unlock_page_lruvec_irqrestore(locked, flags);
 
-			compact_lock_irqsave(&lruvec->lru_lock, &flags, cc);
+			compact_lock_irqsave(zol, &flags, cc);
 			locked = lruvec;
 
 			lruvec_memcg_debug(lruvec, folio);

@@ -1555,7 +1625,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 		if (!area->nr_free)
 			continue;
 
-		spin_lock_irqsave(&cc->zone->lock, flags);
+		zone_lock_irqsave(cc->zone, flags);
 		freelist = &area->free_list[MIGRATE_MOVABLE];
 		list_for_each_entry_reverse(freepage, freelist, buddy_list) {
 			unsigned long pfn;

@@ -1614,7 +1684,7 @@ static void fast_isolate_freepages(struct compact_control *cc)
 			}
 		}
 
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 
 		/* Skip fast search if enough freepages isolated */
 		if (cc->nr_freepages >= cc->nr_migratepages)

@@ -1988,7 +2058,7 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 		if (!area->nr_free)
 			continue;
 
-		spin_lock_irqsave(&cc->zone->lock, flags);
+		zone_lock_irqsave(cc->zone, flags);
 		freelist = &area->free_list[MIGRATE_MOVABLE];
 		list_for_each_entry(freepage, freelist, buddy_list) {
 			unsigned long free_pfn;

@@ -2021,7 +2091,7 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
 				break;
 			}
 		}
-		spin_unlock_irqrestore(&cc->zone->lock, flags);
+		zone_unlock_irqrestore(cc->zone, flags);
 	}
 
 	cc->total_migrate_scanned += nr_scanned;
author
#13 lore raw

PATCH 4/4 mm: add tracepoints for zone lock

In reply to Dmitry Ilvokhin

Add tracepoint instrumentation to zone lock acquire/release operations via the previously introduced wrappers.

The implementation follows the mmap_lock tracepoint pattern: a lightweight inline helper checks whether the tracepoint is enabled and calls into an out-of-line helper when tracing is active. When CONFIG_TRACING is disabled, helpers compile to empty inline stubs.

The fast path is unaffected when tracing is disabled.

Signed-off-by Dmitry Ilvokhin <[email protected]>
---
 MAINTAINERS                      |  2 +
 include/linux/zone_lock.h        | 64 +++++++++++++++++++++++++++++++-
 include/trace/events/zone_lock.h | 64 ++++++++++++++++++++++++++++++++
 mm/Makefile                      |  2 +-
 mm/zone_lock.c                   | 31 ++++++++++++++++
 5 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 include/trace/events/zone_lock.h
 create mode 100644 mm/zone_lock.c
MAINTAINERS +2 -0
index 680c9ae02d7e..711ffa15f4c3 100644
@@ -16499,6 +16499,7 @@ F:	include/linux/ptdump.h
 F:	include/linux/vmpressure.h
 F:	include/linux/vmstat.h
 F:	include/linux/zone_lock.h
+F:	include/trace/events/zone_lock.h
 F:	kernel/fork.c
 F:	mm/Kconfig
 F:	mm/debug.c

@@ -16518,6 +16519,7 @@ F:	mm/sparse.c
 F:	mm/util.c
 F:	mm/vmpressure.c
 F:	mm/vmstat.c
+F:	mm/zone_lock.c
 N:	include/linux/page[-_]*
 
 MEMORY MANAGEMENT - EXECMEM
include/linux/zone_lock.h +63 -1

95 lines not on this page · raw message

include/trace/events/zone_lock.h new file +64 -0

64 lines not on this page · raw message

mm/Makefile +1 -1
index 0d85b10dbdde..fd891710c696 100644
@@ -55,7 +55,7 @@ obj-y			:= filemap.o mempool.o oom_kill.o fadvise.o \
 			   mm_init.o percpu.o slab_common.o \
 			   compaction.o show_mem.o \
 			   interval_tree.o list_lru.o workingset.o \
-			   debug.o gup.o mmap_lock.o vma_init.o $(mmu-y)
+			   debug.o gup.o mmap_lock.o zone_lock.o vma_init.o $(mmu-y)
 
 # Give 'page_alloc' its own module-parameter namespace
 page-alloc-y := page_alloc.o
mm/zone_lock.c new file +31 -0
new file mode 100644
index 000000000000..f647fd2aca48
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+#define CREATE_TRACE_POINTS
+#include <trace/events/zone_lock.h>
+
+#include <linux/zone_lock.h>
+
+EXPORT_TRACEPOINT_SYMBOL(zone_lock_start_locking);
+EXPORT_TRACEPOINT_SYMBOL(zone_lock_acquire_returned);
+EXPORT_TRACEPOINT_SYMBOL(zone_lock_released);
+
+#ifdef CONFIG_TRACING
+
+void __zone_lock_do_trace_start_locking(struct zone *zone)
+{
+	trace_zone_lock_start_locking(zone);
+}
+EXPORT_SYMBOL(__zone_lock_do_trace_start_locking);
+
+void __zone_lock_do_trace_acquire_returned(struct zone *zone, bool success)
+{
+	trace_zone_lock_acquire_returned(zone, success);
+}
+EXPORT_SYMBOL(__zone_lock_do_trace_acquire_returned);
+
+void __zone_lock_do_trace_released(struct zone *zone)
+{
+	trace_zone_lock_released(zone);
+}
+EXPORT_SYMBOL(__zone_lock_do_trace_released);
+
+#endif /* CONFIG_TRACING */
Signature · 1 line
2.47.3
#14 lore raw
In reply to Dmitry Ilvokhin
Quoted 25 lines · Dmitry Ilvokhin

Zone lock contention can significantly impact allocation and reclaim latency, as it is a central synchronization point in the page allocator and reclaim paths. Improved visibility into its behavior is therefore important for diagnosing performance issues in memory-intensive workloads.

On some production workloads at Meta, we have observed noticeable zone lock contention. Deeper analysis of lock holders and waiters is currently difficult with existing instrumentation.

While generic lock contention_begin/contention_end tracepoints cover the slow path, they do not provide sufficient visibility into lock hold times. In particular, the lack of a release-side event makes it difficult to identify long lock holders and correlate them with waiters. As a result, distinguishing between short bursts of contention and pathological long hold times requires additional instrumentation.

This patch series adds dedicated tracepoint instrumentation to zone lock, following the existing mmap_lock tracing model.

The goal is to enable detailed holder/waiter analysis and lock hold time measurements without affecting the fast path when tracing is disabled.

Quoted 7 lines

The series is structured as follows:

1. Introduce zone lock wrappers.
2. Mechanically convert zone lock users to the wrappers.
3. Convert compaction to use the wrappers (requires minor restructuring of compact_lock_irqsave()).
4. Add zone lock tracepoints.

I think you can improve the flow of this series if reorder as follows:
1. Introduce zone lock wrappers
4. Add zone lock tracepoints
2. Mechanically convert zone lock users to the wrappers
3. Convert compaction to use the wrappers...

and possibly squash 1 & 4 (though that might be too big of a patch). It's better to introduce the wrappers and their tracepoints together before the reviewer (i.e. me) forgets what was added in patch 1 by the time they get to patch 4.

Thanks,
Ben

reviewer
#15 lore raw
Quoted 40 lines · Benjamin
Quoted 25 lines · Dmitry Ilvokhin

Zone lock contention can significantly impact allocation and reclaim latency, as it is a central synchronization point in the page allocator and reclaim paths. Improved visibility into its behavior is therefore important for diagnosing performance issues in memory-intensive workloads.

On some production workloads at Meta, we have observed noticeable zone lock contention. Deeper analysis of lock holders and waiters is currently difficult with existing instrumentation.

While generic lock contention_begin/contention_end tracepoints cover the slow path, they do not provide sufficient visibility into lock hold times. In particular, the lack of a release-side event makes it difficult to identify long lock holders and correlate them with waiters. As a result, distinguishing between short bursts of contention and pathological long hold times requires additional instrumentation.

This patch series adds dedicated tracepoint instrumentation to zone lock, following the existing mmap_lock tracing model.

The goal is to enable detailed holder/waiter analysis and lock hold time measurements without affecting the fast path when tracing is disabled.

Quoted 7 lines

The series is structured as follows:

1. Introduce zone lock wrappers.
2. Mechanically convert zone lock users to the wrappers.
3. Convert compaction to use the wrappers (requires minor restructuring of compact_lock_irqsave()).
4. Add zone lock tracepoints.

I think you can improve the flow of this series if reorder as follows:
1. Introduce zone lock wrappers
4. Add zone lock tracepoints
2. Mechanically convert zone lock users to the wrappers
3. Convert compaction to use the wrappers...

Quoted 3 lines

and possibly squash 1 & 4 (though that might be too big of a patch). It's better to introduce the wrappers and their tracepoints together before the reviewer (i.e. me) forgets what was added in patch 1 by the time they get to patch 4.

I don't think this suggestion will make anything better. This just seems like a different taste. If I make a suggestion, I would request to squash (1) and (2) i.e. patch containing wrappers and their use together but that is just my taste and would be a nit. The series ordering is good as is.

author
#16 lore raw
In reply to Cheatham, Benjamin
Quoted 40 lines · Benjamin
Quoted 25 lines · Dmitry Ilvokhin

Zone lock contention can significantly impact allocation and reclaim latency, as it is a central synchronization point in the page allocator and reclaim paths. Improved visibility into its behavior is therefore important for diagnosing performance issues in memory-intensive workloads.

On some production workloads at Meta, we have observed noticeable zone lock contention. Deeper analysis of lock holders and waiters is currently difficult with existing instrumentation.

While generic lock contention_begin/contention_end tracepoints cover the slow path, they do not provide sufficient visibility into lock hold times. In particular, the lack of a release-side event makes it difficult to identify long lock holders and correlate them with waiters. As a result, distinguishing between short bursts of contention and pathological long hold times requires additional instrumentation.

This patch series adds dedicated tracepoint instrumentation to zone lock, following the existing mmap_lock tracing model.

The goal is to enable detailed holder/waiter analysis and lock hold time measurements without affecting the fast path when tracing is disabled.

Quoted 7 lines

The series is structured as follows:

1. Introduce zone lock wrappers.
2. Mechanically convert zone lock users to the wrappers.
3. Convert compaction to use the wrappers (requires minor restructuring of compact_lock_irqsave()).
4. Add zone lock tracepoints.

I think you can improve the flow of this series if reorder as follows:
1. Introduce zone lock wrappers
4. Add zone lock tracepoints
2. Mechanically convert zone lock users to the wrappers
3. Convert compaction to use the wrappers...

Quoted 3 lines

and possibly squash 1 & 4 (though that might be too big of a patch). It's better to introduce the wrappers and their tracepoints together before the reviewer (i.e. me) forgets what was added in patch 1 by the time they get to patch 4.

Hi Ben,

Thanks for the suggestion.

I structured the series intentionally to keep all behavior-preserving refactoring separate from the actual instrumentation change.

In particular, I had to split the conversion into two patches to separate the purely mechanical changes from the compaction restructuring. With the current order, tracepoints addition remains a single, atomic functional change on top of a fully converted tree. This keeps the instrumentation isolated from the refactoring and with an intention to make bisection and review of the behavioral change easier.

Reordering as suggested would mix instrumentation with intermediate refactoring states, which I'd prefer to avoid.

I hope this reasoning makes sense, but I'm happy to discuss if there are strong objections.

Quoted 3 lines

Thanks,
Ben

#17 lore raw
Quoted 61 lines · Dmitry Ilvokhin

[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

Quoted 40 lines · Benjamin
Quoted 25 lines · Dmitry Ilvokhin

Zone lock contention can significantly impact allocation and reclaim latency, as it is a central synchronization point in the page allocator and reclaim paths. Improved visibility into its behavior is therefore important for diagnosing performance issues in memory-intensive workloads.

On some production workloads at Meta, we have observed noticeable zone lock contention. Deeper analysis of lock holders and waiters is currently difficult with existing instrumentation.

While generic lock contention_begin/contention_end tracepoints cover the slow path, they do not provide sufficient visibility into lock hold times. In particular, the lack of a release-side event makes it difficult to identify long lock holders and correlate them with waiters. As a result, distinguishing between short bursts of contention and pathological long hold times requires additional instrumentation.

This patch series adds dedicated tracepoint instrumentation to zone lock, following the existing mmap_lock tracing model.

The goal is to enable detailed holder/waiter analysis and lock hold time measurements without affecting the fast path when tracing is disabled.

Quoted 7 lines

The series is structured as follows:

1. Introduce zone lock wrappers.
2. Mechanically convert zone lock users to the wrappers.
3. Convert compaction to use the wrappers (requires minor restructuring of compact_lock_irqsave()).
4. Add zone lock tracepoints.

I think you can improve the flow of this series if reorder as follows:
1. Introduce zone lock wrappers
4. Add zone lock tracepoints
2. Mechanically convert zone lock users to the wrappers
3. Convert compaction to use the wrappers...

Quoted 3 lines

and possibly squash 1 & 4 (though that might be too big of a patch). It's better to introduce the wrappers and their tracepoints together before the reviewer (i.e. me) forgets what was added in patch 1 by the time they get to patch 4.

Hi Ben,

Thanks for the suggestion.

I structured the series intentionally to keep all behavior-preserving refactoring separate from the actual instrumentation change.

In particular, I had to split the conversion into two patches to separate the purely mechanical changes from the compaction restructuring. With the current order, tracepoints addition remains a single, atomic functional change on top of a fully converted tree. This keeps the instrumentation isolated from the refactoring and with an intention to make bisection and review of the behavioral change easier.

Quoted 5 lines

Reordering as suggested would mix instrumentation with intermediate refactoring states, which I'd prefer to avoid.

I hope this reasoning makes sense, but I'm happy to discuss if there are strong objections.

No that's fine, I figured as much. I just wasn't sure that was more important to you than what (I thought) was a better reading order for the series.

Thanks,
Ben

Quoted 4 lines
Quoted 4 lines

Thanks,
Ben