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

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

> No, it can not get negative and it is very handy to observe
> a disbalance in a crash (I did hit it indeed while debugging).
> 
> > > +static __always_inline bool is_lazy_mmu_active(void)
> > > +{
> > > +	if (__is_defined(__DECOMPRESSOR))
> > > +		return false;
> > > +	if (!get_lowcore()->lazy_mmu_count)
> > > +		return false;
> > 
> > I guess there is opportunity to generate better code here using an
> > alternative and using a flag output constraint too.
> 
> Will try.

Guess that will be quite ugly code. Just let me know if I should
provide that code, since I coded up such a mess already several times.
> > >  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.

> > > +	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
solve that problem without disabling bottom halves by reordering
sequences in enter_ipte_range() and leave_ipte_range()?

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