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

[Juergen Gross <jgross@suse.com>]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel