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

[Juergen Gross <jgross@xxxxxxxx>]

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

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