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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2026 12:15, Roger Pau Monné wrote:
> On Tue, Oct 13, 2026 at 11:06:27AM +0200, Jan Beulich wrote:
>> We have accumulated three of them, plus a hidden similar 4th construct
>> from smp_processor_id(). Latch the result of the latter into a local
>> variable and use the slightly more efficient per_cpu() instead.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Oh, so that's kind of what I've suggested in the previous patch.
> 
>> ---
>> A 5th such construct is "current", yet to replace that we'd need to open-
>> code both it (i.e. get_current()) and then also smp_processor_id(). I
>> wasn't quite sure whether that would be deemed acceptable.
> 
> See below, we can reorder and use the processor ID value stashed in
> current?
> 
>> 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.

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

>> --- 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.)

> So that we avoid two smp_processor_id() calls (ie: the hidden one in
> current).

Strictly speaking there's none hidden in current. That accesses a
different cpu_info field.

Jan