[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.