[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 24/47] xen: switch from for_each_vcpu() to for_each_sched_unit()
- To: Jan Beulich <jbeulich@xxxxxxxx>, Joshua Whitehead <josh.whitehead@xxxxxxxxxxxxxxx>, Robert VanVossen <Robert.VanVossen@xxxxxxxxxxxxxxx>
- From: Jürgen Groß <jgross@xxxxxxxx>
- Date: Tue, 24 Sep 2019 14:13:02 +0200
- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Julien Grall <julien.grall@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Dario Faggioli <dfaggioli@xxxxxxxx>
- Delivery-date: Tue, 24 Sep 2019 12:13:12 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 20.09.19 17:05, Jan Beulich wrote:
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).
Yes, I should put this change into an own patch at the beginning
of the series (or outside of it), as it is really a bug fix.
The data allocated via sched_alloc_vdata() is put into a list by the
arinc scheduler, so just xfree()ing it is clearly wrong.
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).
I guess a653sched_free_vdata() is missing the required locking.
I'll add that in the bugfix.
Finally, at this point, shouldn't the functions and hooks (already)
be named {alloc,free}_udata()?
Indeed they should.
@@ -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?
Yes. sched_set_affinity() is used from outside of schedule.c (by
dom0_build.c).
@@ -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?
Oh, indeed.
@@ -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 ...
Will check again. Thanks for the reminder.
@@ -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?
Yes.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
