[PATCH v2 0/6] xen: sched: fix locking of {insert, remove}_vcpu()

[PATCH v2 0/6] xen: sched: fix locking of {insert, remove}_vcpu()

[Dario Faggioli]

Hey,

Here it comes v2 of this series. It took a bit more than how I wanted, mostly
because of the cpufreq issue I found while testing (as I though I was causing
it! :-/).

Version 1 is here:

 http://www.gossamer-threads.com/lists/xen/devel/401619?page=last

I think I've addressed all the review comments, but the series also changed a
little bit. Because of that, most of the patches don't carry the
Reviewed-/Acked-by tags they were given during v1's review.

More specifically:
 
 * patch 2 needed adjustments as, in case only patches 1 and 2 are applied
   (i.e., if what was patch 3 in v1, which is now patch 4, is _not_applied)
   the runqueue lock in Credit1's implementation of insert_vcpu() needs to
   be an _irqsave() one. In fact, it is acquired with interrupt disabled
   during boot (when initializing idle vCPUs). Since Credit2 and RTDS bail
   out before even trying taking the lock, for idle vCPUs, they're fine with
   just _irq(). Also, it can be turned into an _irq() one in patch 4, when we
   kill the need of calling that hook altogether, for idle vCPUs.  This is
   done like this because I think it is worth considering backporting patches
   1 and 2, so I think it's important that they work well, independently from
   the rest of the series (which is mostly improvements, so backporting is
   out of question)

 * patch 3 is new. It's "just" comments and ASSERT()-s about something
   discussed already with both Jan and George.

The rest, really should be no more than v1 with review comments addressed
(except, of course, patches 6 and 7 from v1, which are in already :-D).

Git branch available here:

 git://xenbits.xen.org/people/dariof/xen.git  rel/sched/fix-vcpu-ins-rem-v2

Thanks and Regards,
Dario
---
Dario Faggioli (6):
      xen: sched: fix locking of remove_vcpu() in credit1
      xen: sched: fix locking for insert_vcpu() in credit1 and RTDS
      xen: sched: clarify use cases of schedule_cpu_switch()
      xen: sched: better handle (not) inserting idle vCPUs in runqueues
      xen: sched: get rid of the per domain vCPU list in RTDS
      xen: sched: get rid of the per domain vCPU list in Credit2

 xen/common/sched_credit.c  |   17 ++++++++++--
 xen/common/sched_credit2.c |   55 ++++++++++++++--------------------------
 xen/common/sched_rt.c      |   61 ++++++++++++++++++++++----------------------
 xen/common/schedule.c      |   55 ++++++++++++++++++++++++++++------------
 4 files changed, 102 insertions(+), 86 deletions(-)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[PATCH v2 1/6] xen: sched: fix locking of remove_vcpu() in credit1

[Dario Faggioli]

In fact, csched_vcpu_remove() (i.e., the credit1
implementation of remove_vcpu()) manipulates runqueues,
so holding the runqueue lock is necessary.

Also, while there, *_lock_irq() (for the private lock) is
enough, there is no need to *_lock_irqsave().

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
Changes from the other series:
 * split the patch (wrt the original patch, in the original
   series), and take care, in this one, only of remove_vcpu();
 * removed pointless parentheses.
---
 xen/common/sched_credit.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index b8f28fe..6f71e0d 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -926,7 +926,7 @@ csched_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
     struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_vcpu * const svc = CSCHED_VCPU(vc);
     struct csched_dom * const sdom = svc->sdom;
-    unsigned long flags;
+    spinlock_t *lock;
 
     SCHED_STAT_CRANK(vcpu_remove);
 
@@ -936,15 +936,19 @@ csched_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
         vcpu_unpause(svc->vcpu);
     }
 
+    lock = vcpu_schedule_lock_irq(vc);
+
     if ( __vcpu_on_runq(svc) )
         __runq_remove(svc);
 
-    spin_lock_irqsave(&(prv->lock), flags);
+    vcpu_schedule_unlock_irq(lock, vc);
+
+    spin_lock_irq(&prv->lock);
 
     if ( !list_empty(&svc->active_vcpu_elem) )
         __csched_vcpu_acct_stop_locked(prv, svc);
 
-    spin_unlock_irqrestore(&(prv->lock), flags);
+    spin_unlock_irq(&prv->lock);
 
     BUG_ON( sdom == NULL );
     BUG_ON( !list_empty(&svc->runq_elem) );

[PATCH v2 2/6] xen: sched: fix locking for insert_vcpu() in credit1 and RTDS

[Dario Faggioli]

The insert_vcpu() hook is handled with inconsistent locking.
In fact, schedule_cpu_switch() calls the hook with runqueue
lock held, while sched_move_domain() relies on the hook
implementations to take the lock themselves (and, since that
is not done in Credit1 and RTDS, such operation is not safe
in those cases).

Turn this situation into _always_ taking the lock in the
hook implementations, in specific schedulers' code.

Note that it is safe to get rid of the locking in
schedule_cpu_switch() as the pCPU being switched is, at
the time of the switch, not a valid member of any cpupool,
so no scheduling event should be expected on it, locking
or not.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Meng Xu <mengxu@cis.upenn.edu>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
Changes from v1 (of this series):
 * in Credit1, the lock wants to be an _irqsave() one. If
   not, the ASSERT() in _spin_lock_irq() will trigger when
   the hook is called, during boot, from sched_init_vcpu();
 * reprhased the changelog (to be less verbose);
 * add a few words, in the changelog, about why it is safe
   to get rid of the locking in schedule_cpu_switch(). Proper
   commentary and ASSERT()-s about that will come in another
   patch.

Changes from the other series:
 * split the patch (wrt the original patch, in the original
   series), and take care, in this one, only of insert_vcpu();
---
Note that I'm not picking up Andrew's Reviewed-by, as the patch
changed a little bit.
---
 xen/common/sched_credit.c |    6 ++++++
 xen/common/sched_rt.c     |    3 +++
 xen/common/schedule.c     |    6 ------
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 6f71e0d..e16bd3a 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -903,10 +903,16 @@ static void
 csched_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 {
     struct csched_vcpu *svc = vc->sched_priv;
+    spinlock_t *lock;
+    unsigned long flags;
+
+    lock = vcpu_schedule_lock_irqsave(vc, &flags);
 
     if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
         __runq_insert(vc->processor, svc);
 
+    vcpu_schedule_unlock_irqrestore(lock, flags, vc);
+
     SCHED_STAT_CRANK(vcpu_insert);
 }
 
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 6a341b1..1086399 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -622,16 +622,19 @@ rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 {
     struct rt_vcpu *svc = rt_vcpu(vc);
     s_time_t now = NOW();
+    spinlock_t *lock;
 
     /* not addlocate idle vcpu to dom vcpu list */
     if ( is_idle_vcpu(vc) )
         return;
 
+    lock = vcpu_schedule_lock_irq(vc);
     if ( now >= svc->cur_deadline )
         rt_update_deadline(now, svc);
 
     if ( !__vcpu_on_q(svc) && vcpu_runnable(vc) && !vc->is_running )
         __runq_insert(ops, svc);
+    vcpu_schedule_unlock_irq(lock, vc);
 
     /* add rt_vcpu svc to scheduler-specific vcpu list of the dom */
     list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c5f640f..9aa209d 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1488,9 +1488,7 @@ void __init scheduler_init(void)
 
 int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
 {
-    unsigned long flags;
     struct vcpu *idle;
-    spinlock_t *lock;
     void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
     struct scheduler *old_ops = per_cpu(scheduler, cpu);
     struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
@@ -1509,8 +1507,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
         return -ENOMEM;
     }
 
-    lock = pcpu_schedule_lock_irqsave(cpu, &flags);
-
     SCHED_OP(old_ops, tick_suspend, cpu);
     vpriv_old = idle->sched_priv;
     idle->sched_priv = vpriv;
@@ -1520,8 +1516,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     SCHED_OP(new_ops, tick_resume, cpu);
     SCHED_OP(new_ops, insert_vcpu, idle);
 
-    pcpu_schedule_unlock_irqrestore(lock, flags, cpu);
-
     SCHED_OP(old_ops, free_vdata, vpriv_old);
     SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);

Re: [PATCH v2 2/6] xen: sched: fix locking for insert_vcpu() in credit1 and RTDS

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1830 bytes --]

On Wed, 2015-10-14 at 17:54 +0200, Dario Faggioli wrote:

So, about this:

> Note that it is safe to get rid of the locking in
> schedule_cpu_switch() as the pCPU being switched is, at
> the time of the switch, not a valid member of any cpupool,
> so no scheduling event should be expected on it, locking
> or not.
> 
I changed my mind about removing this locking.

In fact...
 
> @@ -1509,8 +1507,6 @@ int schedule_cpu_switch(unsigned int cpu,
> struct cpupool *c)
>          return -ENOMEM;
>      }
>  
> -    lock = pcpu_schedule_lock_irqsave(cpu, &flags);
> -
>      SCHED_OP(old_ops, tick_suspend, cpu);
>      vpriv_old = idle->sched_priv;
>      idle->sched_priv = vpriv;
> @@ -1520,8 +1516,6 @@ int schedule_cpu_switch(unsigned int cpu,
> struct cpupool *c)
>      SCHED_OP(new_ops, tick_resume, cpu);
>      SCHED_OP(new_ops, insert_vcpu, idle);
>  
> -    pcpu_schedule_unlock_irqrestore(lock, flags, cpu);
> -
>      SCHED_OP(old_ops, free_vdata, vpriv_old);
>      SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);
>  
... while reworking and testing my other series (the one about Credit2
runqueues) I'm seeing some issues (even without the patches from that
series applied) that may be related to this lock not being taken any
longer.

Also (less strong an argument, but still), when splitting allocation and initialization of per-pCPU data (which is what that series does) I'd probably have to reinstate some similar locking again. :-/

I'll investigate more and respin this series.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Dario Faggioli]

schedule_cpu_switch() is meant to be only used for moving
pCPUs from a cpupool to no cpupool, and from there back
to a cpupool, *not* to move them directly from one cpupool
to another.

This is something that is reflected in the way it is
implemented, and should be kept in mind when looking at
it. However, that is not that clear, by just the look of
it.

Make it more evident by adding commentary and ASSERT()s.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
Changes from v1:
 * new patch, was not there in v1.
---
 xen/common/schedule.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 9aa209d..5ebfa33 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1486,12 +1486,40 @@ void __init scheduler_init(void)
         BUG();
 }
 
+/*
+ * Move a pCPU outside of the influence of the scheduler of its current
+ * cpupool, or subject it to the scheduler of a new cpupool.
+ *
+ * For the pCPUs that are removed from their cpupool, their scheduler becomes
+ * &ops (the default scheduler, selected at boot, which also services the
+ * default cpupool). However, as these pCPUs are not really part of any pool,
+ * there won't be any scheduling event on them, not even from the default
+ * scheduler. Basically, they will just sit idle until they are explicitly
+ * added back to a cpupool.
+ */
 int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
 {
     struct vcpu *idle;
     void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
     struct scheduler *old_ops = per_cpu(scheduler, cpu);
     struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
+    struct cpupool *pool = per_cpu(cpupool, cpu);
+
+    /*
+     * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
+     * or from free to a valid cpupool. In the former case (which happens when
+     * c is NULL), we want the CPU to have been marked as free already, as
+     * well as to not be valid for the source pool any longer, when we get to
+     * here. In the latter case (which happens when c is a valid cpupool), we
+     * want the CPU to still be marked as free, as well as to not yet be valid
+     * for the destination pool.
+     * This all is because we do not want any scheduling activity on the CPU
+     * while, in here, we switch things over.
+     */
+    ASSERT(c != NULL || pool != NULL);
+    ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
+    ASSERT((c == NULL && !cpumask_test_cpu(cpu, pool->cpu_valid)) ||
+           (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
 
     if ( old_ops == new_ops )
         return 0;

Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Juergen Gross]

On 10/14/2015 05:54 PM, Dario Faggioli wrote:
> schedule_cpu_switch() is meant to be only used for moving
> pCPUs from a cpupool to no cpupool, and from there back
> to a cpupool, *not* to move them directly from one cpupool
> to another.
>
> This is something that is reflected in the way it is
> implemented, and should be kept in mind when looking at
> it. However, that is not that clear, by just the look of
> it.
>
> Make it more evident by adding commentary and ASSERT()s.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> ---
> Changes from v1:
>   * new patch, was not there in v1.
> ---
>   xen/common/schedule.c |   28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 9aa209d..5ebfa33 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1486,12 +1486,40 @@ void __init scheduler_init(void)
>           BUG();
>   }
>
> +/*
> + * Move a pCPU outside of the influence of the scheduler of its current
> + * cpupool, or subject it to the scheduler of a new cpupool.
> + *
> + * For the pCPUs that are removed from their cpupool, their scheduler becomes
> + * &ops (the default scheduler, selected at boot, which also services the
> + * default cpupool). However, as these pCPUs are not really part of any pool,
> + * there won't be any scheduling event on them, not even from the default
> + * scheduler. Basically, they will just sit idle until they are explicitly
> + * added back to a cpupool.
> + */
>   int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>   {
>       struct vcpu *idle;
>       void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
>       struct scheduler *old_ops = per_cpu(scheduler, cpu);
>       struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
> +    struct cpupool *pool = per_cpu(cpupool, cpu);
> +
> +    /*
> +     * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
> +     * or from free to a valid cpupool. In the former case (which happens when
> +     * c is NULL), we want the CPU to have been marked as free already, as
> +     * well as to not be valid for the source pool any longer, when we get to
> +     * here. In the latter case (which happens when c is a valid cpupool), we
> +     * want the CPU to still be marked as free, as well as to not yet be valid
> +     * for the destination pool.
> +     * This all is because we do not want any scheduling activity on the CPU
> +     * while, in here, we switch things over.

While this is correct I think you should mention that in the "adding to
a pool" case per_cpu(cpupool, cpu) already contains the new pool
pointer. So c == pool then and both are not NULL.

In fact pool is never NULL, c might be.

Maybe it would be a good idea to move setting of per_cpu(cpupool, cpu)
into schedule_cpu_switch(). Originally I didn't do that to avoid
spreading too much cpupool related actions outside of cpupool.c. But
with those ASSERT()s added hiding that action will cause more confusion
than having the modification of per_cpu(cpupool, cpu) here.

When doing the code movement the current behaviour regarding sequence
of changes must be kept, of course. So when adding the cpu to a pool
the cpupool information must be set _before_ taking the scheduler lock,
while when removing this must happen after release of the lock.

> +     */
> +    ASSERT(c != NULL || pool != NULL);
> +    ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
> +    ASSERT((c == NULL && !cpumask_test_cpu(cpu, pool->cpu_valid)) ||
> +           (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
>
>       if ( old_ops == new_ops )
>           return 0;


Juergen

Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4557 bytes --]

On Thu, 2015-10-15 at 10:25 +0200, Juergen Gross wrote:
> On 10/14/2015 05:54 PM, Dario Faggioli wrote:
> > schedule_cpu_switch() is meant to be only used for moving
> > pCPUs from a cpupool to no cpupool, and from there back
> > to a cpupool, *not* to move them directly from one cpupool
> > to another.
> > 
> > This is something that is reflected in the way it is
> > implemented, and should be kept in mind when looking at
> > it. However, that is not that clear, by just the look of
> > it.
> > 
> > Make it more evident by adding commentary and ASSERT()s.
> > 
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> > ---
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > ---
> > Changes from v1:
> >   * new patch, was not there in v1.
> > ---
> >   xen/common/schedule.c |   28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> > 
> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index 9aa209d..5ebfa33 100644
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -1486,12 +1486,40 @@ void __init scheduler_init(void)
> >           BUG();
> >   }
> > 
> > +/*
> > + * Move a pCPU outside of the influence of the scheduler of its
> > current
> > + * cpupool, or subject it to the scheduler of a new cpupool.
> > + *
> > + * For the pCPUs that are removed from their cpupool, their
> > scheduler becomes
> > + * &ops (the default scheduler, selected at boot, which also
> > services the
> > + * default cpupool). However, as these pCPUs are not really part
> > of any pool,
> > + * there won't be any scheduling event on them, not even from the
> > default
> > + * scheduler. Basically, they will just sit idle until they are
> > explicitly
> > + * added back to a cpupool.
> > + */
> >   int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
> >   {
> >       struct vcpu *idle;
> >       void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
> >       struct scheduler *old_ops = per_cpu(scheduler, cpu);
> >       struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
> > +    struct cpupool *pool = per_cpu(cpupool, cpu);
> > +
> > +    /*
> > +     * pCPUs only move from a valid cpupool to free (i.e., out of
> > any pool),
> > +     * or from free to a valid cpupool. In the former case (which
> > happens when
> > +     * c is NULL), we want the CPU to have been marked as free
> > already, as
> > +     * well as to not be valid for the source pool any longer,
> > when we get to
> > +     * here. In the latter case (which happens when c is a valid
> > cpupool), we
> > +     * want the CPU to still be marked as free, as well as to not
> > yet be valid
> > +     * for the destination pool.
> > +     * This all is because we do not want any scheduling activity
> > on the CPU
> > +     * while, in here, we switch things over.
> 
> While this is correct I think you should mention that in the "adding
> to
> a pool" case per_cpu(cpupool, cpu) already contains the new pool
> pointer. So c == pool then and both are not NULL.
> 
Eheh, I did have an ASSERT() for this too, but I eventually dropped it,
as it looked a bit too verbose. :-)

I can add it, but I like this suggestion of yours here below, so I
guess I'll try it first...

> Maybe it would be a good idea to move setting of per_cpu(cpupool,
> cpu)
> into schedule_cpu_switch(). Originally I didn't do that to avoid
> spreading too much cpupool related actions outside of cpupool.c. But
> with those ASSERT()s added hiding that action will cause more
> confusion
> than having the modification of per_cpu(cpupool, cpu) here.
> 
Yep, as said, in principle, I like the idea. I'll try and see how the
code end up looking like.

> When doing the code movement the current behaviour regarding sequence
> of changes must be kept, of course. So when adding the cpu to a pool
> the cpupool information must be set _before_ taking the scheduler
> lock,
> while when removing this must happen after release of the lock.
> 
I'm not entirely sure what you mean when you refer to "taking the
scheduler lock", but yes, I'll keep the ordering.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Dario Faggioli]

[-- Attachment #1.1.1: Type: text/plain, Size: 5348 bytes --]

On Thu, 2015-10-15 at 10:25 +0200, Juergen Gross wrote:
> Maybe it would be a good idea to move setting of per_cpu(cpupool,
> cpu)
> into schedule_cpu_switch(). Originally I didn't do that to avoid
> spreading too much cpupool related actions outside of cpupool.c. But
> with those ASSERT()s added hiding that action will cause more
> confusion
> than having the modification of per_cpu(cpupool, cpu) here.
> 
Coming back to this.

When reworking the series, I tried to move 'per_cpu(cpupool, cpu)=c' in
schedule_cpu_switch() and, about this part:

> When doing the code movement the current behaviour regarding sequence
> of changes must be kept, of course. So when adding the cpu to a pool
> the cpupool information must be set _before_ taking the scheduler
> lock,
> while when removing this must happen after release of the lock.
> 
I don't think I see why I can't do as in the attached patch (i.e., just
update the cpupool per-cpu variable at the bottom of
schedule_cpu_switch() ).

I don't see anything in the various schedulers' code that relies on
that variable, except one thing in sched_credit.c, which looks safe to
me. And indeed I think that even any future potential code being added
there, should either not depend on it, or do it "safely".

Also, all the operations done in schedule_cpu_switch() itself, depend
either on per_cpu(scheduler) or on per_cpu(schedule_data) being updated
properly, rather than on per_cpu(cpupool) (including the locking that
you are mentioning above).

What am I missing?

Regards,
Dario
---
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index e79850b..8e7b723 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -261,19 +261,13 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
 static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 {
     int ret;
-    struct cpupool *old;
     struct domain *d;
 
     if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
         return -EBUSY;
-    old = per_cpu(cpupool, cpu);
-    per_cpu(cpupool, cpu) = c;
     ret = schedule_cpu_switch(cpu, c);
     if ( ret )
-    {
-        per_cpu(cpupool, cpu) = old;
         return ret;
-    }
 
     cpumask_clear_cpu(cpu, &cpupool_free_cpus);
     if (cpupool_moving_cpu == cpu)
@@ -326,7 +320,6 @@ static long cpupool_unassign_cpu_helper(void *info)
             cpumask_clear_cpu(cpu, &cpupool_free_cpus);
             goto out;
         }
-        per_cpu(cpupool, cpu) = NULL;
         cpupool_moving_cpu = -1;
         cpupool_put(cpupool_cpu_moving);
         cpupool_cpu_moving = NULL;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index d7e2d98..9072540 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1486,6 +1486,17 @@ void __init scheduler_init(void)
         BUG();
 }
 
+/*
+ * Move a pCPU outside of the influence of the scheduler of its current
+ * cpupool, or subject it to the scheduler of a new cpupool.
+ *
+ * For the pCPUs that are removed from their cpupool, their scheduler becomes
+ * &ops (the default scheduler, selected at boot, which also services the
+ * default cpupool). However, as these pCPUs are not really part of any pool,
+ * there won't be any scheduling event on them, not even from the default
+ * scheduler. Basically, they will just sit idle until they are explicitly
+ * added back to a cpupool.
+ */
 int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
 {
     unsigned long flags;
@@ -1494,9 +1505,24 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
     struct scheduler *old_ops = per_cpu(scheduler, cpu);
     struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
+    struct cpupool *old_pool = per_cpu(cpupool, cpu);
+
+    /*
+     * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
+     * or from free to a valid cpupool. In the former case (which happens when
+     * c is NULL), we want the CPU to have been marked as free already, as
+     * well as to not be valid for the source pool any longer, when we get to
+     * here. In the latter case (which happens when c is a valid cpupool), we
+     * want the CPU to still be marked as free, as well as to not yet be valid
+     * for the destination pool.
+     */
+    ASSERT(c != old_pool && (c != NULL || old_pool != NULL));
+    ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
+    ASSERT((c == NULL && !cpumask_test_cpu(cpu, old_pool->cpu_valid)) ||
+           (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
 
     if ( old_ops == new_ops )
-        return 0;
+        goto out;
 
     idle = idle_vcpu[cpu];
     ppriv = SCHED_OP(new_ops, alloc_pdata, cpu);
@@ -1524,6 +1550,9 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     SCHED_OP(old_ops, free_vdata, vpriv_old);
     SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);
 
+ out:
+    per_cpu(cpupool, cpu) = c;
+
     return 0;
 }
 
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.1.2: xen-sched-asserts-in-schedule_cpu_switch.patch --]
[-- Type: text/x-patch, Size: 3421 bytes --]

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index e79850b..8e7b723 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -261,19 +261,13 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
 static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 {
     int ret;
-    struct cpupool *old;
     struct domain *d;
 
     if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
         return -EBUSY;
-    old = per_cpu(cpupool, cpu);
-    per_cpu(cpupool, cpu) = c;
     ret = schedule_cpu_switch(cpu, c);
     if ( ret )
-    {
-        per_cpu(cpupool, cpu) = old;
         return ret;
-    }
 
     cpumask_clear_cpu(cpu, &cpupool_free_cpus);
     if (cpupool_moving_cpu == cpu)
@@ -326,7 +320,6 @@ static long cpupool_unassign_cpu_helper(void *info)
             cpumask_clear_cpu(cpu, &cpupool_free_cpus);
             goto out;
         }
-        per_cpu(cpupool, cpu) = NULL;
         cpupool_moving_cpu = -1;
         cpupool_put(cpupool_cpu_moving);
         cpupool_cpu_moving = NULL;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index d7e2d98..9072540 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1486,6 +1486,17 @@ void __init scheduler_init(void)
         BUG();
 }
 
+/*
+ * Move a pCPU outside of the influence of the scheduler of its current
+ * cpupool, or subject it to the scheduler of a new cpupool.
+ *
+ * For the pCPUs that are removed from their cpupool, their scheduler becomes
+ * &ops (the default scheduler, selected at boot, which also services the
+ * default cpupool). However, as these pCPUs are not really part of any pool,
+ * there won't be any scheduling event on them, not even from the default
+ * scheduler. Basically, they will just sit idle until they are explicitly
+ * added back to a cpupool.
+ */
 int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
 {
     unsigned long flags;
@@ -1494,9 +1505,24 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
     struct scheduler *old_ops = per_cpu(scheduler, cpu);
     struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
+    struct cpupool *old_pool = per_cpu(cpupool, cpu);
+
+    /*
+     * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
+     * or from free to a valid cpupool. In the former case (which happens when
+     * c is NULL), we want the CPU to have been marked as free already, as
+     * well as to not be valid for the source pool any longer, when we get to
+     * here. In the latter case (which happens when c is a valid cpupool), we
+     * want the CPU to still be marked as free, as well as to not yet be valid
+     * for the destination pool.
+     */
+    ASSERT(c != old_pool && (c != NULL || old_pool != NULL));
+    ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
+    ASSERT((c == NULL && !cpumask_test_cpu(cpu, old_pool->cpu_valid)) ||
+           (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
 
     if ( old_ops == new_ops )
-        return 0;
+        goto out;
 
     idle = idle_vcpu[cpu];
     ppriv = SCHED_OP(new_ops, alloc_pdata, cpu);
@@ -1524,6 +1550,9 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     SCHED_OP(old_ops, free_vdata, vpriv_old);
     SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);
 
+ out:
+    per_cpu(cpupool, cpu) = c;
+
     return 0;
 }
 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Juergen Gross]

On 10/21/2015 07:10 PM, Dario Faggioli wrote:
> On Thu, 2015-10-15 at 10:25 +0200, Juergen Gross wrote:
>> Maybe it would be a good idea to move setting of per_cpu(cpupool,
>> cpu)
>> into schedule_cpu_switch(). Originally I didn't do that to avoid
>> spreading too much cpupool related actions outside of cpupool.c. But
>> with those ASSERT()s added hiding that action will cause more
>> confusion
>> than having the modification of per_cpu(cpupool, cpu) here.
>>
> Coming back to this.
>
> When reworking the series, I tried to move 'per_cpu(cpupool, cpu)=c' in
> schedule_cpu_switch() and, about this part:
>
>> When doing the code movement the current behaviour regarding sequence
>> of changes must be kept, of course. So when adding the cpu to a pool
>> the cpupool information must be set _before_ taking the scheduler
>> lock,
>> while when removing this must happen after release of the lock.
>>
> I don't think I see why I can't do as in the attached patch (i.e., just
> update the cpupool per-cpu variable at the bottom of
> schedule_cpu_switch() ).
>
> I don't see anything in the various schedulers' code that relies on
> that variable, except one thing in sched_credit.c, which looks safe to
> me. And indeed I think that even any future potential code being added
> there, should either not depend on it, or do it "safely".
>
> Also, all the operations done in schedule_cpu_switch() itself, depend
> either on per_cpu(scheduler) or on per_cpu(schedule_data) being updated
> properly, rather than on per_cpu(cpupool) (including the locking that
> you are mentioning above).
>
> What am I missing?

Hmm, good question. I'm rather sure I had a problem related to exactly
this topic in the early days of cpupools. Maybe the critical code has
been modified since then. Or my memory is wrong. Or we both don't see
it now. ;-)

In case there is a problem it should show up doing a test which
concurrently does all of the following:

- move a domain between two cpupools
- move a cpu between the two cpupools
- create and destroy a domain in one of the two cpupools

If the system is surviving this test for a couple of hours you are fine
and then for the attached patch:

Acked-by: Juergen Gross <jgross@suse.com>


Juergen

>
> Regards,
> Dario
> ---
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index e79850b..8e7b723 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -261,19 +261,13 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
>   static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
>   {
>       int ret;
> -    struct cpupool *old;
>       struct domain *d;
>
>       if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
>           return -EBUSY;
> -    old = per_cpu(cpupool, cpu);
> -    per_cpu(cpupool, cpu) = c;
>       ret = schedule_cpu_switch(cpu, c);
>       if ( ret )
> -    {
> -        per_cpu(cpupool, cpu) = old;
>           return ret;
> -    }
>
>       cpumask_clear_cpu(cpu, &cpupool_free_cpus);
>       if (cpupool_moving_cpu == cpu)
> @@ -326,7 +320,6 @@ static long cpupool_unassign_cpu_helper(void *info)
>               cpumask_clear_cpu(cpu, &cpupool_free_cpus);
>               goto out;
>           }
> -        per_cpu(cpupool, cpu) = NULL;
>           cpupool_moving_cpu = -1;
>           cpupool_put(cpupool_cpu_moving);
>           cpupool_cpu_moving = NULL;
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index d7e2d98..9072540 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1486,6 +1486,17 @@ void __init scheduler_init(void)
>           BUG();
>   }
>
> +/*
> + * Move a pCPU outside of the influence of the scheduler of its current
> + * cpupool, or subject it to the scheduler of a new cpupool.
> + *
> + * For the pCPUs that are removed from their cpupool, their scheduler becomes
> + * &ops (the default scheduler, selected at boot, which also services the
> + * default cpupool). However, as these pCPUs are not really part of any pool,
> + * there won't be any scheduling event on them, not even from the default
> + * scheduler. Basically, they will just sit idle until they are explicitly
> + * added back to a cpupool.
> + */
>   int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>   {
>       unsigned long flags;
> @@ -1494,9 +1505,24 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>       void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
>       struct scheduler *old_ops = per_cpu(scheduler, cpu);
>       struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
> +    struct cpupool *old_pool = per_cpu(cpupool, cpu);
> +
> +    /*
> +     * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
> +     * or from free to a valid cpupool. In the former case (which happens when
> +     * c is NULL), we want the CPU to have been marked as free already, as
> +     * well as to not be valid for the source pool any longer, when we get to
> +     * here. In the latter case (which happens when c is a valid cpupool), we
> +     * want the CPU to still be marked as free, as well as to not yet be valid
> +     * for the destination pool.
> +     */
> +    ASSERT(c != old_pool && (c != NULL || old_pool != NULL));
> +    ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
> +    ASSERT((c == NULL && !cpumask_test_cpu(cpu, old_pool->cpu_valid)) ||
> +           (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
>
>       if ( old_ops == new_ops )
> -        return 0;
> +        goto out;
>
>       idle = idle_vcpu[cpu];
>       ppriv = SCHED_OP(new_ops, alloc_pdata, cpu);
> @@ -1524,6 +1550,9 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>       SCHED_OP(old_ops, free_vdata, vpriv_old);
>       SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);
>
> + out:
> +    per_cpu(cpupool, cpu) = c;
> +
>       return 0;
>   }
>
>

Re: [PATCH v2 3/6] xen: sched: clarify use cases of schedule_cpu_switch()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1623 bytes --]

On Fri, 2015-10-23 at 15:42 +0200, Juergen Gross wrote:
> On 10/21/2015 07:10 PM, Dario Faggioli wrote:

> > Also, all the operations done in schedule_cpu_switch() itself,
> > depend
> > either on per_cpu(scheduler) or on per_cpu(schedule_data) being
> > updated
> > properly, rather than on per_cpu(cpupool) (including the locking
> > that
> > you are mentioning above).
> > 
> > What am I missing?
> 
> Hmm, good question. I'm rather sure I had a problem related to
> exactly
> this topic in the early days of cpupools. Maybe the critical code has
> been modified since then. Or my memory is wrong. 
>
From a quick archeological investigation, some things certainly have
changed. Still, I can't spot anything directly related to this, but
it's quite possible that it's there and I'm missing it.

> Or we both don't see
> it now. ;-)
> 
Yep! :-)

> In case there is a problem it should show up doing a test which
> concurrently does all of the following:
> 
> - move a domain between two cpupools
> - move a cpu between the two cpupools
> - create and destroy a domain in one of the two cpupools
> 
Ok, I'll arrange for this and report.

> If the system is surviving this test for a couple of hours you are
> fine
> and then for the attached patch:
> 
> Acked-by: Juergen Gross <jgross@suse.com>
> 
Thanks :-)
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 4/6] xen: sched: better handle (not) inserting idle vCPUs in runqueues

