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

[Alexander Gordeev]

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

> But that cannot happen(tm) :)
...

> > > +	/* Check if process has been migrated to a different CPU. */
> > > +	if (regs->cpu == lc->cpu_nr)
> > > +		return;
> > > +	/* Fixup percpu base register */
> > > +	regs->gprs[reg] -= __per_cpu_offset[regs->cpu];
> > > +	regs->gprs[reg] += lc->percpu_offset;
> > 
> > Such one reads bit better:
> > 
> > 	regs->gprs[reg] -= __per_cpu_offset[regs->cpu];
> > 	regs->gprs[reg] += __per_cpu_offset[reg];
> 
> That reads better, but it is wrong. 'reg' is not the index of the
> current cpu into __per_cpu_offset[].
> 
> It could be changed to
> 
> 	regs->gprs[reg] += __per_cpu_offset[lc->cpu_nr];
> 
> but then again that's exactly lc->percpu_offset. I don't see a reason
> to create worse code here.
> 
> > > + * 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'.

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

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