Re: [PATCH v7 2/4] s390/mm: Batch PTE updates in lazy MMU mode

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Wed, Aug 26, 2026 at 03:02:57PM +0200, Heiko Carstens wrote:
> On Wed, Aug 26, 2026 at 02:00:35PM +0200, Alexander Gordeev wrote:
> > On Mon, Aug 24, 2026 at 12:40:48PM +0200, Heiko Carstens wrote:
> > > On Mon, Aug 17, 2026 at 01:33:00PM +0200, Alexander Gordeev wrote:
> > > > diff --git a/arch/s390/include/asm/lowcore.h b/arch/s390/include/asm/lowcore.h
> > > > index 3b3ecc647993..dba236664da9 100644
> > > > --- a/arch/s390/include/asm/lowcore.h
> > > > +++ b/arch/s390/include/asm/lowcore.h
> > > > @@ -163,7 +163,7 @@ struct lowcore {
> > > >  	__s32	preempt_count;			/* 0x03a8 */
> > > >  	__u32	spinlock_lockval;		/* 0x03ac */
> > > >  	__u32	spinlock_index;			/* 0x03b0 */
> > > > -	__u8	pad_0x03b4[0x03b8-0x03b4];	/* 0x03b4 */
> > > > +	__s32	lazy_mmu_count;			/* 0x03b4 */
> > > 
> > > Why is this signed? Can it get negative?
> > 
> > For the same reason preempt_count is signed, I guess.
> 
> Check again, preempt_count is now unsigned and 64 bit ;)

D'oh :)

...

> > > >  void __init arch_cpu_finalize_init(void)
> > > >  {
> > > > +	lazy_mmu_online_boot_cpu();
> > > >  	sclp_init();
> > > >  }
> > > 
> > > What makes this code so special that an explicit call from
> > > arch_cpu_finalize_init() is required? This is really the last resort if
> > > everything else fails. To me it looks like the code can be changed to use a
> > > new static key, and add a generic early (pre-smp) initcall to allocate
> > > memory for cpu 0, and if that succeeds enable the static key.
> > 
> > I had exactly similar variant, but failed to resolve a race when a secondary
> > CPU callback was called before the CPU0's one. Probably, used a wrong event.
> > Will look into it again.
> 
> early_initcall() should do the trick.

Yep, early_initcall() + CPUHP_BP_PREPARE_DYN seems to be flying ;)

> > > > +	local_bh_disable();
> > > > +
> > > > +	lockdep_assert_preemption_disabled();
> > > > +	range = this_cpu_read(ipte_range);
> > > 
> > > Why is it required to disable bottom halves? A comment would be helpful.
> > > Or a hint in the commit message - this is not obvious.
> > 
> > When an interrupt arrives in the middle of enter|leave_ipte_range()
> > the chain pcpu_addr_to_page() -> vmalloc_to_page() -> ptep_get()
> > decides ptep_get() is called in lazy mode, while the per-cpu state
> > not yet (de-)initialized (AKA inconsistent). That led to crashes:
> 
> So, I don't know what exactly lead to the crash, but I guess the
> problem is an only partially initialized ipte_range struct, while the
> lazy_mmu_count has been incremented already? Isn't it possible to

Exactly.

> solve that problem without disabling bottom halves by reordering
> sequences in enter_ipte_range() and leave_ipte_range()?

I guess you mean setting|unsetting the lazy mmu flag after|before the
state is initialized? I will think about it, though I keep the door
open for a possible lazy_mmu_mode_pause()|resume() implementation,
which I am unable to predict how such rework would fit: lazy_mmu_count
would have to stay non-zero in such a case.

> Would be nice if we could avoid the not so obvious local_bh_disable()
> and local_bh_enable() pairs.

Calling ptep_get() from BH context was certainly unexpected, but the way
local_bh_enable|disable() pairs are used is actually straightforward.
This is a slow path anyway, so I would think the simplicity prevails in
this case.

But again, I will try to avoid that.

Thanks!