| Message ID | 20190809145833.1020-28-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: add core scheduling support | expand |
On 09.08.2019 16:58, Juergen Gross wrote: > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -733,35 +733,40 @@ void vcpu_unblock(struct vcpu *v) > } > > /* > - * Do the actual movement of a vcpu from old to new CPU. Locks for *both* > + * Do the actual movement of an unit from old to new CPU. Locks for *both* > * CPUs needs to have been taken already when calling this! > */ > -static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu) > +static void sched_unit_move_locked(struct sched_unit *unit, > + unsigned int new_cpu) > { > - unsigned int old_cpu = v->processor; > + unsigned int old_cpu = unit->res->processor; > + struct vcpu *v; > > /* > * Transfer urgency status to new CPU before switching CPUs, as > * once the switch occurs, v->is_urgent is no longer protected by > * the per-CPU scheduler lock we are holding. > */ > - if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) ) > + for_each_sched_unit_vcpu ( unit, v ) > { > - atomic_inc(&get_sched_res(new_cpu)->urgent_count); > - atomic_dec(&get_sched_res(old_cpu)->urgent_count); > + if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) ) > + { > + atomic_inc(&get_sched_res(new_cpu)->urgent_count); > + atomic_dec(&get_sched_res(old_cpu)->urgent_count); > + } > } Shouldn't is_urgent become an attribute of unit rather than a vCPU, too, eliminating the need for a loop here? I can't see a reason why not, seeing this collapsing into a single urgent_count. Then again the question remains whether the non-deep sleeping as a result of a non-zero urgent_count should indeed be distributed to all constituents of a unit. I can see arguments both in favor and against. > -static void vcpu_migrate_finish(struct vcpu *v) > +static void sched_unit_migrate_finish(struct sched_unit *unit) > { > unsigned long flags; > unsigned int old_cpu, new_cpu; > spinlock_t *old_lock, *new_lock; > bool_t pick_called = 0; > + struct vcpu *v; > > /* > - * If the vcpu is currently running, this will be handled by > + * If the unit is currently running, this will be handled by > * context_saved(); and in any case, if the bit is cleared, then > * someone else has already done the work so we don't need to. > */ > - if ( v->sched_unit->is_running || > - !test_bit(_VPF_migrating, &v->pause_flags) ) > - return; > + for_each_sched_unit_vcpu ( unit, v ) > + { > + if ( unit->is_running || !test_bit(_VPF_migrating, &v->pause_flags) ) > + return; > + } Do you really need the loop invariant unit->is_running to be evaluated once per loop iteration? (Same again further down at least once.) Furthermore I wonder if VPF_migrating shouldn't become a per-unit attribute. > @@ -858,22 +871,30 @@ static void vcpu_migrate_finish(struct vcpu *v) > * because they both happen in (different) spinlock regions, and those > * regions are strictly serialised. > */ > - if ( v->sched_unit->is_running || > - !test_and_clear_bit(_VPF_migrating, &v->pause_flags) ) > + for_each_sched_unit_vcpu ( unit, v ) > { > - sched_spin_unlock_double(old_lock, new_lock, flags); > - return; > + if ( unit->is_running || > + !test_and_clear_bit(_VPF_migrating, &v->pause_flags) ) > + { > + sched_spin_unlock_double(old_lock, new_lock, flags); > + return; > + } > } > > - vcpu_move_locked(v, new_cpu); > + sched_unit_move_locked(unit, new_cpu); > > sched_spin_unlock_double(old_lock, new_lock, flags); > > if ( old_cpu != new_cpu ) > - sched_move_irqs(v->sched_unit); > + { > + for_each_sched_unit_vcpu ( unit, v ) > + sync_vcpu_execstate(v); This is new without being explained anywhere. Or wait, it is mentioned (with the wrong function name, which is why initially - by searching - I didn't spot it), but only with a justification of "needed anyway". > @@ -1794,7 +1814,7 @@ void context_saved(struct vcpu *prev) > > sched_context_saved(vcpu_scheduler(prev), prev->sched_unit); > > - vcpu_migrate_finish(prev); > + sched_unit_migrate_finish(prev->sched_unit); > } By the end of the series context_saved() still acts on vCPU-s, not units. What is the meaning/effect of multiple sched_unit_migrate_*()? Jan
On 10.09.19 17:11, Jan Beulich wrote: > On 09.08.2019 16:58, Juergen Gross wrote: >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -733,35 +733,40 @@ void vcpu_unblock(struct vcpu *v) >> } >> >> /* >> - * Do the actual movement of a vcpu from old to new CPU. Locks for *both* >> + * Do the actual movement of an unit from old to new CPU. Locks for *both* >> * CPUs needs to have been taken already when calling this! >> */ >> -static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu) >> +static void sched_unit_move_locked(struct sched_unit *unit, >> + unsigned int new_cpu) >> { >> - unsigned int old_cpu = v->processor; >> + unsigned int old_cpu = unit->res->processor; >> + struct vcpu *v; >> >> /* >> * Transfer urgency status to new CPU before switching CPUs, as >> * once the switch occurs, v->is_urgent is no longer protected by >> * the per-CPU scheduler lock we are holding. >> */ >> - if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) ) >> + for_each_sched_unit_vcpu ( unit, v ) >> { >> - atomic_inc(&get_sched_res(new_cpu)->urgent_count); >> - atomic_dec(&get_sched_res(old_cpu)->urgent_count); >> + if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) ) >> + { >> + atomic_inc(&get_sched_res(new_cpu)->urgent_count); >> + atomic_dec(&get_sched_res(old_cpu)->urgent_count); >> + } >> } > > Shouldn't is_urgent become an attribute of unit rather than a vCPU, > too, eliminating the need for a loop here? I can't see a reason > why not, seeing this collapsing into a single urgent_count. With moving urgent_count to a percpu variable this no longer applies. > > Then again the question remains whether the non-deep sleeping as > a result of a non-zero urgent_count should indeed be distributed > to all constituents of a unit. I can see arguments both in favor > and against. Against has won. :-) > >> -static void vcpu_migrate_finish(struct vcpu *v) >> +static void sched_unit_migrate_finish(struct sched_unit *unit) >> { >> unsigned long flags; >> unsigned int old_cpu, new_cpu; >> spinlock_t *old_lock, *new_lock; >> bool_t pick_called = 0; >> + struct vcpu *v; >> >> /* >> - * If the vcpu is currently running, this will be handled by >> + * If the unit is currently running, this will be handled by >> * context_saved(); and in any case, if the bit is cleared, then >> * someone else has already done the work so we don't need to. >> */ >> - if ( v->sched_unit->is_running || >> - !test_bit(_VPF_migrating, &v->pause_flags) ) >> - return; >> + for_each_sched_unit_vcpu ( unit, v ) >> + { >> + if ( unit->is_running || !test_bit(_VPF_migrating, &v->pause_flags) ) >> + return; >> + } > > Do you really need the loop invariant unit->is_running to be evaluated > once per loop iteration? (Same again further down at least once.) No, I should test that before entering the loop. > > Furthermore I wonder if VPF_migrating shouldn't become a per-unit > attribute. This would make vcpu_runnable() much more complicated. I don't think that is worth it. > >> @@ -858,22 +871,30 @@ static void vcpu_migrate_finish(struct vcpu *v) >> * because they both happen in (different) spinlock regions, and those >> * regions are strictly serialised. >> */ >> - if ( v->sched_unit->is_running || >> - !test_and_clear_bit(_VPF_migrating, &v->pause_flags) ) >> + for_each_sched_unit_vcpu ( unit, v ) >> { >> - sched_spin_unlock_double(old_lock, new_lock, flags); >> - return; >> + if ( unit->is_running || >> + !test_and_clear_bit(_VPF_migrating, &v->pause_flags) ) >> + { >> + sched_spin_unlock_double(old_lock, new_lock, flags); >> + return; >> + } >> } >> >> - vcpu_move_locked(v, new_cpu); >> + sched_unit_move_locked(unit, new_cpu); >> >> sched_spin_unlock_double(old_lock, new_lock, flags); >> >> if ( old_cpu != new_cpu ) >> - sched_move_irqs(v->sched_unit); >> + { >> + for_each_sched_unit_vcpu ( unit, v ) >> + sync_vcpu_execstate(v); > > This is new without being explained anywhere. Or wait, it is mentioned > (with the wrong function name, which is why initially - by searching - > I didn't spot it), but only with a justification of "needed anyway". I'll correct it and make it more verbose. > >> @@ -1794,7 +1814,7 @@ void context_saved(struct vcpu *prev) >> >> sched_context_saved(vcpu_scheduler(prev), prev->sched_unit); >> >> - vcpu_migrate_finish(prev); >> + sched_unit_migrate_finish(prev->sched_unit); >> } > > By the end of the series context_saved() still acts on vCPU-s, not > units. What is the meaning/effect of multiple sched_unit_migrate_*()? That's corrected in V3 by having split context_saved() into a vcpu- and a unit-part. Juergen
diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 4c488ddde0..e4d0dd4b65 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -733,35 +733,40 @@ void vcpu_unblock(struct vcpu *v) } /* - * Do the actual movement of a vcpu from old to new CPU. Locks for *both* + * Do the actual movement of an unit from old to new CPU. Locks for *both* * CPUs needs to have been taken already when calling this! */ -static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu) +static void sched_unit_move_locked(struct sched_unit *unit, + unsigned int new_cpu) { - unsigned int old_cpu = v->processor; + unsigned int old_cpu = unit->res->processor; + struct vcpu *v; /* * Transfer urgency status to new CPU before switching CPUs, as * once the switch occurs, v->is_urgent is no longer protected by * the per-CPU scheduler lock we are holding. */ - if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) ) + for_each_sched_unit_vcpu ( unit, v ) { - atomic_inc(&get_sched_res(new_cpu)->urgent_count); - atomic_dec(&get_sched_res(old_cpu)->urgent_count); + if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) ) + { + atomic_inc(&get_sched_res(new_cpu)->urgent_count); + atomic_dec(&get_sched_res(old_cpu)->urgent_count); + } } /* * Actual CPU switch to new CPU. This is safe because the lock * pointer can't change while the current lock is held. */ - sched_migrate(vcpu_scheduler(v), v->sched_unit, new_cpu); + sched_migrate(unit_scheduler(unit), unit, new_cpu); } /* * Initiating migration * - * In order to migrate, we need the vcpu in question to have stopped + * In order to migrate, we need the unit in question to have stopped * running and had sched_sleep() called (to take it off any * runqueues, for instance); and if it is currently running, it needs * to be scheduled out. Finally, we need to hold the scheduling locks @@ -777,37 +782,45 @@ static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu) * should be called like this: * * lock = unit_schedule_lock_irq(unit); - * vcpu_migrate_start(v); + * sched_unit_migrate_start(unit); * unit_schedule_unlock_irq(lock, unit) - * vcpu_migrate_finish(v); + * sched_unit_migrate_finish(unit); * - * vcpu_migrate_finish() will do the work now if it can, or simply - * return if it can't (because v is still running); in that case - * vcpu_migrate_finish() will be called by context_saved(). + * sched_unit_migrate_finish() will do the work now if it can, or simply + * return if it can't (because unit is still running); in that case + * sched_unit_migrate_finish() will be called by context_saved(). */ -static void vcpu_migrate_start(struct vcpu *v) +static void sched_unit_migrate_start(struct sched_unit *unit) { - set_bit(_VPF_migrating, &v->pause_flags); - vcpu_sleep_nosync_locked(v); + struct vcpu *v; + + for_each_sched_unit_vcpu ( unit, v ) + { + set_bit(_VPF_migrating, &v->pause_flags); + vcpu_sleep_nosync_locked(v); + } } -static void vcpu_migrate_finish(struct vcpu *v) +static void sched_unit_migrate_finish(struct sched_unit *unit) { unsigned long flags; unsigned int old_cpu, new_cpu; spinlock_t *old_lock, *new_lock; bool_t pick_called = 0; + struct vcpu *v; /* - * If the vcpu is currently running, this will be handled by + * If the unit is currently running, this will be handled by * context_saved(); and in any case, if the bit is cleared, then * someone else has already done the work so we don't need to. */ - if ( v->sched_unit->is_running || - !test_bit(_VPF_migrating, &v->pause_flags) ) - return; + for_each_sched_unit_vcpu ( unit, v ) + { + if ( unit->is_running || !test_bit(_VPF_migrating, &v->pause_flags) ) + return; + } - old_cpu = new_cpu = v->processor; + old_cpu = new_cpu = unit->res->processor; for ( ; ; ) { /* @@ -820,7 +833,7 @@ static void vcpu_migrate_finish(struct vcpu *v) sched_spin_lock_double(old_lock, new_lock, &flags); - old_cpu = v->processor; + old_cpu = unit->res->processor; if ( old_lock == get_sched_res(old_cpu)->schedule_lock ) { /* @@ -829,15 +842,15 @@ static void vcpu_migrate_finish(struct vcpu *v) */ if ( pick_called && (new_lock == get_sched_res(new_cpu)->schedule_lock) && - cpumask_test_cpu(new_cpu, v->sched_unit->cpu_hard_affinity) && - cpumask_test_cpu(new_cpu, v->domain->cpupool->cpu_valid) ) + cpumask_test_cpu(new_cpu, unit->cpu_hard_affinity) && + cpumask_test_cpu(new_cpu, unit->domain->cpupool->cpu_valid) ) break; /* Select a new CPU. */ - new_cpu = sched_pick_resource(vcpu_scheduler(v), - v->sched_unit)->processor; + new_cpu = sched_pick_resource(unit_scheduler(unit), + unit)->processor; if ( (new_lock == get_sched_res(new_cpu)->schedule_lock) && - cpumask_test_cpu(new_cpu, v->domain->cpupool->cpu_valid) ) + cpumask_test_cpu(new_cpu, unit->domain->cpupool->cpu_valid) ) break; pick_called = 1; } @@ -858,22 +871,30 @@ static void vcpu_migrate_finish(struct vcpu *v) * because they both happen in (different) spinlock regions, and those * regions are strictly serialised. */ - if ( v->sched_unit->is_running || - !test_and_clear_bit(_VPF_migrating, &v->pause_flags) ) + for_each_sched_unit_vcpu ( unit, v ) { - sched_spin_unlock_double(old_lock, new_lock, flags); - return; + if ( unit->is_running || + !test_and_clear_bit(_VPF_migrating, &v->pause_flags) ) + { + sched_spin_unlock_double(old_lock, new_lock, flags); + return; + } } - vcpu_move_locked(v, new_cpu); + sched_unit_move_locked(unit, new_cpu); sched_spin_unlock_double(old_lock, new_lock, flags); if ( old_cpu != new_cpu ) - sched_move_irqs(v->sched_unit); + { + for_each_sched_unit_vcpu ( unit, v ) + sync_vcpu_execstate(v); + sched_move_irqs(unit); + } /* Wake on new CPU. */ - vcpu_wake(v); + for_each_sched_unit_vcpu ( unit, v ) + vcpu_wake(v); } /* @@ -1041,10 +1062,9 @@ 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(unit->vcpu_list); + sched_unit_migrate_start(unit); unit_schedule_unlock_irqrestore(lock, flags, unit); - - vcpu_migrate_finish(unit->vcpu_list); + sched_unit_migrate_finish(unit); /* * The only caveat, in this case, is that if a vcpu active in @@ -1128,14 +1148,14 @@ static int vcpu_set_affinity( ASSERT(which == unit->cpu_soft_affinity); sched_set_affinity(v, NULL, affinity); } - vcpu_migrate_start(v); + sched_unit_migrate_start(unit); } unit_schedule_unlock_irq(lock, unit); domain_update_node_affinity(v->domain); - vcpu_migrate_finish(v); + sched_unit_migrate_finish(unit); return ret; } @@ -1396,12 +1416,12 @@ int vcpu_temporary_affinity(struct vcpu *v, unsigned int cpu, uint8_t reason) migrate = !ret && !cpumask_test_cpu(v->processor, unit->cpu_hard_affinity); if ( migrate ) - vcpu_migrate_start(v); + sched_unit_migrate_start(unit); unit_schedule_unlock_irq(lock, unit); if ( migrate ) - vcpu_migrate_finish(v); + sched_unit_migrate_finish(unit); return ret; } @@ -1794,7 +1814,7 @@ void context_saved(struct vcpu *prev) sched_context_saved(vcpu_scheduler(prev), prev->sched_unit); - vcpu_migrate_finish(prev); + sched_unit_migrate_finish(prev->sched_unit); } /* The scheduler timer: force a run through the scheduler */
Now that vcpu_migrate_start() and vcpu_migrate_finish() are used only to ensure a vcpu is running on a suitable processor they can be switched to operate on schedule units instead of vcpus. While doing that rename them accordingly and make the _start() variant static. As it is needed anyway call vcpu_sync_execstate() for each vcpu of the unit when changing processors. vcpu_move_locked() is switched to schedule unit, too. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/schedule.c | 106 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 63 insertions(+), 43 deletions(-)