linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Dmitry Ilvokhin <d@ilvokhin.com>
To: "Cheatham, Benjamin" <benjamin.cheatham@amd.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-trace-kernel@vger.kernel.org, linux-cxl@vger.kernel.org,
	kernel-team@meta.com, Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@kernel.org>,
	Lorenzo Stoakes <lorenzo.stoakes@oracle.com>,
	"Liam R. Howlett" <Liam.Howlett@oracle.com>,
	Vlastimil Babka <vbabka@suse.cz>, Mike Rapoport <rppt@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Michal Hocko <mhocko@suse.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Brendan Jackman <jackmanb@google.com>,
	Johannes Weiner <hannes@cmpxchg.org>, Zi Yan <ziy@nvidia.com>,
	Oscar Salvador <osalvador@suse.de>,
	Qi Zheng <zhengqi.arch@bytedance.com>,
	Shakeel Butt <shakeel.butt@linux.dev>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Yuanchu Xie <yuanchu@google.com>, Wei Xu <weixugc@google.com>
Subject: Re: [PATCH 3/4] mm: convert compaction to zone lock wrappers
Date: Tue, 24 Feb 2026 15:50:40 +0000	[thread overview]
Message-ID: <aZ3I0ADTAdCN6UmN@shell.ilvokhin.com> (raw)
In-Reply-To: <74fc1f28-b77e-4b9c-b208-51babae9d18e@amd.com>

On Fri, Feb 20, 2026 at 01:10:05PM -0600, Cheatham, Benjamin wrote:
> On 2/11/2026 9:22 AM, Dmitry Ilvokhin wrote:
> > 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 <d@ilvokhin.com>
> > ---
> >  mm/compaction.c | 108 +++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 89 insertions(+), 19 deletions(-)
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 1e8f8eca318c..1b000d2b95b2 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -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);
> > +}
> 
> 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!

> > +
> > +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);
> 
> You don't need the return statement here (and you shouldn't be returning a value at all).

Yes, agree, will fix in v2.

> 
> It may be cleaner to just do an if-else statement here instead.
> 
> > +}
> > +
> >  /*
> >   * 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);
> 
> 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.

> 
> >  		*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;
> 


  reply	other threads:[~2026-02-24 15:50 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-11 15:22 [PATCH 0/4] mm: zone lock tracepoint instrumentation Dmitry Ilvokhin
2026-02-11 15:22 ` [PATCH 1/4] mm: introduce zone lock wrappers Dmitry Ilvokhin
2026-02-23 22:36   ` Shakeel Butt
2026-02-24 15:18     ` Dmitry Ilvokhin
2026-02-24 15:30       ` Shakeel Butt
2026-02-24 15:31   ` Shakeel Butt
2026-02-11 15:22 ` [PATCH 2/4] mm: convert zone lock users to wrappers Dmitry Ilvokhin
2026-02-23 23:28   ` Shakeel Butt
2026-02-11 15:22 ` [PATCH 3/4] mm: convert compaction to zone lock wrappers Dmitry Ilvokhin
2026-02-20 19:10   ` Cheatham, Benjamin
2026-02-24 15:50     ` Dmitry Ilvokhin [this message]
2026-02-11 15:22 ` [PATCH 4/4] mm: add tracepoints for zone lock Dmitry Ilvokhin
2026-02-20 19:09 ` [PATCH 0/4] mm: zone lock tracepoint instrumentation Cheatham, Benjamin
2026-02-20 22:36   ` Shakeel Butt
2026-02-23 16:46   ` Dmitry Ilvokhin
2026-02-23 17:17     ` Cheatham, Benjamin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aZ3I0ADTAdCN6UmN@shell.ilvokhin.com \
    --to=d@ilvokhin.com \
    --cc=Liam.Howlett@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=axelrasmussen@google.com \
    --cc=benjamin.cheatham@amd.com \
    --cc=david@kernel.org \
    --cc=hannes@cmpxchg.org \
    --cc=jackmanb@google.com \
    --cc=kernel-team@meta.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=lorenzo.stoakes@oracle.com \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=mhocko@suse.com \
    --cc=osalvador@suse.de \
    --cc=rostedt@goodmis.org \
    --cc=rppt@kernel.org \
    --cc=shakeel.butt@linux.dev \
    --cc=surenb@google.com \
    --cc=vbabka@suse.cz \
    --cc=weixugc@google.com \
    --cc=yuanchu@google.com \
    --cc=zhengqi.arch@bytedance.com \
    --cc=ziy@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox