Re: [Xen-devel] [PATCH v2 26/48] xen/sched: rework and rename vcpu_force_reschedule()

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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