Re: [PATCH v5 1/7] s390/percpu: Infrastructure for more efficient this_cpu operations

[David Laight]

On Mon, 1 Jun 2026 17:08:13 +0200
Heiko Carstens <hca@xxxxxxxxxxxxx> wrote:

> On Mon, Jun 01, 2026 at 04:49:35PM +0200, Alexander Gordeev wrote:
> > On Mon, Jun 01, 2026 at 03:27:50PM +0200, Heiko Carstens wrote:
> > ...  
> > > > > +	if ((insn & 0xff0f) != 0xe300)  
> > > > 
> > > > Any reason mask/not to check the register number as well?  
> > > 
> > > Just to save code. If there would be a mismatch with the percpu
> > > register, something serious would be wrong..  
> > 
> > Is it worth checking the disp then? ;)  
> 
> It is: the check makes sure this is an AG instruction, which adds the
> percpu offset from lowcore - by checking that the displacement is
> correct, as well as that the base register is zero.
> 
> There could be a different AG instruction within the inline assembly,
> for whatever reason.

Do you actually even need to check the instruction?

This sequence can only work for simple per-cpu accesses, so I don't
see a reason to let the specified register point anywhere other than the
base of the per-cpu data.

That means the process switch code can just load the register with the
base of the per-cpu data for the new cpu.
If that happens before the 'AG' is executed it won't matter.

The only reason would be to support non-offsettable memory accesses.
But it looks like the 'laag    %r5,%r2,0(%r4)' in the example has an
offset (of zero).
Probably only stops you doing a direct access of an array.

That would mean that needs_fixup goes in the bin and percpu_exit() becomes:
	...
	reg = regs->percpu_register;
	if (likely(!reg))
		return;
	lc->percpu_register = reg;
	regs->gprs[reg] = lc->percpu_offset
}

I guess I'm missing something?

-- David

> 
> > > > > + * Inline assemblies making use of this typically have a code sequence like:
> > > > > + *
> > > > > + *   MVIY_PERCPU(...) <- start of percpu code section
> > > > > + *   AG_ALT(...)      <- add percpu offset; must be the second instruction
> > > > > + *   atomic_op	      <- atomic op  
> > > >                  \t here, but should be spaces?  
> > > 
> > > I can't follow? We have tabs in comments all over the place in s390 code.  
> > 
> > The other '<-' comments below and above use spaces, but this one
> > mixes spaces with '\t'.  
> 
> Because it is not possible to use tabs there. We put tabs in our
> comments whenever possible.
> 
> > > > Probably it worth noting that no extra instructions between atomic_op
> > > > and MVIY_ALT may exist (e.g. in the future), especially ones that use
> > > > the percpu_register.  
> > > 
> > > That's not true. There may be additional instructions, they may just
> > > not use the same register that is used for the percpu variable.  
> > 
> > That is what I tried to say, but I also thought it is intentionally
> > only two instructions between mviy brackets allowed: ag + atomic_op.
> > Am I wrong here?  
> 
> Yes, you are wrong. You can use as many instructions as you want, as
> long as the inline assembly follows the rules with respect to the
> register which contains the percpu variable.
> 
> But seriously: all of this works only with atomic ops, so I don't see
> reason why one put more instructions there. We actively try to keep
> our inline assemblies as small as possible.
> 
> Even though with relocatable lowcore and alternatives the C code looks
> huge, while it only generates very few instructions.
> 
> > > But that was true before this patch as well, even though it might not
> > > have been "obvious".  
> > 
> > Hmm.. I do not get it. We would not get scheduled away before this patch,
> > aren't we?  
> 
> True. What I tried to say: before and after this patch there was and
> is no code which _uses_ the pointer of the percpu variable more than
> once. Accessing the percpu variable twice within such a section would
> be potentially broken, since between two accesses an interrupt / nmi
> could happen, which could modify the percpu var contents, which again
> could result in loss of information.
>