Re: [Xen-devel] [PATCH v3 07/47] xen/sched: move per cpu scheduler private data into struct sched_resource

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

On 19.09.19 17:27, Jan Beulich wrote:
> On 14.09.2019 10:52, Juergen Gross wrote:
> > This prepares support of larger scheduling granularities, e.g. core
> > scheduling.
> >
> > While at it move sched_has_urgent_vcpu() from include/asm-x86/cpuidle.h
> > into sched.h removing the need for including sched-if.h in cpuidle.h.
> > For that purpose remobe urgent_count from the scheduler private data
> > and make it a plain percpu variable.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Fundamentally
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> but a couple of remarks:
>
> > @@ -643,7 +643,7 @@ static spinlock_t *
> >   a653_switch_sched(struct scheduler *new_ops, unsigned int cpu,
> >                     void *pdata, void *vdata)
> >   {
> > -    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
> > +    struct sched_resource *sd = get_sched_res(cpu);
> I can understand why you keep "sd" as a name here and in similar
> cases. But ...
>
> > @@ -3881,6 +3881,7 @@ csched2_switch_sched(struct scheduler *new_ops, unsigned 
> > int cpu,
> >   {
> >       struct csched2_private *prv = csched2_priv(new_ops);
> >       struct csched2_unit *svc = vdata;
> > +    struct sched_resource *sd = get_sched_res(cpu);
> ... here (and in at least one more place) you introduce a new
> variable. Wouldn't this better be named e.g. "sr"? Furthermore
> you use it just once ...
>
> > @@ -3906,7 +3907,7 @@ csched2_switch_sched(struct scheduler *new_ops, unsigned 
> > int cpu,
> >        * this scheduler, and so it's safe to have taken it /before/ our
> >        * private global lock.
> >        */
> > -    ASSERT(per_cpu(schedule_data, cpu).schedule_lock != &prv->rqd[rqi].lock);
> > +    ASSERT(sd->schedule_lock != &prv->rqd[rqi].lock);
> ... here; it doesn't seem worthwhile here, but I guess
> subsequent changes make more use of it?
In fact they don't. I'll remove it here.

Regarding to rename "sd" to "sr": I agree this would be a sensible
change. OTOH I'd like to be consistent, so I'd like to defer that to
the planned scheduling cleanup series.

> > @@ -393,7 +395,7 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
> >       /* Idle VCPUs are scheduled immediately, so don't put them in runqueue. */
> >       if ( is_idle_domain(d) )
> >       {
> > -        per_cpu(schedule_data, v->processor).curr = unit;
> > +        get_sched_res(v->processor)->curr = unit;
> As long as it's a macro (see below), why not use curr_on_cpu() here?
There will be another sched_resource initialization added here
later. This makes it more obvious.

> > @@ -1916,7 +1917,7 @@ void __init scheduler_init(void)
> >       idle_domain->max_vcpus = nr_cpu_ids;
> >       if ( vcpu_create(idle_domain, 0, 0) == NULL )
> >           BUG();
> > -    this_cpu(schedule_data).curr = idle_vcpu[0]->sched_unit;
> > +    get_sched_res(0)->curr = idle_vcpu[0]->sched_unit;
> Hmm, yet another plain 0. But yes, there are quite a few of them
> here already, so one more doesn't really matter.
Yes, we should add a boot_cpu variable. But not in this series.

> > --- a/xen/include/xen/sched-if.h
> > +++ b/xen/include/xen/sched-if.h
> > @@ -33,22 +33,19 @@ extern int sched_ratelimit_us;
> >    * For cache betterness, keep the actual lock in the same cache area
> >    * as the rest of the struct.  Just have the scheduler point to the
> >    * one it wants (This may be the one right in front of it).*/
> > -struct schedule_data {
> > +struct sched_resource {
> >       spinlock_t         *schedule_lock,
> >                          _lock;
> >       struct sched_unit  *curr;
> >       void               *sched_priv;
> >       struct timer        s_timer;        /* scheduling timer                */
> > -    atomic_t            urgent_count;   /* how many urgent vcpus           */
> > -};
> > -
> > -#define curr_on_cpu(c)    (per_cpu(schedule_data, c).curr)
> >   
> > -struct sched_resource {
> > -    unsigned int master_cpu;  /* Cpu with lowest id in scheduling resource. */
> > +    /* Cpu with lowest id in scheduling resource. */
> > +    unsigned int        master_cpu;
> >   };
> >   
> > -DECLARE_PER_CPU(struct schedule_data, schedule_data);
> > +#define curr_on_cpu(c)    (get_sched_res(c)->curr)
> By moving this a few lines down if could become an inline function
> as it seems, if (see above) its use as an lvalue is not intended.
I like that idea. Will change to inline function.


Juergen

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