Re: [PATCH v3 11/11] s390/percpu: Rework to simplify percpu_entry() and percpu_exit()

[David Laight]

On Mon, 21 Sep 2026 10:40:05 +0200
Heiko Carstens <hca@xxxxxxxxxxxxx> wrote:

> The percpu code section functionality uses a rather complex method to
> figure out if the register, which contains the address of the current
> cpu's percpu variable, needs to be adjusted.
> 
> If an interrupt happens within a percpu code section (indicated by a
> lowcore field), the instruction at the interrupted location is
> checked. If it is not a specific AG instruction, the register needs to
> be updated. This mechanism needs to take kprobes into account, and
> enforces a specific instruction ordering.
> 
> Mark Rutland provided a different solution for arm64 [1] which comes
> without such limitations, but requires to use one more instruction, and
> two more registers. Given that this simplifies percpu_entry() and
> percpu_exit() it seems to be worth to go that route.
> 
> Change s390 to implement a similar approach. This requires to encode
> three register numbers into the "percpu_register" field, which is used
> to indicate if a percpu code section is executed.
> 
> The used mviy instruction can write only one byte, which allows to
> encode only two register numbers. Use a register pair for the inline
> assemblies, and only encode the even register number of the register
> pair to work around this.
> 
> The generated code changes like this for e.g. a simple this_cpu_inc():
> 
> Old:
> 
> c0 20 00 00 00 00  larl    %r2,c6 <foo+0x6> <-- load address of percpu var
> b9 04 00 32        lgr     %r3,%r2          <-- pointless copy of address
> eb 03 03 c0 00 52  mviy    960,3            <-- start of percpu code section
>                                                 - gpr 3 contains percpu var address
> e3 30 03 b8 00 08  ag      %r3,952          <-- add percpu offset
> eb 01 30 00 00 7a  agsi    0(%r3),1         <-- atomic inc
> eb 00 03 c0 00 52  mviy    960,0            <-- end of percpu code section
> 
> New:
> 
> c0 10 00 00 00 00  larl    %r1,c6 <foo+0x6> <-- load address of percpu var
> eb 21 03 c0 00 52  mviy    960,33           <-- start of percpu code section
>                                                 33 == 0x21:
> 						- gpr 1 contains percpu var address
> 						- gpr 2 used for percpu offset
>   					        - gpr 2+1 == 3 used for current cpu's percpu var address
> e3 20 03 b8 00 04  lg      %r2,952          <-- load percpu offset
> 41 32 10 00        la      %r3,0(%r2,%r1)   <-- generate current cpu's percpu var address
> eb 01 30 00 00 7a  agsi    0(%r3),1         <-- atomic inc
> eb 00 03 c0 00 52  mviy    960,0            <-- end of percpu code section
> 
> In the above "new" code example 33 (0x21) is used as indicator value to
> mark that a percpu code section is executed. This value implies that
> registers 2 and 3 will (only) hold the percpu offset and the current
> cpu's percpu var address. Those registers will be updated by
> percpu_exit() if the process was migrated to a different cpu to contain
> the percpu offset and percpu var address of the new cpu.
> 
> Note that because of the pointless lgr instruction in the "old" code
> example the two code sequences have identical size, and it looks like
> only one more register is used. However this is because of suboptimal
> gcc code generation.

In both cases the register dependency chain length is 3 (ignoring the
pointless lgr) so the execution time is likely to be the same.

...
> +	regpcp = FIELD_GET(PCPU_REG_PCP, regval);
> +	regoff = FIELD_GET(PCPU_REG_OFF, regval);
...
> +#define PCPU_REG_PCP_SHIFT		0
> +#define PCPU_REG_PCP			GENMASK(3, 0)
> +#define PCPU_REG_OFF_SHIFT		4
> +#define PCPU_REG_OFF			GENMASK(7, 4)
...
> +#define __PCPU_CALC_REGVAL(regpcp, regoff)					\
> +	"(" regpcp " << " __stringify(PCPU_REG_PCP_SHIFT) ") |"			\
> +	"(" regoff " << " __stringify(PCPU_REG_OFF_SHIFT) ")"

I'm not a big fan of GENMASK() + FIELD_GET() and I'm not at all sure it
really helps here.
Maybe:
	regpcp = (regval >> PCPU_REG_PCP_SHIFT) & 15;
	regoff = (regval >> PCPU_REG_OFF_SHIFT) & 15;
Which at least uses the same constants for encode and decode.

But I might just comment that the pcp register is in the high nibble
(twice) and remove the 'crud'.
#define __PCPU_CALC_REGVAL(regpcp, regoff) "(" regpcp " << 4 ) | " regoff
	regpcp = regval >> 4;
	regoff = regval & 15;

David