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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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?

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

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

Jan

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