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

[Heiko Carstens <hca@linux.ibm.com>]

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?

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

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

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

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

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()?
If not you could as well remove it