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

[George Dunlap <dunlapg@xxxxxxxxx>]

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>

> >> --- a/xen/arch/x86/domain_page.c
> >> +++ b/xen/arch/x86/domain_page.c
> >> @@ -20,7 +20,8 @@
> >>
> >>  static inline struct vcpu *mapcache_current_vcpu(void)
> >>  {
> >> -    const struct vcpu *v = this_cpu(pgtable_vcpu);
> >> +    unsigned int cpu = smp_processor_id();
> >> +    const struct vcpu *v = per_cpu(pgtable_vcpu, cpu);
> >>      const struct vcpu *curr = current;
> >
> > Maybe you can reorder the variables here:
> >
> > const struct vcpu *curr = current;
> > unsigned int cpu = curr->processor;
> > const struct vcpu *v = per_cpu(pgtable_vcpu, cpu);
>
> The use of curr->processor in the remark in patch 1 was safe afaict.
> I'm not fully sure the one here would be safe: It would require
> thorough auditing of scheduler code to be sure, I think. (I don't
> _think_ ->processor would be altered from a remote CPU, but I'm not
> entirely certain.)

Claude points out that curr is never dereferenced, only compared; so
the code as it is works before `current` is set.  The first
map_domain_page() comes long before `current` is set in
init_idle_domain(), so Roger's version crashes during boot (Claude
verified this).

 -George