[PATCH 0/2] x86: mapcache_current_vcpu() adjustments

[PATCH 0/2] x86: mapcache_current_vcpu() adjustments

[Jan Beulich]

1: constify local variables in mapcache_current_vcpu()
2: replace this_cpu() uses in mapcache_current_vcpu()

The latter patch is somewhat RFC; see commentary there.

Jan

[PATCH 1/2] x86: constify local variables in mapcache_current_vcpu()

[Jan Beulich]

As of the referenced commit v won't be returned from the function anymore.
Yet that was the sole reason for it to not be pointer-to-const. curr,
which was introduced by that change, had no reason to be pointer-to-non-
const from the start.

Amends: 622c9a5ba95d ("x86/mm: accurately track which vCPU page-tables are loaded")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I was heavily tempted to also replace the smp_processor_id() at this
occasion (by curr->processor). With gcc16, that leads to the function
actually being inlined (in debug builds), i.e. a significant change in
generated code, and a measurable increase in code size. From the function
being declared "inline", that looks to have been the intention, yet then
that use of "inline" doesn't quite fit our principles. The main reasoning
for this as a secondary change would be:
- "curr" could be dropped (instead of having const added) if code
  generated from ASSERT() wouldn't matter quality-wise,
- if generated code quality matters there, then the unnecessary use of
  smp_processor_id() should matter, too.
However, an adjustment along these lines may better fit in a subsequent
change dealing with the repeated this_cpu() here.

--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -20,8 +20,8 @@
 
 static inline struct vcpu *mapcache_current_vcpu(void)
 {
-    struct vcpu *v = this_cpu(pgtable_vcpu);
-    struct vcpu *curr = current;
+    const struct vcpu *v = this_cpu(pgtable_vcpu);
+    const struct vcpu *curr = current;
 
     /*
      * During early boot pgtable_vcpu is not set, callers must handle NULL.

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

[Jan Beulich]

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@suse.com>
---
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.

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

--- 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;
 
     /*
@@ -37,9 +38,9 @@ static inline struct vcpu *mapcache_curr
      * change the page tables under our feet an invalidate any in-use mapcache
      * entries.
      */
-    if ( unlikely(this_cpu(curr_vcpu) != curr) )
+    if ( unlikely(per_cpu(curr_vcpu, cpu) != curr) )
     {
-        ASSERT(curr == idle_vcpu[smp_processor_id()]);
+        ASSERT(curr == idle_vcpu[cpu]);
         sync_local_execstate();
         /* We must now be running on the idle page table. */
         ASSERT(cr3_pa(read_cr3()) == __pa(idle_pg_table));
@@ -51,7 +52,7 @@ static inline struct vcpu *mapcache_curr
      * FLUSH_VCPU_STATE IPI has done so behind our back.  Use ACCESS_ONCE to
      * ensure the compiler never returns the locally cached pgtable_vcpu value.
      */
-    return ACCESS_ONCE(this_cpu(pgtable_vcpu));
+    return ACCESS_ONCE(per_cpu(pgtable_vcpu, cpu));
 }
 
 #define mapcache_l2_entry(e) ((e) >> PAGETABLE_ORDER)

Re: [PATCH 1/2] x86: constify local variables in mapcache_current_vcpu()

[Roger Pau Monné]

On Tue, Oct 13, 2026 at 11:05:44AM +0200, Jan Beulich wrote:
> As of the referenced commit v won't be returned from the function anymore.
> Yet that was the sole reason for it to not be pointer-to-const. curr,
> which was introduced by that change, had no reason to be pointer-to-non-
> const from the start.
> 
> Amends: 622c9a5ba95d ("x86/mm: accurately track which vCPU page-tables are loaded")

I think I recall how this ended the way it is: initially I was
returning v, which was wrong because this_cpu(pgtable_vcpu) will have
changed after the sync_local_execstate().  Later I adjusted the return
value of the function to be this_cpu(pgtable_vcpu), but didn't made v
const.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

> ---
> I was heavily tempted to also replace the smp_processor_id() at this
> occasion (by curr->processor). With gcc16, that leads to the function
> actually being inlined (in debug builds), i.e. a significant change in
> generated code, and a measurable increase in code size. From the function
> being declared "inline", that looks to have been the intention, yet then
> that use of "inline" doesn't quite fit our principles. The main reasoning
> for this as a secondary change would be:
> - "curr" could be dropped (instead of having const added) if code
>   generated from ASSERT() wouldn't matter quality-wise,
> - if generated code quality matters there, then the unnecessary use of
>   smp_processor_id() should matter, too.
> However, an adjustment along these lines may better fit in a subsequent
> change dealing with the repeated this_cpu() here.

We might want to introduce a local cpu variable to hold
curr->processor and then replace the this_cpu() with per_cpu(),
together with removing the usage of smp_processor_id().  Shouldn't
make the code more complicated IMO.

Thanks, Roger.

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

[Roger Pau Monné]

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@suse.com>

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.

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.

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

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

Thanks, Roger.

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

[Jan Beulich]

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@suse.com>
> 
> 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