[PATCH 1/2] sched/core: For a new metric, add vcpu->nonaffine_time

[PATCH 1/2] sched/core: For a new metric, add vcpu->nonaffine_time

[Bernhard Kaindl]

To monitor the effectiveness of vCPU soft-affinity on NUMA hosts,
we'd like to create a vCPU metric that accumulates the amount of
vCPU time running outside of the soft affinity mask of the sched-unit:

- Add a new time counter, nonaffine_time to struct vcpu.

- Accumulate the nonaffine_time on vcpu_runstate_change():
  Account the time spent in the RUNSTATE_running state outside
  of unit->cpu_soft_affinity: It is always initialized and defaults
  to cpumask_all (bits for all NR_CPUS set), so we only accumulate
  nonaffine time when the vCPU runs on an unset CPU (non-affine).

In the next patch, this field can be used to retrieve the accumulated
nonaffine running time e.g. using vcpu_runstate_get().

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
---
 xen/common/sched/core.c | 20 ++++++++++++++++++++
 xen/include/xen/sched.h | 11 +++++++++++
 2 files changed, 31 insertions(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 13fdf57e57..489255b9c6 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -260,6 +260,23 @@ static inline void vcpu_urgent_count_update(struct vcpu *v)
     }
 }
 
+/*
+ * For accounting non-affine running time of a vCPU, return true if
+ * the vCPU is running in RUNSTATE_running state while not on a CPU
+ * in unit->cpu_soft_affinity.
+ */
+static inline bool nonaffine(const struct vcpu *v,
+                             const struct sched_unit *unit)
+{
+    /*
+     * unit->cpu_soft_affinity is always initialized and defaults to
+     * cpumask_all (bits for all NR_CPUS set), so we only accumulate
+     * nonaffine time when the vCPU runs on an unset CPU (non-affine).
+     */
+    return v->runstate.state == RUNSTATE_running &&
+           !cpumask_test_cpu(v->processor, unit->cpu_soft_affinity);
+}
+
 static inline void vcpu_runstate_change(
     struct vcpu *v, int new_state, s_time_t new_entry_time)
 {
@@ -285,6 +302,9 @@ static inline void vcpu_runstate_change(
     {
         v->runstate.time[v->runstate.state] += delta;
         v->runstate.state_entry_time = new_entry_time;
+
+        if ( nonaffine(v, unit) ) /* When running nonaffine, add delta */
+            v->nonaffine_time += delta;
     }
 
     v->runstate.state = new_state;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index fe53d4fab7..aba60afd4f 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -198,7 +198,18 @@ struct vcpu
 
     struct sched_unit *sched_unit;
 
+    /*
+     * The struct vcpu_runstate_info contains the vCPU time spent
+     * in each runstate and the entry time of the current runstate.
+     *
+     * Note: This field is used for the guest runstate shared memory area.
+     * Therefore, it is part of the frozen guest API and cannot be changed.
+     */
     struct vcpu_runstate_info runstate;
+
+    /* vCPU time running outside the scheduling unit's soft_affinity mask */
+    uint64_t         nonaffine_time;
+
 #ifndef CONFIG_COMPAT
 # define runstate_guest(v) ((v)->runstate_guest)
     XEN_GUEST_HANDLE(vcpu_runstate_info_t) runstate_guest; /* guest address */
-- 
2.43.0

[PATCH 2/2] sched/core: Update vcpu_runstate_get() to return nonaffine time

[Bernhard Kaindl]

Update vcpu_runstate_get() to return a snapshot of the accumulated
non-affine vCPU running time at the current time of this call.

We cannot change the struct vcpu_runstate_info: It is part of the
Guest shared memory area that is part of the frozen VM ABI.

Instead return the new value: This way we do not have to change all
old callers to pass a NULL in place of it, and we also we don't want
an internal shadow struct that we memcpy from with sizeof(). To be open
open to return data in the future, return a struct with the new field.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
---
 xen/common/sched/core.c   | 26 ++++++++++++++++++++++++--
 xen/include/public/vcpu.h | 10 ++++++++++
 xen/include/xen/sched.h   |  4 ++--
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 489255b9c6..319bd7a928 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -324,12 +324,25 @@ void sched_guest_idle(void (*idle) (void), unsigned int cpu)
     atomic_dec(&per_cpu(sched_urgent_count, cpu));
 }
 
-void vcpu_runstate_get(const struct vcpu *v,
-                       struct vcpu_runstate_info *runstate)
+/**
+ * vcpu_runstate_get(): Return vCPU time spent in different runstates
+ *
+ * @param v:        vCPU to get runstate times (since vCPU start)
+ * @param runstate: Return time spent in each runstate.
+ *                  This structure is part of the runstate memory areas
+ *                  shared with the domains which is part of the ABI
+ *                  with domains that is frozen and cannot be changed.
+ *                  To return additional values, use e.g. the return
+ *                  value(no need to change all callers) of this function.
+ * @returns         struct with non-affine running time since vcpu creation
+ */
+vcpu_runstate_extra_t vcpu_runstate_get(const struct vcpu *v,
+                                        struct vcpu_runstate_info *runstate)
 {
     spinlock_t *lock;
     s_time_t delta;
     struct sched_unit *unit;
+    vcpu_runstate_extra_t ret;
 
     rcu_read_lock(&sched_res_rculock);
 
@@ -343,14 +356,23 @@ void vcpu_runstate_get(const struct vcpu *v,
                            : v->sched_unit;
     lock = likely(v == current) ? NULL : unit_schedule_lock_irq(unit);
     memcpy(runstate, &v->runstate, sizeof(*runstate));
+    ret.nonaffine_time = v->nonaffine_time; /* accumulated nonaffine time */
+
     delta = NOW() - runstate->state_entry_time;
     if ( delta > 0 )
+    {
         runstate->time[runstate->state] += delta;
 
+        if ( nonaffine(v, unit) ) /* When running nonaffine, add the delta */
+            ret.nonaffine_time += delta;
+    }
+
     if ( unlikely(lock != NULL) )
         unit_schedule_unlock_irq(lock, unit);
 
     rcu_read_unlock(&sched_res_rculock);
+
+    return ret;
 }
 
 uint64_t get_cpu_idle_time(unsigned int cpu)
diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
index f7445ac0b0..59e6647a24 100644
--- a/xen/include/public/vcpu.h
+++ b/xen/include/public/vcpu.h
@@ -79,8 +79,18 @@ struct vcpu_runstate_info {
     uint64_t time[4];
 };
 typedef struct vcpu_runstate_info vcpu_runstate_info_t;
+/* vcpu_runstate_info_t is in the Guest shared memory area (frozen ABI) */
 DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_t);
 
+/*
+ * Extra information returned from vcpu_runstate_get that is not part
+ * of the Guest shared memory area (not part of the frozen Guest ABI)
+ */
+struct vcpu_runstate_extra {
+    uint64_t nonaffine_time; /* Time running outside soft_affinity mask */
+};
+typedef struct vcpu_runstate_extra vcpu_runstate_extra_t;
+
 /* VCPU is currently running on a physical CPU. */
 #define RUNSTATE_running  0
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index aba60afd4f..4fdbbaea87 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1110,8 +1110,8 @@ int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity);
 int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
                          struct xen_domctl_vcpuaffinity *vcpuaff);
 
