Re: [PATCH 1/8] mm: use zone lock guard in reserve_highatomic_pageblock()

[Dmitry Ilvokhin <d@ilvokhin.com>]

On Tue, Apr 28, 2026 at 01:47:21PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2026 at 10:58:41AM +0000, Dmitry Ilvokhin wrote:
> 
> > I re-tested my original patchset after rebasing and can still reproduce
> > the regression (though smaller). It appears to depend on compiler
> > inlining decisions: in some cases the compiler is able to deduplicate
> > the cleanup path across multiple return sites, while in others it is
> > not.
> 
> I'm confused, all this has __always_inline on. And the compilers should
> be able to track the assignment of the variable and eliminate the test
> themselves if value-tracking excludes NULL.
> 
> > Given that, I think we can go further than just removing
> > __GUARD_IS_ERR(). It should be possible to eliminate this branch
> > entirely and simplify the cleanup flow.
> > 
> >     https://lore.kernel.org/all/20260427165037.205337-1-d@ilvokhin.com/
> > 
> > Reposting here to increase visibility, as several people involved in
> > this code have participated in this thread already.
> > 
> > Any feedback would be appreciated.
> 
> This would require very careful audit of all the current users, it's had
> this behaviour from the start.

I did a bit of code archeology to double check I was not missing
anything here.

- May 2023: guards were introduced by 54da6a092431 ("locking: Introduce
  __cleanup() based infrastructure") and DEFINE_GUARD() called _unlock
  unconditionally. __DEFINE_UNLOCK_GUARD() had a NULL check, but it was
  unnecessary since conditional variants did not exist yet and the
  constructor always set .lock.

- Sep 2023: conditional guards were added by e4ab322fbaaa ("cleanup: Add
  conditional guard support"). The branch was added there, because
  conditional destructor directly delegated to the base class destructor.
  When a trylock failed, .lock was set to NULL.

- Jul 2025: ACQUIRE() was introduced by 857d18f23ab1 ("cleanup:
  Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks") and
  changed the conditional failure value from NULL to ERR_PTR(_RET).

- Mar 2026: 2deccd5c862a ("cleanup: Optimize guards")
  EXTEND_CLASS_COND() was introduced with its own if (_cond) return;
  pre-check before calling the base destructor.

After EXTEND_CLASS_COND() was introduced, the conditional destructor
does if (_cond) return; before calling the base, so the base destructor
is only ever reached for successfully acquired locks.

Peter, do you think it is accurate?

All the commits above are from you, so you are the best person to catch
if I missed something here :)