[Dario Faggioli]

Idle vCPUs are set to run immediately, as a part of their
own initialization, so we shouldn't even try to put them
in a runqueue. In fact, actual schedulers don't do that,
even when asked to (that is rather explicit in Credit2
and RTDS, a bit less evident in Credit1).

Let's make things look as follows:
 - in generic code, explicitly avoid even trying to
   insert idle vcpus in runqueues;
 - in specific schedulers' code, enforce that.

Note that, as csched_vcpu_insert() is no longer being
called, during boot, from sched_init_vcpu(), we can
safely avoid saving the flags when taking the runqueue
lock.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
Changes from v1:
 * changelog: updated and made a little bit less verbose.
---
 xen/common/sched_credit.c  |    7 ++++---
 xen/common/sched_credit2.c |   25 ++++++++++---------------
 xen/common/sched_rt.c      |    4 +---
 xen/common/schedule.c      |   21 +++++++++++----------
 4 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index e16bd3a..fc447a7 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -904,14 +904,15 @@ csched_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 {
     struct csched_vcpu *svc = vc->sched_priv;
     spinlock_t *lock;
-    unsigned long flags;
 
-    lock = vcpu_schedule_lock_irqsave(vc, &flags);
+    BUG_ON( is_idle_vcpu(vc) );
+
+    lock = vcpu_schedule_lock_irq(vc);
 
     if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
         __runq_insert(vc->processor, svc);
 
-    vcpu_schedule_unlock_irqrestore(lock, flags, vc);
+    vcpu_schedule_unlock_irq(lock, vc);
 
     SCHED_STAT_CRANK(vcpu_insert);
 }
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 6695729..59da919 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -870,30 +870,25 @@ csched2_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 {
     struct csched2_vcpu *svc = vc->sched_priv;
     struct csched2_dom * const sdom = svc->sdom;
+    spinlock_t *lock;
 
     printk("%s: Inserting %pv\n", __func__, vc);
 
-    /* NB: On boot, idle vcpus are inserted before alloc_pdata() has
-     * been called for that cpu.
-     */
-    if ( ! is_idle_vcpu(vc) )
-    {
-        spinlock_t *lock;
+    BUG_ON(is_idle_vcpu(vc));
 
-        /* FIXME: Do we need the private lock here? */
-        list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
+    /* FIXME: Do we need the private lock here? */
+    list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
 
-        /* Add vcpu to runqueue of initial processor */
-        lock = vcpu_schedule_lock_irq(vc);
+    /* Add vcpu to runqueue of initial processor */
+    lock = vcpu_schedule_lock_irq(vc);
 
-        runq_assign(ops, vc);
+    runq_assign(ops, vc);
 
-        vcpu_schedule_unlock_irq(lock, vc);
+    vcpu_schedule_unlock_irq(lock, vc);
 
-        sdom->nr_vcpus++;
+    sdom->nr_vcpus++;
 
-        SCHED_STAT_CRANK(vcpu_insert);
-    }
+    SCHED_STAT_CRANK(vcpu_insert);
 
     CSCHED2_VCPU_CHECK(vc);
 }
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 1086399..37a32a4 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -624,9 +624,7 @@ rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
     s_time_t now = NOW();
     spinlock_t *lock;
 
