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

[Juergen Gross <jgross@xxxxxxxx>]

On 02.09.19 11:07, Jan Beulich wrote:
> 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.
I think const is fine.

> > @@ -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.
Will change.

> > @@ -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.
Yes.

> > --- 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.
Right.

> > @@ -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)?
I just copied the vcpu_id type. Will change to unsigned int.


Juergen


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