Re: [PATCH 2/2] x86: replace this_cpu() uses in mapcache_current_vcpu()

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, Oct 29, 2026 at 1:56 PM George Dunlap <dunlapg@xxxxxxxxx> wrote:
>
> On Tue, Oct 13, 2026 at 12:52 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >> Like indicated in the earlier patch, this similarly leads to the function
> > >> actually being inlined (and hence generated code size actually growing) 
> > >> in
> > >> gcc16 debug builds. In release builds (with older gcc) the function is
> > >> being inlined no matter what. Overall, having looked at generated code
> > >> differences, I'm not quite sure anymore whether the earlier observed
> > >> inefficiency with repeated this_cpu() really is much of an issue (here or
> > >> maybe even more generally).
> > >
> > > I think we sometimes focus too much on this kind of
> > > micro-optimizations when the cost of the operation itself is orders of
> > > magnitude bigger than any performance difference the optimization
> > > might cause.
> >
> > Well, any improvement we can reasonably (and safely) make counts. Even
> > if not measurable performance-wise, it may still make a difference
> > (also in power consumption) when accumulated over long enough a period.
>
> If we had an infinite amount of developer and reviewer attention, this
> would be true; if for instance we could have LLMs just review and
> check in patches they were confident of.  But reviewer bandwidth is in
> fact a major bottleneck; the fact that Roger and I are looking at this
> patch now means we're not looking at some other patch.
>
> > > In this case I don't think the resulting code is bad or harder to
> > > read, so I don't mind adjusting the logic.  But that's a question of
> > > taste, if someone else has a preference for the original form I
> > > wouldn't change it.
> >
> > I'll give it some time for comments to that (or any other) effect.
>
> FWIW, having already looked at the patch, I don't mind either way.
> "this_cpu()" is slightly more readable; one could argue that
> per_cpu(foo, cpu) sort of implies that it could be a *different* cpu
> -- at very least someone reading the code has to go check at least
> once.  And again, human cognitive effort is the main bottleneck for a
> correct, feature-ful, and performant hypervisor.  But both the
> potential efficiency win, and the potential cognitive load, are so
> small that I don't feel like it's a clear win either way.
>
> So I would leave it off, but if you'd rather:
>
> Reviewed-by: George Dunlap <gwd@xxxxxxxxxxxxxx>

That said, Claude's analysis of the generated code doesn't give any
benefits.  Its take below.

 -George

It saves no work in the generated code (gcc 16.2.0, staging 41ddcfe808):

- Why: is_pv_vcpu() → evaluate_nospec() is an asm volatile with a
"memory" clobber. So the per-CPU offset is reloaded after it either
way, whether it comes from cpu_info or from __per_cpu_offset[cpu].
Latching cpu only saves the %rsp arithmetic, and costs the
processor_id load.

- Release builds: the same number of loads on the common path, one
instruction fewer, and map_domain_page/unmap_domain_page grow by 16
and 21 bytes. In release builds this code runs only for frames above 5
TiB anyway, because of the NDEBUG shortcut.

- Debug builds: gcc inlines the function into both callers (.text +384
bytes, as Jan said), and the common path gains one load.

So, as Roger said, it’s a matter of taste; the generated code gives no
reason to take it.