-    /* not addlocate idle vcpu to dom vcpu list */
-    if ( is_idle_vcpu(vc) )
-        return;
+    BUG_ON( is_idle_vcpu(vc) );
 
     lock = vcpu_schedule_lock_irq(vc);
     if ( now >= svc->cur_deadline )
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 5ebfa33..73114c5 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -240,20 +240,22 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
     init_timer(&v->poll_timer, poll_timer_fn,
                v, v->processor);
 
-    /* Idle VCPUs are scheduled immediately. */
+    v->sched_priv = SCHED_OP(DOM2OP(d), alloc_vdata, v, d->sched_priv);
+    if ( v->sched_priv == NULL )
+        return 1;
+
+    TRACE_2D(TRC_SCHED_DOM_ADD, v->domain->domain_id, v->vcpu_id);
+
+    /* Idle VCPUs are scheduled immediately, so don't put them in runqueue. */
     if ( is_idle_domain(d) )
     {
         per_cpu(schedule_data, v->processor).curr = v;
         v->is_running = 1;
     }
-
-    TRACE_2D(TRC_SCHED_DOM_ADD, v->domain->domain_id, v->vcpu_id);
-
-    v->sched_priv = SCHED_OP(DOM2OP(d), alloc_vdata, v, d->sched_priv);
-    if ( v->sched_priv == NULL )
-        return 1;
-
-    SCHED_OP(DOM2OP(d), insert_vcpu, v);
+    else
+    {
+        SCHED_OP(DOM2OP(d), insert_vcpu, v);
+    }
 
     return 0;
 }
@@ -1542,7 +1544,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     ppriv_old = per_cpu(schedule_data, cpu).sched_priv;
     per_cpu(schedule_data, cpu).sched_priv = ppriv;
     SCHED_OP(new_ops, tick_resume, cpu);
-    SCHED_OP(new_ops, insert_vcpu, idle);
 
     SCHED_OP(old_ops, free_vdata, vpriv_old);
     SCHED_OP(old_ops, free_pdata, ppriv_old, cpu);

[PATCH v2 5/6] xen: sched: get rid of the per domain vCPU list in RTDS

[Dario Faggioli]

As, curently, there is no reason for bothering having
it and keeping it updated.

In fact, it is only used for dumping and changing
vCPUs parameters, but that can be achieved easily with
for_each_vcpu.

While there, take care of the case when
XEN_DOMCTL_SCHEDOP_getinfo is called but no vCPUs have
been allocated yet (by returning the default scheduling
parameters).

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Meng Xu <mengxu@cis.upenn.edu>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes from v1:
 * address the case when d->vcpu[] has not been allocated
   yet, as requested during review;
 * style: space before brackets of for_each_vcpu, as
   requested during review;
 * used 'v' instead of 'vc' for 'vcpu' (in new code).
---
Not applying the review tags, as the patch changed a little
bit.
---
 xen/common/sched_rt.c |   54 ++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 37a32a4..3f1d047 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -160,7 +160,6 @@ struct rt_private {
  */
 struct rt_vcpu {
     struct list_head q_elem;    /* on the runq/depletedq list */
-    struct list_head sdom_elem; /* on the domain VCPU list */
 
     /* Up-pointers */
     struct rt_dom *sdom;
@@ -182,7 +181,6 @@ struct rt_vcpu {
  * Domain
  */
 struct rt_dom {
-    struct list_head vcpu;      /* link its VCPUs */
     struct list_head sdom_elem; /* link list on rt_priv */
     struct domain *dom;         /* pointer to upper domain */
 };
@@ -290,7 +288,7 @@ rt_dump_pcpu(const struct scheduler *ops, int cpu)
 static void
 rt_dump(const struct scheduler *ops)
 {
-    struct list_head *iter_sdom, *iter_svc, *runq, *depletedq, *iter;
+    struct list_head *runq, *depletedq, *iter;
     struct rt_private *prv = rt_priv(ops);
     struct rt_vcpu *svc;
     struct rt_dom *sdom;
@@ -319,14 +317,16 @@ rt_dump(const struct scheduler *ops)
     }
 
     printk("Domain info:\n");
-    list_for_each( iter_sdom, &prv->sdom )
+    list_for_each( iter, &prv->sdom )
     {
-        sdom = list_entry(iter_sdom, struct rt_dom, sdom_elem);
+        struct vcpu *v;
+
+        sdom = list_entry(iter, struct rt_dom, sdom_elem);
         printk("\tdomain: %d\n", sdom->dom->domain_id);
 
-        list_for_each( iter_svc, &sdom->vcpu )
+        for_each_vcpu ( sdom->dom, v )
         {
-            svc = list_entry(iter_svc, struct rt_vcpu, sdom_elem);
+            svc = rt_vcpu(v);
             rt_dump_vcpu(ops, svc);
         }
     }
@@ -527,7 +527,6 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom)
     if ( sdom == NULL )
         return NULL;
 
-    INIT_LIST_HEAD(&sdom->vcpu);
     INIT_LIST_HEAD(&sdom->sdom_elem);
     sdom->dom = dom;
 
@@ -587,7 +586,6 @@ rt_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
         return NULL;
 
     INIT_LIST_HEAD(&svc->q_elem);
-    INIT_LIST_HEAD(&svc->sdom_elem);
     svc->flags = 0U;
     svc->sdom = dd;
     svc->vcpu = vc;
@@ -614,8 +612,7 @@ rt_free_vdata(const struct scheduler *ops, void *priv)
  * This function is called in sched_move_domain() in schedule.c
  * When move a domain to a new cpupool.
  * It inserts vcpus of moving domain to the scheduler's RunQ in
- * dest. cpupool; and insert rt_vcpu svc to scheduler-specific
- * vcpu list of the dom
+ * dest. cpupool.
  */
 static void
 rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
@@ -634,15 +631,11 @@ rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
         __runq_insert(ops, svc);
     vcpu_schedule_unlock_irq(lock, vc);
 
-    /* add rt_vcpu svc to scheduler-specific vcpu list of the dom */
-    list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
-
     SCHED_STAT_CRANK(vcpu_insert);
 }
 
 /*
- * Remove rt_vcpu svc from the old scheduler in source cpupool; and
- * Remove rt_vcpu svc from scheduler-specific vcpu list of the dom
+ * Remove rt_vcpu svc from the old scheduler in source cpupool.
  */
 static void
 rt_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
@@ -659,9 +652,6 @@ rt_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
     if ( __vcpu_on_q(svc) )
         __q_remove(svc);
     vcpu_schedule_unlock_irq(lock, vc);
-
-    if ( !is_idle_vcpu(vc) )
-        list_del_init(&svc->sdom_elem);
 }
 
 /*
@@ -1135,20 +1125,28 @@ rt_dom_cntl(
     struct xen_domctl_scheduler_op *op)
 {
     struct rt_private *prv = rt_priv(ops);
-    struct rt_dom * const sdom = rt_dom(d);
     struct rt_vcpu *svc;
-    struct list_head *iter;
+    struct vcpu *v;
     unsigned long flags;
     int rc = 0;
 
     switch ( op->cmd )
     {
     case XEN_DOMCTL_SCHEDOP_getinfo:
-        spin_lock_irqsave(&prv->lock, flags);
-        svc = list_entry(sdom->vcpu.next, struct rt_vcpu, sdom_elem);
-        op->u.rtds.period = svc->period / MICROSECS(1); /* transfer to us */
-        op->u.rtds.budget = svc->budget / MICROSECS(1);
-        spin_unlock_irqrestore(&prv->lock, flags);
+        if ( d->max_vcpus > 0 )
+        {
+            spin_lock_irqsave(&prv->lock, flags);
+            svc = rt_vcpu(d->vcpu[0]);
+            op->u.rtds.period = svc->period / MICROSECS(1);
+            op->u.rtds.budget = svc->budget / MICROSECS(1);
+            spin_unlock_irqrestore(&prv->lock, flags);
+        }
+        else
+        {
+            /* If we don't have vcpus yet, let's just return the defaults. */
+            op->u.rtds.period = RTDS_DEFAULT_PERIOD;
+            op->u.rtds.budget = RTDS_DEFAULT_BUDGET;
+        }
         break;
     case XEN_DOMCTL_SCHEDOP_putinfo:
         if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
@@ -1157,9 +1155,9 @@ rt_dom_cntl(
             break;
         }
         spin_lock_irqsave(&prv->lock, flags);
-        list_for_each( iter, &sdom->vcpu )
+        for_each_vcpu ( d, v )
         {
-            struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
+            svc = rt_vcpu(v);
             svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
             svc->budget = MICROSECS(op->u.rtds.budget);
         }

Re: [PATCH v2 5/6] xen: sched: get rid of the per domain vCPU list in RTDS

[Meng Xu]

2015-10-14 11:54 GMT-04:00 Dario Faggioli <dario.faggioli@citrix.com>:
> As, curently, there is no reason for bothering having
> it and keeping it updated.
>
> In fact, it is only used for dumping and changing
> vCPUs parameters, but that can be achieved easily with
> for_each_vcpu.
>
> While there, take care of the case when
> XEN_DOMCTL_SCHEDOP_getinfo is called but no vCPUs have
> been allocated yet (by returning the default scheduling
> parameters).
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: Meng Xu <mengxu@cis.upenn.edu>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Changes from v1:
>  * address the case when d->vcpu[] has not been allocated
>    yet, as requested during review;
>  * style: space before brackets of for_each_vcpu, as
>    requested during review;
>  * used 'v' instead of 'vc' for 'vcpu' (in new code).
> ---
> Not applying the review tags, as the patch changed a little
> bit.
> ---
>  xen/common/sched_rt.c |   54 ++++++++++++++++++++++++-------------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
>

Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>

Best,

Meng


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

[PATCH v2 6/6] xen: sched: get rid of the per domain vCPU list in Credit2

[Dario Faggioli]

As, curently, there is no reason for bothering having
it and keeping it updated.

In fact, it is only used for dumping and changing
vCPUs parameters, but that can be achieved easily with
for_each_vcpu.

While there, improve alignment of comments, ad
add a const qualifier to a pointer, making things
more consistent with what happens everywhere else
in the source file.

This also allows us to kill one of the remaining
FIXMEs in the code, which is always good.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes from v1:
 * removed spurious hunk about sched_rt.c, as noted
   during review;
 * fixed the BUG_ON inside csched2_dom_destroy(), as
   noted during review;
 * used 'v' instead of 'vc' for 'vcpu' (in new code),
   as suggested during review.
---
Not applying the review tags, as the patch changed a little
bit.
---
 xen/common/sched_credit2.c |   34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 59da919..df85ecd 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -234,9 +234,8 @@ struct csched2_private {
  * Virtual CPU
  */
 struct csched2_vcpu {
-    struct list_head rqd_elem;  /* On the runqueue data list */
-    struct list_head sdom_elem; /* On the domain vcpu list */
-    struct list_head runq_elem; /* On the runqueue         */
+    struct list_head rqd_elem;         /* On the runqueue data list  */
+    struct list_head runq_elem;        /* On the runqueue            */
     struct csched2_runqueue_data *rqd; /* Up-pointer to the runqueue */
 
     /* Up-pointers */
@@ -261,7 +260,6 @@ struct csched2_vcpu {
  * Domain
  */
 struct csched2_dom {
-    struct list_head vcpu;
     struct list_head sdom_elem;
     struct domain *dom;
     uint16_t weight;
@@ -772,7 +770,6 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
         return NULL;
 
     INIT_LIST_HEAD(&svc->rqd_elem);
-    INIT_LIST_HEAD(&svc->sdom_elem);
     INIT_LIST_HEAD(&svc->runq_elem);
 
     svc->sdom = dd;
@@ -876,9 +873,6 @@ csched2_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 
     BUG_ON(is_idle_vcpu(vc));
 
-    /* FIXME: Do we need the private lock here? */
-    list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
-
     /* Add vcpu to runqueue of initial processor */
     lock = vcpu_schedule_lock_irq(vc);
 
@@ -923,10 +917,6 @@ csched2_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
 
         vcpu_schedule_unlock_irq(lock, vc);
 
-        /* Remove from sdom list.  Don't need a lock for this, as it's called
-         * syncronously when nothing else can happen. */
-        list_del_init(&svc->sdom_elem);
-
         svc->sdom->nr_vcpus--;
     }
 }
@@ -1443,7 +1433,7 @@ csched2_dom_cntl(
 
         if ( op->u.credit2.weight != 0 )
         {
-            struct list_head *iter;
+            struct vcpu *v;
             int old_weight;
 
             old_weight = sdom->weight;
@@ -1451,9 +1441,9 @@ csched2_dom_cntl(
             sdom->weight = op->u.credit2.weight;
 
             /* Update weights for vcpus, and max_weight for runqueues on which they reside */
-            list_for_each ( iter, &sdom->vcpu )
+            for_each_vcpu ( d, v )
             {
-                struct csched2_vcpu *svc = list_entry(iter, struct csched2_vcpu, sdom_elem);
+                struct csched2_vcpu *svc = CSCHED2_VCPU(v);
 
                 /* NB: Locking order is important here.  Because we grab this lock here, we
                  * must never lock csched2_priv.lock if we're holding a runqueue lock.
@@ -1487,7 +1477,6 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
         return NULL;
 
     /* Initialize credit and weight */
-    INIT_LIST_HEAD(&sdom->vcpu);
     INIT_LIST_HEAD(&sdom->sdom_elem);
     sdom->dom = dom;
     sdom->weight = CSCHED2_DEFAULT_WEIGHT;
@@ -1539,9 +1528,7 @@ csched2_free_domdata(const struct scheduler *ops, void *data)
 static void
 csched2_dom_destroy(const struct scheduler *ops, struct domain *dom)
 {
-    struct csched2_dom *sdom = CSCHED2_DOM(dom);
-
-    BUG_ON(!list_empty(&sdom->vcpu));
+    BUG_ON(CSCHED2_DOM(dom)->nr_vcpus > 0);
 
     csched2_free_domdata(ops, CSCHED2_DOM(dom));
 }
@@ -1881,7 +1868,7 @@ csched2_dump_pcpu(const struct scheduler *ops, int cpu)
 static void
 csched2_dump(const struct scheduler *ops)
 {
-    struct list_head *iter_sdom, *iter_svc;
+    struct list_head *iter_sdom;
     struct csched2_private *prv = CSCHED2_PRIV(ops);
     unsigned long flags;
     int i, loop;
@@ -1926,6 +1913,8 @@ csched2_dump(const struct scheduler *ops)
     list_for_each( iter_sdom, &prv->sdom )
     {
         struct csched2_dom *sdom;
+        struct vcpu *v;
+
         sdom = list_entry(iter_sdom, struct csched2_dom, sdom_elem);
 
         printk("\tDomain: %d w %d v %d\n",
@@ -1933,12 +1922,11 @@ csched2_dump(const struct scheduler *ops)
                sdom->weight,
                sdom->nr_vcpus);
 
-        list_for_each( iter_svc, &sdom->vcpu )
+        for_each_vcpu( sdom->dom, v )
         {
-            struct csched2_vcpu *svc;
+            struct csched2_vcpu * const svc = CSCHED2_VCPU(v);
             spinlock_t *lock;
 
-            svc = list_entry(iter_svc, struct csched2_vcpu, sdom_elem);
             lock = vcpu_schedule_lock(svc->vcpu);
 
             printk("\t%3d: ", ++loop);