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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2019 16:02, Juergen Gross wrote:
> On 09.09.19 17:14, Jan Beulich wrote:
>> On 09.08.2019 16:58, Juergen Gross wrote:
>>> @@ -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.

Yes, and I appreciate you separating mechanical from logical changes.
However, as already pointed out in the context where I had convinced
you of using "vcpu_list" as a name, individual actions on vcpu_list
now look bogus throughout the series. They should really (almost?)
all be loops over the entire list; I have a hard time imagining
possible exceptions, but I'm not going to exclude there may be one
or a few. Introducing such loops should, as long as there's only
ever on vCPU on such a list, too be a mostly mechanical step, which
imo should have happened before (or with) changes like this one.

Jan

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