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

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxxx>
> ---
>   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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature