Re: [Xen-devel] [PATCH v2 01/48] xen/sched: use new sched_unit instead of vcpu in scheduler interfaces

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2019 16:57, Juergen Gross wrote:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -87,13 +87,13 @@ sched_idle_switch_sched(struct scheduler *new_ops, 
> unsigned int cpu,
>  }
>  
>  static int
> -sched_idle_cpu_pick(const struct scheduler *ops, struct vcpu *v)
> +sched_idle_cpu_pick(const struct scheduler *ops, struct sched_unit *unit)
>  {
> -    return v->processor;
> +    return unit->vcpu_list->processor;
>  }

Neither this nor any of the cpu_pick functions in the sched_*.c files
actually mean to modify "*unit", so unless later changes need this be
non-const I think it should get "const" added.

> @@ -308,9 +308,17 @@ static void sched_spin_unlock_double(spinlock_t *lock1, 
> spinlock_t *lock2,
>  int sched_init_vcpu(struct vcpu *v, unsigned int processor)
>  {
>      struct domain *d = v->domain;
> +    struct sched_unit *unit;
>  
>      v->processor = processor;
>  
> +    if ( (unit = xzalloc(struct sched_unit)) == NULL )
> +        return 1;
> +    v->sched_unit = unit;
> +    unit->vcpu_list = v;
> +    unit->unit_id = v->vcpu_id;
> +    unit->domain = d;

I guess it doesn't matter much since this will change quite a bit with
subsequent patches, but generally I'd consider it better to initialize
relevant structure fields first, before hooking it up the structure
itself.

> @@ -452,13 +465,17 @@ int sched_move_domain(struct domain *d, struct cpupool 
> *c)
>  
>  void sched_destroy_vcpu(struct vcpu *v)
>  {
> +    struct sched_unit *unit = v->sched_unit;
> +
>      kill_timer(&v->periodic_timer);
>      kill_timer(&v->singleshot_timer);
>      kill_timer(&v->poll_timer);
>      if ( test_and_clear_bool(v->is_urgent) )
>          atomic_dec(&per_cpu(schedule_data, v->processor).urgent_count);
> -    sched_remove_vcpu(vcpu_scheduler(v), v);
> +    sched_remove_unit(vcpu_scheduler(v), unit);
>      sched_free_vdata(vcpu_scheduler(v), v->sched_priv);
> +    xfree(unit);
> +    v->sched_unit = NULL;

Along the lines of the above, storing NULL would generally better be
done prior to actually freeing the pointed at structure.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -140,6 +140,7 @@ void evtchn_destroy(struct domain *d); /* from 
> domain_kill */
>  void evtchn_destroy_final(struct domain *d); /* from complete_domain_destroy 
> */
>  
>  struct waitqueue_vcpu;
> +struct sched_unit;

In C I don't think this is needed with ...

> @@ -160,6 +161,7 @@ struct vcpu
>  
>      struct timer     poll_timer;    /* timeout for SCHEDOP_poll */
>  
> +    struct sched_unit *sched_unit;

... this being ahead of any function prototypes using the struct.

> @@ -272,6 +274,12 @@ struct vcpu
>      struct arch_vcpu arch;
>  };
>  
> +struct sched_unit {
> +    struct domain         *domain;
> +    struct vcpu           *vcpu_list;
> +    int                    unit_id;

Any reason for this being plain int (rather than unsigned int)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel