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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> +{
> +    struct vcpu *v;

const

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

>              /*
>               * 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?

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

Jan

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