| Message ID | 20190809145833.1020-27-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: > vcpu_force_reschedule() is only used for modifying the periodic timer > of a vcpu. I don't think this is true prior to this patch, or else ... > @@ -419,8 +419,6 @@ int pv_shim_shutdown(uint8_t reason) > > if ( v != current ) > vcpu_unpause_by_systemcontroller(v); > - else > - vcpu_force_reschedule(v); ... there wouldn't be this deletion of code. Without further explanation it's unclear to me whether the re-schedule here isn't also needed for other than processing the reset of the periodic timer period. > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -877,21 +877,25 @@ static void vcpu_migrate_finish(struct vcpu *v) > } > > /* > - * Force a VCPU through a deschedule/reschedule path. > - * For example, using this when setting the periodic timer period means that > - * most periodic-timer state need only be touched from within the scheduler > - * which can thus be done without need for synchronisation. > + * Set the periodic timer of a vcpu. > */ > -void vcpu_force_reschedule(struct vcpu *v) > +void vcpu_set_periodic_timer(struct vcpu *v, s_time_t value) > { > - spinlock_t *lock = unit_schedule_lock_irq(v->sched_unit); > + s_time_t now; > > - if ( v->sched_unit->is_running ) > - vcpu_migrate_start(v); > + if ( v != current ) > + vcpu_pause(v); > + else > + stop_timer(&v->periodic_timer); > > - unit_schedule_unlock_irq(lock, v->sched_unit); > + now = NOW(); > + v->periodic_period = value; > + v->periodic_last_event = now; I'm afraid this alters an implicit property of the previous implementation: periodic_last_event would get re-set only when the newly calculated next event wouldn't be in the future. The goal of this (aiui) is to not disturb a periodic timer which gets (redundantly) set again to the same period. > - vcpu_migrate_finish(v); > + if ( v != current ) > + vcpu_unpause(v); > + else if ( value != 0 ) > + set_timer(&v->periodic_timer, now + value); > } While perhaps not overly important, another difference to vcpu_periodic_timer_work() is the lack of migrate_timer() here. There would indeed be no migration needed if a periodic timer was active already (and if v == current), because it would have been migrated during the most recent scheduling cycle. But in case this is an off->on transition, no such migration may have occurred before. Finally a remark towards ordering in the series: This looks to be textually but not functionally dependent upon earlier patches in the series. Such patches, when placed early in a series, have a fair chance of going in ahead of the bulk of such series. Jan
On 10.09.19 16:06, Jan Beulich wrote: > On 09.08.2019 16:58, Juergen Gross wrote: >> vcpu_force_reschedule() is only used for modifying the periodic timer >> of a vcpu. > > I don't think this is true prior to this patch, or else ... > >> @@ -419,8 +419,6 @@ int pv_shim_shutdown(uint8_t reason) >> >> if ( v != current ) >> vcpu_unpause_by_systemcontroller(v); >> - else >> - vcpu_force_reschedule(v); > > ... there wouldn't be this deletion of code. Without further > explanation it's unclear to me whether the re-schedule here > isn't also needed for other than processing the reset of the > periodic timer period. This deletion is related to replacing the direct write of v->periodic_period by vcpu_set_periodic_timer(). I can't see any other reason for the re-schedule. > >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -877,21 +877,25 @@ static void vcpu_migrate_finish(struct vcpu *v) >> } >> >> /* >> - * Force a VCPU through a deschedule/reschedule path. >> - * For example, using this when setting the periodic timer period means that >> - * most periodic-timer state need only be touched from within the scheduler >> - * which can thus be done without need for synchronisation. >> + * Set the periodic timer of a vcpu. >> */ >> -void vcpu_force_reschedule(struct vcpu *v) >> +void vcpu_set_periodic_timer(struct vcpu *v, s_time_t value) >> { >> - spinlock_t *lock = unit_schedule_lock_irq(v->sched_unit); >> + s_time_t now; >> >> - if ( v->sched_unit->is_running ) >> - vcpu_migrate_start(v); >> + if ( v != current ) >> + vcpu_pause(v); >> + else >> + stop_timer(&v->periodic_timer); >> >> - unit_schedule_unlock_irq(lock, v->sched_unit); >> + now = NOW(); >> + v->periodic_period = value; >> + v->periodic_last_event = now; > > I'm afraid this alters an implicit property of the previous > implementation: periodic_last_event would get re-set only when > the newly calculated next event wouldn't be in the future. The > goal of this (aiui) is to not disturb a periodic timer which > gets (redundantly) set again to the same period. Ah, right. Will change that. > >> - vcpu_migrate_finish(v); >> + if ( v != current ) >> + vcpu_unpause(v); >> + else if ( value != 0 ) >> + set_timer(&v->periodic_timer, now + value); >> } > > While perhaps not overly important, another difference to > vcpu_periodic_timer_work() is the lack of migrate_timer() here. > There would indeed be no migration needed if a periodic timer > was active already (and if v == current), because it would > have been migrated during the most recent scheduling cycle. But > in case this is an off->on transition, no such migration may > have occurred before. True. Will change that. > Finally a remark towards ordering in the series: This looks to > be textually but not functionally dependent upon earlier > patches in the series. Such patches, when placed early in a > series, have a fair chance of going in ahead of the bulk of > such series. I'll move it to the front and post it on its own. Juergen
On 13.09.2019 11:33, Juergen Gross wrote: > On 10.09.19 16:06, Jan Beulich wrote: >> On 09.08.2019 16:58, Juergen Gross wrote: >>> vcpu_force_reschedule() is only used for modifying the periodic timer >>> of a vcpu. >> >> I don't think this is true prior to this patch, or else ... >> >>> @@ -419,8 +419,6 @@ int pv_shim_shutdown(uint8_t reason) >>> >>> if ( v != current ) >>> vcpu_unpause_by_systemcontroller(v); >>> - else >>> - vcpu_force_reschedule(v); >> >> ... there wouldn't be this deletion of code. Without further >> explanation it's unclear to me whether the re-schedule here >> isn't also needed for other than processing the reset of the >> periodic timer period. > > This deletion is related to replacing the direct write of > v->periodic_period by vcpu_set_periodic_timer(). > > I can't see any other reason for the re-schedule. Roger, you added this in e89cb79aae ("xen/pvshim: add migration support") - can you confirm the above? Thanks, Jan
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c index 324ca27f93..5edbcd9ac5 100644 --- a/xen/arch/x86/pv/shim.c +++ b/xen/arch/x86/pv/shim.c @@ -410,7 +410,7 @@ int pv_shim_shutdown(uint8_t reason) unmap_vcpu_info(v); /* Reset the periodic timer to the default value. */ - v->periodic_period = MILLISECS(10); + vcpu_set_periodic_timer(v, MILLISECS(10)); /* Stop the singleshot timer. */ stop_timer(&v->singleshot_timer); @@ -419,8 +419,6 @@ int pv_shim_shutdown(uint8_t reason) if ( v != current ) vcpu_unpause_by_systemcontroller(v); - else - vcpu_force_reschedule(v); } return 0; diff --git a/xen/common/domain.c b/xen/common/domain.c index 91b01c220e..863b7cae35 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1479,15 +1479,13 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) if ( set.period_ns > STIME_DELTA_MAX ) return -EINVAL; - v->periodic_period = set.period_ns; - vcpu_force_reschedule(v); + vcpu_set_periodic_timer(v, set.period_ns); break; } case VCPUOP_stop_periodic_timer: - v->periodic_period = 0; - vcpu_force_reschedule(v); + vcpu_set_periodic_timer(v, 0); break; case VCPUOP_set_singleshot_timer: diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 3f8fffc329..4c488ddde0 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -877,21 +877,25 @@ static void vcpu_migrate_finish(struct vcpu *v) } /* - * Force a VCPU through a deschedule/reschedule path. - * For example, using this when setting the periodic timer period means that - * most periodic-timer state need only be touched from within the scheduler - * which can thus be done without need for synchronisation. + * Set the periodic timer of a vcpu. */ -void vcpu_force_reschedule(struct vcpu *v) +void vcpu_set_periodic_timer(struct vcpu *v, s_time_t value) { - spinlock_t *lock = unit_schedule_lock_irq(v->sched_unit); + s_time_t now; - if ( v->sched_unit->is_running ) - vcpu_migrate_start(v); + if ( v != current ) + vcpu_pause(v); + else + stop_timer(&v->periodic_timer); - unit_schedule_unlock_irq(lock, v->sched_unit); + now = NOW(); + v->periodic_period = value; + v->periodic_last_event = now; - vcpu_migrate_finish(v); + if ( v != current ) + vcpu_unpause(v); + else if ( value != 0 ) + set_timer(&v->periodic_timer, now + value); } static bool sched_check_affinity_broken(struct sched_unit *unit) diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 0cece3b921..7f84b823cb 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -896,7 +896,7 @@ struct scheduler *scheduler_get_default(void); struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr); void scheduler_free(struct scheduler *sched); int schedule_cpu_switch(unsigned int cpu, struct cpupool *c); -void vcpu_force_reschedule(struct vcpu *v); +void vcpu_set_periodic_timer(struct vcpu *v, s_time_t value); int cpu_disable_scheduler(unsigned int cpu); /* We need it in dom0_setup_vcpu */ void sched_set_affinity(struct vcpu *v, const cpumask_t *hard,
vcpu_force_reschedule() is only used for modifying the periodic timer of a vcpu. Forcing a vcpu to give up the physical cpu for that purpose is kind of brutal. So instead of doing the reschedule dance just operate on the timer directly. In case we are modifying the timer of the currently running vcpu we can just do that. In case it is for a foreign vcpu we should pause it for that purpose like we do for all other vcpu state modifications. Rename the function to vcpu_set_periodic_timer() as this now reflects the functionality. Signed-off-by: Juergen Gross <jgross@suse.com> --- V1: latch NOW() only after stopping the timer (Jan Beulich) --- xen/arch/x86/pv/shim.c | 4 +--- xen/common/domain.c | 6 ++---- xen/common/schedule.c | 24 ++++++++++++++---------- xen/include/xen/sched.h | 2 +- 4 files changed, 18 insertions(+), 18 deletions(-)