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

[Jan Beulich <jbeulich@suse.com>]

On 14.09.2019 10:52, Juergen Gross wrote:
> @@ -508,25 +515,27 @@ 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 )
> +    /* TODO: fix array size with multiple vcpus per unit. */
> +    unit_priv = xzalloc_array(void *, d->max_vcpus);
> +    if ( unit_priv == NULL )
>      {
>          sched_free_domdata(c->sched, domdata);
>          return -ENOMEM;
>      }
>  
> -    for_each_vcpu ( d, v )
> +    unit_idx = 0;
> +    for_each_sched_unit ( d, unit )
>      {
> -        vcpu_priv[v->vcpu_id] = sched_alloc_vdata(c->sched, v->sched_unit,
> -                                                  domdata);
> -        if ( vcpu_priv[v->vcpu_id] == NULL )
> +        unit_priv[unit_idx] = sched_alloc_vdata(c->sched, unit, domdata);
> +        if ( unit_priv[unit_idx] == NULL )
>          {
> -            for_each_vcpu ( d, v )
> -                xfree(vcpu_priv[v->vcpu_id]);
> -            xfree(vcpu_priv);
> +            for ( unit_idx = 0; unit_priv[unit_idx]; unit_idx++ )
> +                sched_free_vdata(c->sched, unit_priv[unit_idx]);

This is an unexpected change from xfree() to sched_free_vdata(). If
it really is correct, it should be mentioned in the description. I
can see why this might be better from an abstract pov, but it's
questionable whether arinc653's update_schedule_vcpus() really wants
calling at this point (perhaps it does, as a653sched_alloc_vdata()
also calls it).

Josh, Robert: Besides this immediate aspect I also wonder whether
said call is correct to make outside of a sched_priv->lock'ed
region, when both other instances occur inside of one (and in one
case immediately before an unlock, i.e. if the lock wasn't needed
the two steps could well be re-ordered).

Finally, at this point, shouldn't the functions and hooks (already)
be named {alloc,free}_udata()?

> @@ -896,18 +929,22 @@ 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;
> +                /* Affinity settings of one vcpu are for the complete unit. */
> +                sched_set_affinity(unit->vcpu_list,
> +                                   unit->cpu_hard_affinity_saved, NULL);

Yet despite the comment the function gets passed a struct vcpu *,
and this doesn't look to change by the end of the series. Is there
a reason for this?

> @@ -950,17 +986,19 @@ 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 )
> +                /* TODO: multiple vcpus per unit. */
> +                if ( unit->vcpu_list->affinity_broken )

Why not sched_check_affinity_broken(unit)? Quite possibly this would
make the TODO item unnecessary?

> @@ -968,14 +1006,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) )

Didn't you agree that this can be had cheaper? Quite likely my v2
review remark was on a different instance, but the pattern ought
to be relatively simple to find in the entire series (and by the
end of the series there's one other instance in schedule.c ...

> @@ -988,17 +1027,18 @@ 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);
> +            /* TODO: multiple vcpus per unit. */
> +            vcpu_migrate_start(unit->vcpu_list);
>              unit_schedule_unlock_irqrestore(lock, flags, unit);
>  
> -            vcpu_migrate_finish(v);
> +            vcpu_migrate_finish(unit->vcpu_list);
>  
>              /*
>               * 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) )

... here; I didn't check other files).

> @@ -1009,8 +1049,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;

Unnecessary change?

Jan

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