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

[Juergen Gross <jgross@suse.com>]

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