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.kai...@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 0diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.hindex 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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
