Re: [Xen-devel] [PATCH v2 24/48] xen: switch from for_each_vcpu() to for_each_sched_unit()

[Juergen Gross]

On 09.09.19 17:14, Jan Beulich wrote:

On 09.08.2019 16:58, Juergen Gross wrote:

@@ -504,22 +511,21 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
      if ( IS_ERR(domdata) )
          return PTR_ERR(domdata);

- vcpu_priv = xzalloc_array(void *, d->max_vcpus);

-    if ( vcpu_priv == NULL )
+    unit_priv = xzalloc_array(void *, d->max_vcpus);

I find it confusing that an array of units (as per the use below)
is dimensioned by the domain's vCPU count. Isn't there a correlation
between vCPU IDs and units IDs, perhaps along the lines of CPU
APIC (thread), core, and socket IDs? If so, the array size could
be bounded here by a smaller (down the road) value.

I'll add a comment in this regard in this patch and when the number
of vcpus per unit gets added (patch 31) I'll modify the array size.

@@ -880,18 +889,36 @@ void vcpu_force_reschedule(struct vcpu *v)
      vcpu_migrate_finish(v);
  }

+static bool sched_check_affinity_broken(struct sched_unit *unit)

const

Okay.

+{
+    struct vcpu *v;

const

Okay.

@@ -910,18 +937,20 @@ void restore_vcpu_affinity(struct domain *d)
                      cpupool_domain_cpumask(d));
          if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
          {
-            if ( v->affinity_broken )
+            if ( sched_check_affinity_broken(unit) )
              {
-                sched_set_affinity(v, unit->cpu_hard_affinity_saved, NULL);
-                v->affinity_broken = 0;
+                sched_set_affinity(unit->vcpu_list,
+                                   unit->cpu_hard_affinity_saved, NULL);
+                sched_reset_affinity_broken(unit);
                  cpumask_and(cpumask_scratch_cpu(cpu), unit->cpu_hard_affinity,
                              cpupool_domain_cpumask(d));
              }

if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )

              {
-                printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
-                sched_set_affinity(v, &cpumask_all, NULL);
+                printk(XENLOG_DEBUG "Breaking affinity for %pv\n",
+                       unit->vcpu_list);
+                sched_set_affinity(unit->vcpu_list, &cpumask_all, NULL);
                  cpumask_and(cpumask_scratch_cpu(cpu), unit->cpu_hard_affinity,
                              cpupool_domain_cpumask(d));
              }
[...]> @@ -964,17 +992,18 @@ int cpu_disable_scheduler(unsigned int cpu)

for_each_domain_in_cpupool ( d, c )

      {
-        for_each_vcpu ( d, v )
+        struct sched_unit *unit;
+
+        for_each_sched_unit ( d, unit )
          {
              unsigned long flags;
-            struct sched_unit *unit = v->sched_unit;
              spinlock_t *lock = unit_schedule_lock_irqsave(unit, &flags);

cpumask_and(&online_affinity, unit->cpu_hard_affinity, c->cpu_valid);

              if ( cpumask_empty(&online_affinity) &&
                   cpumask_test_cpu(cpu, unit->cpu_hard_affinity) )
              {
-                if ( v->affinity_broken )
+                if ( unit->vcpu_list->affinity_broken )
                  {
                      /* The vcpu is temporarily pinned, can't move it. */
                      unit_schedule_unlock_irqrestore(lock, flags, unit);
@@ -982,14 +1011,15 @@ int cpu_disable_scheduler(unsigned int cpu)
                      break;
                  }

- printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);

+                printk(XENLOG_DEBUG "Breaking affinity for %pv\n",
+                       unit->vcpu_list);

- sched_set_affinity(v, &cpumask_all, NULL);

+                sched_set_affinity(unit->vcpu_list, &cpumask_all, NULL);
              }

- if ( v->processor != cpu )

+            if ( sched_unit_cpu(unit) != sched_get_resource_cpu(cpu) )
              {
-                /* The vcpu is not on this cpu, so we can move on. */
+                /* The unit is not on this cpu, so we can move on. */
                  unit_schedule_unlock_irqrestore(lock, flags, unit);
                  continue;
              }
@@ -1002,17 +1032,17 @@ int cpu_disable_scheduler(unsigned int cpu)
               *  * the scheduler will always find a suitable solution, or
               *    things would have failed before getting in here.
               */
-            vcpu_migrate_start(v);
+            vcpu_migrate_start(unit->vcpu_list);
              unit_schedule_unlock_irqrestore(lock, flags, unit);

- vcpu_migrate_finish(v);

+            vcpu_migrate_finish(unit->vcpu_list);

All the ->vcpu_list references look bogus considering where you're
moving, but I can only guess that all of this will need touching
again later in the series. I wonder though whether these wouldn't
better change into for-each-vCPU-in-unit loops right away.

Especially the vcpu_migrate part is more complicated. I think it is
much easier to review with the more mechanical changes split from the
logical changes.

              /*
               * The only caveat, in this case, is that if a vcpu active in
               * the hypervisor isn't migratable. In this case, the caller
               * should try again after releasing and reaquiring all locks.
               */
-            if ( v->processor == cpu )
+            if ( sched_unit_cpu(unit) == sched_get_resource_cpu(cpu) )

Is comparing the (pseudo) CPU values here the most efficient approach
generated code wise? Can't there be some pointer comparison that's
cheaper?

Yes, you are right. unit->res == get_sched_res(cpu) is equivalent.

@@ -1023,8 +1053,8 @@ int cpu_disable_scheduler(unsigned int cpu)
  static int cpu_disable_scheduler_check(unsigned int cpu)
  {
      struct domain *d;
-    struct vcpu *v;
      struct cpupool *c;
+    struct vcpu *v;

c = per_cpu(cpupool, cpu);

      if ( c == NULL )

Stray change?

Yes.


Juergen

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