-void vcpu_runstate_get(const struct vcpu *v,
-                       struct vcpu_runstate_info *runstate);
+vcpu_runstate_extra_t vcpu_runstate_get(const struct vcpu *v,
+                                        struct vcpu_runstate_info *runstate);
 uint64_t get_cpu_idle_time(unsigned int cpu);
 void sched_guest_idle(void (*idle) (void), unsigned int cpu);
 void scheduler_enable(void);
-- 
2.43.0

Re: [PATCH 2/2] sched/core: Update vcpu_runstate_get() to return nonaffine time

[Jan Beulich]

On 21.07.2025 11:49, Bernhard Kaindl wrote:
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -79,8 +79,18 @@ struct vcpu_runstate_info {
>      uint64_t time[4];
>  };
>  typedef struct vcpu_runstate_info vcpu_runstate_info_t;
> +/* vcpu_runstate_info_t is in the Guest shared memory area (frozen ABI) */
>  DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_t);

Personally I'm opposed to this kind of comment.

> +/*
> + * Extra information returned from vcpu_runstate_get that is not part
> + * of the Guest shared memory area (not part of the frozen Guest ABI)
> + */
> +struct vcpu_runstate_extra {
> +    uint64_t nonaffine_time; /* Time running outside soft_affinity mask */
> +};
> +typedef struct vcpu_runstate_extra vcpu_runstate_extra_t;

And the next time we need some further piece of statistics, we'll have
to add yet another new sub-hypercall? Or wait, there is no new sub-
hypercall here. How's this piece of data going to make it out to guest
space then?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1110,8 +1110,8 @@ int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity);
>  int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
>                           struct xen_domctl_vcpuaffinity *vcpuaff);
>  
> -void vcpu_runstate_get(const struct vcpu *v,
> -                       struct vcpu_runstate_info *runstate);
> +vcpu_runstate_extra_t vcpu_runstate_get(const struct vcpu *v,
> +                                        struct vcpu_runstate_info *runstate);

As long as it's only a single 64-bit field in the struct, returning this
by value may be okay(ish). Yet I'd still recommend against doing so.

