Re: [Xen-devel] [PATCH] xen: sched: don't call hooks of the wrong scheduler via VCPU2OP

[Juergen Gross <jgross@xxxxxxxx>]

On 16/03/17 22:30, Dario Faggioli wrote:
> Within context_saved(), we call the context_saved hook,
> and we use VCPU2OP() to determine from what scheduler.
> VCPU2OP uses DOM2OP, which uses d->cpupool, which is
> NULL when d is the idle domain. And in that case,
> DOM2OP just returns ops, the scheduler of cpupool0.
> 
> Therefore, if:
> - cpupool0's scheduler defines context_saved (like
>   Credit2 and RTDS do),
> - we are not in cpupool0 (i.e., our scheduler is
>   not ops),
> - we are context switching from idle,
> 
> we call VCPU2OP(idle_vcpu), which means
> DOM2OP(idle->cpupool), which is ops.
> 
> Therefore, we both:
> - check if context_saved is defined in the wrong
>   scheduler;
> - if yes, call the wrong one.
> 
> When using Credit2 at boot, and also Credit2 in
> the other cpupool, this is wrong but innocuous,
> because it only involves the idle vcpus.
> 
> When using Credit2 at boot, and Credit1 in the
> other cpupool, this is *totally* wrong, and
> it's by chance it does not explode!
> 
> When using Credit2 and other schedulers I'm
> developping, I hit the following assert (in
> sched_credit2.c, on a CPU inside a cpupool that
> does not use Credit2):
> 
> csched2_context_saved()
> {
>  ...
>  ASSERT(!vcpu_on_runq(svc));
>  ...
> }
> 
> Fix this by taking care, in VCPU2OP, of the case
> when the vcpu is an idle one.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

... with or without the remark below addressed.

> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Cc-ing Jan, as this should be backported at least to 4.8, but, IMO, as back as
> possible.
> ---
>  xen/common/schedule.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 223a120..d12f346 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -78,7 +78,19 @@ static struct scheduler __read_mostly ops;
>            : (typeof((opsptr)->fn(opsptr, ##__VA_ARGS__)))0 )
>  
>  #define DOM2OP(_d)    (((_d)->cpupool == NULL) ? &ops : 
> ((_d)->cpupool->sched))
> -#define VCPU2OP(_v)   (DOM2OP((_v)->domain))
> +static inline struct scheduler* VCPU2OP(const struct vcpu *v)

Rename, e.g. get_scheduler() ?

> +{
> +    struct domain *d = v->domain;
> +
> +    if ( likely(d->cpupool != NULL) )
> +        return d->cpupool->sched;
> +
> +    /* v->processor never changes for idle vcpus, so using it here is safe */
> +    if ( likely(is_idle_domain(d)) )
> +        return per_cpu(scheduler, v->processor);
> +    else
> +        return &ops;
> +}
>  #define VCPU2ONLINE(_v) cpupool_domain_cpumask((_v)->domain)
>  
>  static inline void trace_runstate_change(struct vcpu *v, int new_state)


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel