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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2019 14:14, Juergen Gross wrote:
> ---
> - Carved out from my core scheduling series
> - Reworked to avoid deadlock when 2 vcpus are trying to modify each
>   others periodic timers, leading to address all comments by Jan
>   Beulich.

Oh, indeed - a mutual vcpu_pause() can't end  well.

> @@ -724,24 +725,6 @@ static void vcpu_migrate_finish(struct vcpu *v)
>      vcpu_wake(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.
> - */
> -void vcpu_force_reschedule(struct vcpu *v)
> -{
> -    spinlock_t *lock = vcpu_schedule_lock_irq(v);
> -
> -    if ( v->is_running )
> -        vcpu_migrate_start(v);
> -
> -    vcpu_schedule_unlock_irq(lock, v);
> -
> -    vcpu_migrate_finish(v);
> -}

So the comment specifically said this approach was chosen to
avoid the need for synchronization. You now introduce
synchronization. I'm not severely opposed, but I think there
wants to be justification why the added synchronization is not
a problem (and would perhaps never have been).

> @@ -1458,14 +1441,11 @@ long sched_adjust_global(struct 
> xen_sysctl_scheduler_op *op)
>      return rc;
>  }
>  
> -static void vcpu_periodic_timer_work(struct vcpu *v)
> +static void vcpu_periodic_timer_work_locked(struct vcpu *v)
>  {
>      s_time_t now;
>      s_time_t periodic_next_event;
>  
> -    if ( v->periodic_period == 0 )
> -        return;

I'm afraid you can't pull this out of here, or ...

> @@ -1476,10 +1456,36 @@ static void vcpu_periodic_timer_work(struct vcpu *v)
>          periodic_next_event = now + v->periodic_period;
>      }
>  
> -    migrate_timer(&v->periodic_timer, smp_processor_id());
> +    migrate_timer(&v->periodic_timer, v->processor);
>      set_timer(&v->periodic_timer, periodic_next_event);
>  }
>  
> +static void vcpu_periodic_timer_work(struct vcpu *v)
> +{
> +    if ( v->periodic_period == 0 )
> +        return;
> +
> +    spin_lock(&v->periodic_timer_lock);

... the conditional here needs to move into the locked region.
Otherwise, despite having found non-zero above, by the time the
lock was acquired the value may have changed to zero.

As to the migrate_timer() change: Other than I feared, using
v->processor of a non-running vCPU does not look to have any
chance of acting on an offline CPU, thanks to
cpu_disable_scheduler() dealing with all vCPU-s (and not just
running ones), and thanks to CPU offlining happening in
stop-machine context.

Jan

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