[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 12.09.19 16:40, Jan Beulich wrote:
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.

I think the easiest way to handle that is to add a comment like:

/* TODO: switch to for_each_sched_unit_vcpu() */

This will show the need for the loop without having to do logic
changes.


Juergen


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.