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

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

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.

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.

> > --- a/arch/s390/kernel/setup.c
> > +++ b/arch/s390/kernel/setup.c
> > @@ -77,6 +77,7 @@
> >  #include <asm/maccess.h>
> >  #include <asm/uv.h>
> >  #include <asm/asm-offsets.h>
> > +#include <asm/lazy_mmu.h>
> >  #include "entry.h"
> >  
> >  /*
> > @@ -1012,5 +1013,6 @@ void __init setup_arch(char **cmdline_p)
> >  
> >  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.

> > --- a/arch/s390/kernel/smp.c
> > +++ b/arch/s390/kernel/smp.c
> > @@ -59,6 +59,7 @@
> >  #include <asm/topology.h>
> >  #include <asm/vdso.h>
> >  #include <asm/maccess.h>
> > +#include <asm/lazy_mmu.h>
> >  #include "entry.h"
> >  
> >  enum {
> > @@ -866,6 +867,11 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> >  	rc = pcpu_alloc_lowcore(pcpu, cpu);
> >  	if (rc)
> >  		return rc;
> > +	rc = lazy_mmu_online_cpu(GFP_KERNEL, cpu);
> > +	if (rc) {
> > +		pcpu_free_lowcore(pcpu, cpu);
> > +		return rc;
> > +	}
> >  	/*
> >  	 * Make sure global control register contents do not change
> >  	 * until new CPU has initialized control registers.
> > @@ -921,6 +927,7 @@ void __cpu_die(unsigned int cpu)
> >  	pcpu = per_cpu_ptr(&pcpu_devices, cpu);
> >  	while (!pcpu_stopped(pcpu))
> >  		cpu_relax();
> > +	lazy_mmu_offline_cpu(cpu);
> 
> Same here: what makes this code so special that this needs to be open-coded
> into the low level cpu hotplug code? Everybody who needs to change this

This is just a follow-up of the boot CPU initialization above.
AKA "Do not use CPU hotplug events".

> code in future will wonder why the mmu code is so special that it needs to
> be directly handled here, and then needs to understand the mmu code.
> And the answer is: there is no reason.
> 
> Please use a generic cpu hotplug notifier to avoid that maintenance get's
> more expensive.

Will retry.

> > +static void leave_ipte_range(void)
> > +{
> > +	pte_t *ptep, *start, *start_cache, *cache;
> > +	unsigned long start_addr, addr;
> > +	struct ipte_range *range;
> > +	int start_idx;
> > +
> > +	if (!test_facility(13))
> > +		return;
> > +
> > +	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:

    [    6.784258] Call Trace:
    [    7.784260]  [<0013d8935c3fe9dc>] pcpu_free_area+0x11c/0x3f0
    [    6.784265]  [<0013d8935c400bb6>] free_percpu.part.0+0x1b6/0xc70
    [    6.784270]  [<0013d8935bd8931c>] sched_free_group_rcu+0x2c/0x60
    [    6.784274]  [<0013d8935bea9196>] rcu_do_batch+0x2f6/0xdd0
    [    6.784280]  [<0013d8935beba0d0>] rcu_core+0x270/0x4e0
    [    6.784285]  [<0013d8935bd0a4b4>] handle_softirqs+0x294/0x800
    [    6.784289]  [<0013d8935bd0b020>] irq_exit_rcu+0x140/0x200
    [    6.784293]  [<0013d8935e26d994>] do_ext_irq+0xe4/0x330
    [    6.784298]  [<0013d8935e28c25c>] ext_int_handler+0xec/0x118
    [    6.784303]  [<0013d8935bc9764e>] enter_ipte_range+0xfe/0x1a0
    [    6.784308] ([<0013d8935bc975f6>] enter_ipte_range+0xa6/0x1a0)
    [    6.784313]  [<0013d8935c4432ce>] zap_pte_range+0x9ee/0xef0
    [    6.784317]  [<0013d8935c443a44>] zap_p4d_range+0x274/0x710
    [    6.784321]  [<0013d8935c44411c>] __zap_vma_range+0x23c/0x450
    [    6.784325]  [<0013d8935c445078>] unmap_vmas+0x1c8/0x440
    [    6.784329]  [<0013d8935c4a8f46>] unmap_region+0x196/0x320
    [    6.784332]  [<0013d8935c4ac174>] vms_complete_munmap_vmas+0x734/0x990
    [    6.784336]  [<0013d8935c4aec74>] do_vmi_align_munmap+0x2a4/0x3a0
    [    6.784340]  [<0013d8935c46138e>] __do_sys_brk+0x5fe/0x750
    [    6.784344]  [<0013d8935e26d3be>] __do_syscall+0x17e/0x3f0
    [    6.784348]  [<0013d8935e28b882>] system_call+0x72/0x90
    [    6.784352] Last Breaking-Event-Address:
    [    6.784353]  [<0013d8935c3febea>] pcpu_free_area+0x32a/0x3f0
    [    6.784361] Kernel panic - not syncing: Fatal exception in interrupt

> Also at least for !PREEMPT_RT (which is always true for s390)
> lockdep_assert_preemption_disabled() is quite pointless, since
> local_bh_disable() just one line above disables preemption.
> 
> I guess you wanted to add that check above local_bh_disable()?

Exactly.

> If not you could as well remove it

Thanks a lot for the review, Heiko!