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.

I'd expect that too, but seems it is not the case. I was looking at this
patchset rebased on top of the mm-stable build with GCC 16 with
defconfig.

    https://lore.kernel.org/all/cover.1775654118.git.d@ilvokhin.com/


$ ./scripts/bloat-o-meter /tmp/bloat-gcc-16/page_alloc.o.before /tmp/bloat-gcc-16/page_alloc.o.after | grep -v UNIQUE
add/remove: 24/24 grow/shrink: 2/1 up/down: 232/-193 (39)
Function                                     old     new   delta
free_pcppages_bulk                           577     601     +24
free_pages_exact                             153     169     +16
unreserve_highatomic_pageblock               608     607      -1
Total: Before=42366, After=42405, chg +0.09%

I looked at free_pcppages_bulk() generated code diff and got the
following differences (only relevant parts).

[...]

- mov 0x20(%rsp),%r12
- mov 0x28(%rsp),%r15
+ mov 0x20(%rsp),%r13
+ mov 0x28(%rsp),%r12
+ test %r13,%r13                       ; guard NULL branch
+ je 2ba6 <free_pcppages_bulk+0x246>

[...]

  add $0x30,%rsp
- mov %r15,%rsi
- mov %r12,%rdi
+ mov %r12,%rsi
+ mov %r13,%rdi
  pop %rbx
  pop %rbp
  pop %r12
  pop %r13
  pop %r14
  pop %r15
- jmp 2b8b <free_pcppages_bulk+0x22b>
+ jmp 2b90 <free_pcppages_bulk+0x230>
  mov $0x800,%eax
  jmp 2af3 <free_pcppages_bulk+0x193>
  mov 0x1c(%rsp),%edx
  mov %ebp,%r14d
  jmp 29ae <free_pcppages_bulk+0x4e>
- data16 cs nopw 0x0(%rax,%rax,1)
+ add $0x30,%rsp                      ; epilogue is duplicated once again.
+ pop %rbx
+ pop %rbp
+ pop %r12
+ pop %r13
+ pop %r14
+ pop %r15
+ jmp 2bb9 <free_pcppages_bulk+0x259>

So, NULL check is not eliminated and in addition compiler couldn't merge
two almost identical epilogues here. I suspect the function calls
between construction and destruction (e.g., __free_one_page()) prevent
the compiler from proving .lock is non-NULL across the scope, but this
is just speculation, I am not a compiler expert.

> 
> > 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.

My thinking process was the following: the constructor doesn't check for
NULL either, so if something is passing NULL to begin with it should
already crash in constructor not even reaching the destructor.

The only case when we'd need the NULL check in the destructor is if
something changes .lock to NULL after construction, but before
destruction.

This can't happen:

- The guard infrastructure provides no API to clear a guard after
  construction (no_free_ptr()/retain_ptr() only work with __free, not
  guards).

- guard() creates an anonymous variable via __UNIQUE_ID, so user code                                                   
  can't reference it to modify .lock directly.

- CLASS_INIT() is the only macro that allows bypassing the constructor,                                                 
  but it is only used for fd_prepare in include/linux/file.h, not for
  any lock guards.

- Conditional (_try) variants have their own destructor via
  EXTEND_CLASS_COND() that checks __GUARD_IS_ERR() and returns early
  before reaching the base destructor. They are not affected by this
  change.