Also, having reached the end of the patch: Where's the caller making use
of this new return value?

Jan

Re: [PATCH 2/2] sched/core: Update vcpu_runstate_get() to return nonaffine time

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 5630 bytes --]

On 21.07.25 11:49, Bernhard Kaindl wrote:
> Update vcpu_runstate_get() to return a snapshot of the accumulated
> non-affine vCPU running time at the current time of this call.
> 
> We cannot change the struct vcpu_runstate_info: It is part of the
> Guest shared memory area that is part of the frozen VM ABI.
> 
> Instead return the new value: This way we do not have to change all
> old callers to pass a NULL in place of it, and we also we don't want
> an internal shadow struct that we memcpy from with sizeof(). To be open
> open to return data in the future, return a struct with the new field.

Double "open".

It is not clear to me how the non-affine vCPU running time returned by
vcpu_runstate_get() will be used afterwards, as you don't add any users
looking at the return value. I can hardly review the patch without this
information. Right now I'd NACK this series, as it seems to add dead code
only.

IOW: please include a/some patch(es) making use of that code.

> 
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
> ---
>   xen/common/sched/core.c   | 26 ++++++++++++++++++++++++--
>   xen/include/public/vcpu.h | 10 ++++++++++
>   xen/include/xen/sched.h   |  4 ++--
>   3 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 489255b9c6..319bd7a928 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -324,12 +324,25 @@ void sched_guest_idle(void (*idle) (void), unsigned int cpu)
>       atomic_dec(&per_cpu(sched_urgent_count, cpu));
>   }
>   
> -void vcpu_runstate_get(const struct vcpu *v,
> -                       struct vcpu_runstate_info *runstate)
> +/**
> + * vcpu_runstate_get(): Return vCPU time spent in different runstates
> + *
> + * @param v:        vCPU to get runstate times (since vCPU start)
> + * @param runstate: Return time spent in each runstate.
> + *                  This structure is part of the runstate memory areas
> + *                  shared with the domains which is part of the ABI
> + *                  with domains that is frozen and cannot be changed.
> + *                  To return additional values, use e.g. the return
> + *                  value(no need to change all callers) of this function.
> + * @returns         struct with non-affine running time since vcpu creation
> + */
> +vcpu_runstate_extra_t vcpu_runstate_get(const struct vcpu *v,
> +                                        struct vcpu_runstate_info *runstate)

Returning a struct by value is limiting the possibility to expand this
struct later.

I don't think it is a good idea to do it this way.

>   {
>       spinlock_t *lock;
>       s_time_t delta;
>       struct sched_unit *unit;
> +    vcpu_runstate_extra_t ret;
>   
>       rcu_read_lock(&sched_res_rculock);
>   
> @@ -343,14 +356,23 @@ void vcpu_runstate_get(const struct vcpu *v,
>                              : v->sched_unit;
>       lock = likely(v == current) ? NULL : unit_schedule_lock_irq(unit);
>       memcpy(runstate, &v->runstate, sizeof(*runstate));
> +    ret.nonaffine_time = v->nonaffine_time; /* accumulated nonaffine time */
> +
>       delta = NOW() - runstate->state_entry_time;
>       if ( delta > 0 )
> +    {
>           runstate->time[runstate->state] += delta;
>   
> +        if ( nonaffine(v, unit) ) /* When running nonaffine, add the delta */
> +            ret.nonaffine_time += delta;
> +    }
> +
>       if ( unlikely(lock != NULL) )
>           unit_schedule_unlock_irq(lock, unit);
>   
>       rcu_read_unlock(&sched_res_rculock);
> +
> +    return ret;
>   }
>   
>   uint64_t get_cpu_idle_time(unsigned int cpu)
> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> index f7445ac0b0..59e6647a24 100644
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -79,8 +79,18 @@ struct vcpu_runstate_info {
>       uint64_t time[4];
>   };
>   typedef struct vcpu_runstate_info vcpu_runstate_info_t;
> +/* vcpu_runstate_info_t is in the Guest shared memory area (frozen ABI) */
>   DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_t);
>   
> +/*
> + * Extra information returned from vcpu_runstate_get that is not part
> + * of the Guest shared memory area (not part of the frozen Guest ABI)
> + */
> +struct vcpu_runstate_extra {
> +    uint64_t nonaffine_time; /* Time running outside soft_affinity mask */
> +};
> +typedef struct vcpu_runstate_extra vcpu_runstate_extra_t;
> +

Is it really needed to add this to the public header? This way you are
just adding another stable interface which can't be expanded easily.

