[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 13.09.19 16:42, Jan Beulich wrote:
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).

The comment doesn't say I'm avoiding synchronization, but
scheduling.

The (now needed) synchronization is very unlikely to cause (short)
blocking.


@@ -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.

Yes, indeed. I think I'll let the initial test in place in order
to avoid taking the lock in the (common) case where the periodic
timer is disabled. Just adding another test after taking the lock
is needed, though.


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.

Correct. Without that the schedule lock wouldn't work at all, as
it is using v->processor to find the correct lock.


Juergen

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.