>   /* VCPU is currently running on a physical CPU. */
>   #define RUNSTATE_running  0
>   
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index aba60afd4f..4fdbbaea87 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1110,8 +1110,8 @@ int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity);
>   int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
>                            struct xen_domctl_vcpuaffinity *vcpuaff);
>   
> -void vcpu_runstate_get(const struct vcpu *v,
> -                       struct vcpu_runstate_info *runstate);
> +vcpu_runstate_extra_t vcpu_runstate_get(const struct vcpu *v,
> +                                        struct vcpu_runstate_info *runstate);
>   uint64_t get_cpu_idle_time(unsigned int cpu);
>   void sched_guest_idle(void (*idle) (void), unsigned int cpu);
>   void scheduler_enable(void);


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] sched/core: For a new metric, add vcpu->nonaffine_time

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 4370 bytes --]

On 21.07.25 11:49, Bernhard Kaindl wrote:
> To monitor the effectiveness of vCPU soft-affinity on NUMA hosts,
> we'd like to create a vCPU metric that accumulates the amount of
> vCPU time running outside of the soft affinity mask of the sched-unit:
> 
> - Add a new time counter, nonaffine_time to struct vcpu.
> 
> - Accumulate the nonaffine_time on vcpu_runstate_change():
>    Account the time spent in the RUNSTATE_running state outside
>    of unit->cpu_soft_affinity: It is always initialized and defaults
>    to cpumask_all (bits for all NR_CPUS set), so we only accumulate
>    nonaffine time when the vCPU runs on an unset CPU (non-affine).
> 
> In the next patch, this field can be used to retrieve the accumulated
> nonaffine running time e.g. using vcpu_runstate_get().

Please avoid phrases like "in the next patch" in commit messages.
There is no guarantee a series will be committed in one go.

I'd just drop this last sentence.

> 
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
> ---
>   xen/common/sched/core.c | 20 ++++++++++++++++++++
>   xen/include/xen/sched.h | 11 +++++++++++
>   2 files changed, 31 insertions(+)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 13fdf57e57..489255b9c6 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -260,6 +260,23 @@ static inline void vcpu_urgent_count_update(struct vcpu *v)
>       }
>   }
>   
> +/*
> + * For accounting non-affine running time of a vCPU, return true if
> + * the vCPU is running in RUNSTATE_running state while not on a CPU
> + * in unit->cpu_soft_affinity.

"the vCPU is running in RUNSTATE_running state" is a weird statement.
When running it will always be in the RUNSTATE_running state. I'd write
"the vCPU is in RUNSTATE_running state".

> + */
> +static inline bool nonaffine(const struct vcpu *v,
> +                             const struct sched_unit *unit)
> +{
> +    /*
> +     * unit->cpu_soft_affinity is always initialized and defaults to
> +     * cpumask_all (bits for all NR_CPUS set), so we only accumulate
> +     * nonaffine time when the vCPU runs on an unset CPU (non-affine).
> +     */
> +    return v->runstate.state == RUNSTATE_running &&
> +           !cpumask_test_cpu(v->processor, unit->cpu_soft_affinity);
> +}
> +
>   static inline void vcpu_runstate_change(
>       struct vcpu *v, int new_state, s_time_t new_entry_time)
>   {
> @@ -285,6 +302,9 @@ static inline void vcpu_runstate_change(
>       {
>           v->runstate.time[v->runstate.state] += delta;
>           v->runstate.state_entry_time = new_entry_time;
> +
> +        if ( nonaffine(v, unit) ) /* When running nonaffine, add delta */
> +            v->nonaffine_time += delta;
>       }

Is this really correct? Imagine a vcpu running for very long time on
a physical cpu without losing it (RUNSTATE_running for minutes, hours
or even days). Now someone is changing the soft-affinity of the vcpu
and as a result it will be moved to another physical cpu. You will add
all the long time the vcpu was running to v->nonaffine_time in spite of
the affinity change having happened only nanoseconds before.

>       v->runstate.state = new_state;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index fe53d4fab7..aba60afd4f 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -198,7 +198,18 @@ struct vcpu
>   
>       struct sched_unit *sched_unit;
>   
> +    /*
> +     * The struct vcpu_runstate_info contains the vCPU time spent
> +     * in each runstate and the entry time of the current runstate.
> +     *
> +     * Note: This field is used for the guest runstate shared memory area.
> +     * Therefore, it is part of the frozen guest API and cannot be changed.
> +     */

s/frozen/public/

In the end I'm not really sure this comment is adding much value.
But maybe I'm biased as I've worked with this code a lot.

>       struct vcpu_runstate_info runstate;
> +
> +    /* vCPU time running outside the scheduling unit's soft_affinity mask */
> +    uint64_t         nonaffine_time;
> +
>   #ifndef CONFIG_COMPAT
>   # define runstate_guest(v) ((v)->runstate_guest)
>       XEN_GUEST_HANDLE(vcpu_runstate_info_t) runstate_guest; /* guest address */


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]