rework locking in sched_adjust

PATCH Superseded Dario Faggioli 5 participants 21 messages Nov 23, 2011 – Dec 14, 2011

lore

Share & tools

lore

Dario Faggioli

author

2011-11-23 14:55 UTC #0 lore raw

PATCH 1/1 rework locking in sched_adjust

Hi everyone,

This series changes how locks are dealt with while adjusting domains' scheduling parameters.

I've done and am still doing tests for credit and credit2, and it's surviving to all I threw at it up to now. Unfortunately, I can't test the sedf part yet, since it is not working on my test boxes due to other issues (which I'm also trying to track down). I'm sending the series out anyway, so that at least you can have a look at it, and maybe give a spin on the sedf part, if you don't have anything more interesting to do. ;-P

Juergen series on xl scheduling support attached to this mail, in the form of a single patch, for testing convenience.

xen/common/sched_credit.c  |   10 +++++++---
xen/common/sched_credit2.c |   21 +++++++++++----------
xen/common/sched_sedf.c    |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
xen/common/schedule.c      |   34 ++--------------------------------
4 files changed, 130 insertions(+), 66 deletions(-)

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

# HG changeset patch
# Parent a80a577c34ca421fe97141eee329fb14b100faa2
Improved xl scheduling support from Juergen.

This is here as a single patch, just for convenience, in case one wants to test the series. For details see
http://osdir.com/ml/xen-development/2011-11/msg01097.html .

docs/man/xl.pod.1 +85 -5

diff -r a80a577c34ca docs/man/xl.pod.1
@@ -579,25 +579,30 @@ default B<credit> is used for scheduling
 
 =over 4
 
-=item B<sched-credit> [ B<-d> I<domain-id> [ B<-w>[B<=>I<WEIGHT>] | B<-c>[B<=>I<CAP>] ] ]
+=item B<sched-credit> [I<OPTIONS>]
 
-Set credit scheduler parameters.  The credit scheduler is a
+Set or get credit scheduler parameters.  The credit scheduler is a
 proportional fair share CPU scheduler built from the ground up to be
 work conserving on SMP hosts.
 
 Each domain (including Domain0) is assigned a weight and a cap.
 
-B<PARAMETERS>
+B<OPTIONS>
 
 =over 4
 
-=item I<WEIGHT>
+=item B<-d DOMAIN>, B<--domain=DOMAIN>
+
+Specify domain for which scheduler parameters are to be modified or retrieved.
+Mandatory for modifying scheduler parameters.
+
+=item B<-w WEIGHT>, B<--weight=WEIGHT>
 
 A domain with a weight of 512 will get twice as much CPU as a domain
 with a weight of 256 on a contended host. Legal weights range from 1
 to 65535 and the default is 256.
 
-=item I<CAP>
+=item B<-c CAP>, B<--cap=CAP>
 
 The cap optionally fixes the maximum amount of CPU a domain will be
 able to consume, even if the host system has idle CPU cycles. The cap

@@ -605,6 +610,81 @@ is expressed in percentage of one physic
 50 is half a CPU, 400 is 4 CPUs, etc. The default, 0, means there is
 no upper cap.
 
+=item B<-p CPUPOOL>, B<--cpupool=CPUPOOL>
+
+Restrict output to domains in the specified cpupool.
+
+=back
+
+=item B<sched-credit2> [I<OPTIONS>]
+
+Set or get credit2 scheduler parameters.  The credit2 scheduler is a
+proportional fair share CPU scheduler built from the ground up to be
+work conserving on SMP hosts.
+
+Each domain (including Domain0) is assigned a weight.
+
+B<OPTIONS>
+
+=over 4
+
+=item B<-d DOMAIN>, B<--domain=DOMAIN>
+
+Specify domain for which scheduler parameters are to be modified or retrieved.
+Mandatory for modifying scheduler parameters.
+
+=item B<-w WEIGHT>, B<--weight=WEIGHT>
+
+A domain with a weight of 512 will get twice as much CPU as a domain
+with a weight of 256 on a contended host. Legal weights range from 1
+to 65535 and the default is 256.
+
+=item B<-p CPUPOOL>, B<--cpupool=CPUPOOL>
+
+Restrict output to domains in the specified cpupool.
+
+=back
+
+=item B<sched-sedf> [I<OPTIONS>]
+
+Set or get Simple EDF (Earliest Deadline First) scheduler parameters. This
+scheduler provides weighted CPU sharing in an intuitive way and uses
+realtime-algorithms to ensure time guarantees.  For more information see
+docs/misc/sedf_scheduler_mini-HOWTO.txt in the Xen distribution.
+
+B<OPTIONS>
+
+=over 4
+
+=item B<-d DOMAIN>, B<--domain=DOMAIN>
+
+Specify domain for which scheduler parameters are to be modified or retrieved.
+Mandatory for modifying scheduler parameters.
+
+=item B<-p PERIOD>, B<--period=PERIOD>
+
+The normal EDF scheduling usage in milliseconds.
+
+=item B<-s SLICE>, B<--slice=SLICE>
+
+The normal EDF scheduling usage in milliseconds.
+
+=item B<-l LATENCY>, B<--latency=LATENCY>
+
+Scaled period if domain is doing heavy I/O.
+
+=item B<-e EXTRA>, B<--extra=EXTRA>
+
+Flag for allowing domain to run in extra time (0 or 1).
+
+=item B<-w WEIGHT>, B<--weight=WEIGHT>
+
+Another way of setting CPU slice.
+
+=item B<-c CPUPOOL>, B<--cpupool=CPUPOOL>
+
+Restrict output to domains in the specified cpupool.
+
 =back
 
 =back

tools/libxl/libxl.c +99 -1

118 lines not on this page · raw message

tools/libxl/libxl.h +8 -0

diff -r a80a577c34ca tools/libxl/libxl.h
@@ -567,6 +567,14 @@ int libxl_sched_credit_domain_get(libxl_
                                   libxl_sched_credit *scinfo);
 int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
                                   libxl_sched_credit *scinfo);
+int libxl_sched_credit2_domain_get(libxl_ctx *ctx, uint32_t domid,
+                                   libxl_sched_credit2 *scinfo);
+int libxl_sched_credit2_domain_set(libxl_ctx *ctx, uint32_t domid,
+                                   libxl_sched_credit2 *scinfo);
+int libxl_sched_sedf_domain_get(libxl_ctx *ctx, uint32_t domid,
+                                libxl_sched_sedf *scinfo);
+int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
+                                libxl_sched_sedf *scinfo);
 int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
                        char *trigger_name, uint32_t vcpuid);
 int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq);

tools/libxl/libxl_types.idl +13 -0

diff -r a80a577c34ca tools/libxl/libxl_types.idl
@@ -109,6 +109,7 @@ SHUTDOWN_* constant."""),
     ("cpu_time",    uint64),
     ("vcpu_max_id", uint32),
     ("vcpu_online", uint32),
+    ("cpupool",     uint32),
     ], dispose_fn=None)
 
 libxl_cpupoolinfo = Struct("cpupoolinfo", [

@@ -374,3 +375,15 @@ libxl_sched_credit = Struct("sched_credi
     ("weight", integer),
     ("cap", integer),
     ], dispose_fn=None)
+
+libxl_sched_credit2 = Struct("sched_credit2", [
+    ("weight", integer),
+    ], dispose_fn=None)
+
+libxl_sched_sedf = Struct("sched_sedf", [
+    ("period", integer),
+    ("slice", integer),
+    ("latency", integer),
+    ("extratime", integer),
+    ("weight", integer),
+    ], dispose_fn=None)

tools/libxl/xl.h +2 -0

diff -r a80a577c34ca tools/libxl/xl.h
@@ -55,6 +55,8 @@ int main_vcpuset(int argc, char **argv);
 int main_memmax(int argc, char **argv);
 int main_memset(int argc, char **argv);
 int main_sched_credit(int argc, char **argv);
+int main_sched_credit2(int argc, char **argv);
+int main_sched_sedf(int argc, char **argv);
 int main_domid(int argc, char **argv);
 int main_domname(int argc, char **argv);
 int main_rename(int argc, char **argv);

tools/libxl/xl_cmdimpl.c +381 ~~-23~~

452 lines not on this page · raw message

tools/libxl/xl_cmdtable.c +27 -2

diff -r a80a577c34ca tools/libxl/xl_cmdtable.c
@@ -192,10 +192,35 @@ struct cmd_spec cmd_table[] = {
     { "sched-credit",
       &main_sched_credit, 0,
       "Get/set credit scheduler parameters",
-      "[-d <Domain> [-w[=WEIGHT]|-c[=CAP]]]",
+      "[-d <Domain> [-w[=WEIGHT]|-c[=CAP]]] [-p CPUPOOL]",
       "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
       "-w WEIGHT, --weight=WEIGHT     Weight (int)\n"
-      "-c CAP, --cap=CAP              Cap (int)"
+      "-c CAP, --cap=CAP              Cap (int)\n"
+      "-p CPUPOOL, --cpupool=CPUPOOL  Restrict output to CPUPOOL"
+    },
+    { "sched-credit2",
+      &main_sched_credit2, 0,
+      "Get/set credit2 scheduler parameters",
+      "[-d <Domain> [-w[=WEIGHT]]] [-p CPUPOOL]",
+      "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
+      "-w WEIGHT, --weight=WEIGHT     Weight (int)\n"
+      "-p CPUPOOL, --cpupool=CPUPOOL  Restrict output to CPUPOOL"
+    },
+    { "sched-sedf",
+      &main_sched_sedf, 0,
+      "Get/set sedf scheduler parameters",
+      "[options]",
+      "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
+      "-p MS, --period=MS             Relative deadline(ms)\n"
+      "-s MS, --slice=MS              Worst-case execution time(ms). (slice <\n"
+      "                               period)\n"
+      "-l MS, --latency=MS            Scaled period (ms) when domain performs\n"
+      "                               heavy I/O\n"
+      "-e FLAG, --extra=FLAG          Flag (0 or 1) controls if domain can run\n"
+      "                               in extra time\n"
+      "-w FLOAT, --weight=FLOAT       CPU Period/slice (do not set with\n"
+      "                               --period/--slice)\n"
+      "-c CPUPOOL, --cpupool=CPUPOOL  Restrict output to CPUPOOL"
     },
     { "domid",
       &main_domid, 0,

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dario Faggioli

author

2011-11-23 15:07 UTC #1 lore raw

PATCH 1/1 Move locking into pluggable schedulers.

Pluggable schedulers' code knows what (and when) to lock better than generic code, let's delegate to them all the concurrency related issues.

Signed-off-by Dario Faggioli <[email protected]>

xen/common/sched_credit.c +7 -3

diff -r 84b3e46aa7d2 xen/common/sched_credit.c
@@ -161,6 +161,7 @@ struct csched_dom {
  * System-wide private data
  */
 struct csched_private {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;

@@ -800,6 +801,10 @@ csched_dom_cntl(
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
 
+    /* Protect both get and put branches with the pluggable scheduler
+     * lock. Runq lock not needed anywhere in here. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         op->u.credit.weight = sdom->weight;

@@ -809,8 +814,6 @@ csched_dom_cntl(
     {
         ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
 
-        spin_lock_irqsave(&prv->lock, flags);
-
         if ( op->u.credit.weight != 0 )
         {
             if ( !list_empty(&sdom->active_sdom_elem) )

@@ -824,9 +827,10 @@ csched_dom_cntl(
         if ( op->u.credit.cap != (uint16_t)~0U )
             sdom->cap = op->u.credit.cap;
 
-        spin_unlock_irqrestore(&prv->lock, flags);
     }
 
+    spin_unlock_irqrestore(&prv->lock, flags);
+
     return 0;
 }

xen/common/sched_credit2.c +11 ~~-10~~

diff -r 84b3e46aa7d2 xen/common/sched_credit2.c
@@ -1384,6 +1384,10 @@ csched_dom_cntl(
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
 
+    /* Must hold csched_priv lock to read and update sdom,
+     * runq lock to update csvcs. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         op->u.credit2.weight = sdom->weight;

@@ -1397,10 +1401,6 @@ csched_dom_cntl(
             struct list_head *iter;
             int old_weight;
 
-            /* Must hold csched_priv lock to update sdom, runq lock to
-             * update csvcs. */
-            spin_lock_irqsave(&prv->lock, flags);
-
             old_weight = sdom->weight;
 
             sdom->weight = op->u.credit2.weight;

@@ -1411,22 +1411,23 @@ csched_dom_cntl(
                 struct csched_vcpu *svc = list_entry(iter, struct csched_vcpu, sdom_elem);
 
                 /* NB: Locking order is important here.  Because we grab this lock here, we
-                 * must never lock csched_priv.lock if we're holding a runqueue
-                 * lock. */
-                vcpu_schedule_lock_irq(svc->vcpu);
+                 * must never lock csched_priv.lock if we're holding a runqueue lock.
+                 * Also, calling vcpu_schedule_lock() is enough, since IRQs have already
+                 * been disabled. */
+                vcpu_schedule_lock(svc->vcpu);
 
                 BUG_ON(svc->rqd != RQD(ops, svc->vcpu->processor));
 
                 svc->weight = sdom->weight;
                 update_max_weight(svc->rqd, svc->weight, old_weight);
 
-                vcpu_schedule_unlock_irq(svc->vcpu);
+                vcpu_schedule_unlock(svc->vcpu);
             }
-
-            spin_unlock_irqrestore(&prv->lock, flags);
         }
     }
 
+    spin_unlock_irqrestore(&prv->lock, flags);
+
     return 0;
 }

xen/common/sched_sedf.c +29 -7

diff -r 84b3e46aa7d2 xen/common/sched_sedf.c
@@ -1397,18 +1397,28 @@ static int sedf_adjust_weights(struct cp
 static int sedf_adjust(const struct scheduler *ops, struct domain *p, struct xen_domctl_scheduler_op *op)
 {
     struct vcpu *v;
-    int rc;
+    int rc = 0;
 
     PRINT(2,"sedf_adjust was called, domain-id %i new period %"PRIu64" "
           "new slice %"PRIu64"\nlatency %"PRIu64" extra:%s\n",
           p->domain_id, op->u.sedf.period, op->u.sedf.slice,
           op->u.sedf.latency, (op->u.sedf.extratime)?"yes":"no");
 
+    /* Serialize everything against runq lock to prevent concurrent update
+     * (notice all non-current VCPUs of the domain have been paused in the
+     * caller). */
+    if ( p == current->domain )
+        vcpu_schedule_lock_irq(current);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_putinfo )
     {
         /* Check for sane parameters. */
         if ( !op->u.sedf.period && !op->u.sedf.weight )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            goto out;
+        }
+
         if ( op->u.sedf.weight )
         {
             if ( (op->u.sedf.extratime & EXTRA_AWARE) &&

@@ -1443,7 +1453,11 @@ static int sedf_adjust(const struct sche
                      (op->u.sedf.period < PERIOD_MIN) ||
                      (op->u.sedf.slice  > op->u.sedf.period) ||
                      (op->u.sedf.slice  < SLICE_MIN) )
-                    return -EINVAL;
+                {
+                    rc = -EINVAL;
+                    goto out;
+                }
+
                 EDOM_INFO(v)->weight = 0;
                 EDOM_INFO(v)->extraweight = 0;
                 EDOM_INFO(v)->period_orig =

@@ -1455,7 +1469,7 @@ static int sedf_adjust(const struct sche
 
         rc = sedf_adjust_weights(p->cpupool, op);
         if ( rc )
-            return rc;
+            goto out;
 
         for_each_vcpu ( p, v )
         {

@@ -1469,7 +1483,11 @@ static int sedf_adjust(const struct sche
     else if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         if ( p->vcpu[0] == NULL )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            goto out;
+        }
+
         op->u.sedf.period    = EDOM_INFO(p->vcpu[0])->period;
         op->u.sedf.slice     = EDOM_INFO(p->vcpu[0])->slice;
         op->u.sedf.extratime = EDOM_INFO(p->vcpu[0])->status & EXTRA_AWARE;

@@ -1477,8 +1495,12 @@ static int sedf_adjust(const struct sche
         op->u.sedf.weight    = EDOM_INFO(p->vcpu[0])->weight;
     }
 
-    PRINT(2,"sedf_adjust_finished\n");
-    return 0;
+out:
+    if ( p == current->domain )
+        vcpu_schedule_unlock_irq(current);
+
+    PRINT(2,"sedf_adjust_finished with return code %d\n", rc);
+    return rc;
 }
 
 const struct scheduler sched_sedf_def = {

xen/common/schedule.c +2 ~~-15~~

diff -r 84b3e46aa7d2 xen/common/schedule.c
@@ -1015,15 +1015,8 @@ long sched_adjust(struct domain *d, stru
 
     /*
      * Most VCPUs we can simply pause. If we are adjusting this VCPU then
-     * we acquire the local schedule_lock to guard against concurrent updates.
-     *
-     * We only acquire the local schedule lock after we have paused all other
-     * VCPUs in this domain. There are two reasons for this:
-     * 1- We don't want to hold up interrupts as pausing a VCPU can
-     *    trigger a tlb shootdown.
-     * 2- Pausing other VCPUs involves briefly locking the schedule
-     *    lock of the CPU they are running on. This CPU could be the
-     *    same as ours.
+     * concurrent updates shall be prevented within the actual pluggable
+     * scheduler.
      */
 
     for_each_vcpu ( d, v )

@@ -1032,15 +1025,9 @@ long sched_adjust(struct domain *d, stru
             vcpu_pause(v);
     }
 
-    if ( d == current->domain )
-        vcpu_schedule_lock_irq(current);
-
     if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 )
         TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id);
 
-    if ( d == current->domain )
-        vcpu_schedule_unlock_irq(current);
-
     for_each_vcpu ( d, v )
     {
         if ( v != current )

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2011-11-23 16:24 UTC #2 lore raw

Re: [RFC/RFT][PATCH 1 of 3] Move locking into pluggable schedulers.

Quoted 9 lines · Dario Faggioli

Pluggable schedulers' code knows what (and when) to lock better than generic code, let's delegate to them all the concurrency related issues.
Signed-off-by Dario Faggioli <[email protected]>
diff -r 84b3e46aa7d2 xen/common/sched_credit.c

xen/common/sched_credit.c struct csched_dom { · line 167

Quoted 7 lines

xen/common/sched_credit.c +1 -0

@@ -161,6 +161,7 @@ struct csched_dom {
  * System-wide private data
  */
 struct csched_private {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;

Given that every scheduler plugin is going to need this lock perhaps it could be provided globally (but still have the responsibility for using it fall on the plugin)?

I was mainly thinking of the sedf case where you add and maintain the whole structure for just that lock. Perhaps you have future plans which involve having to do that anyway in which case maybe my suggestion doesn't make sense.

Ian.

Dario Faggioli

author

2011-11-23 17:09 UTC #3 lore raw

Re: [RFC/RFT][PATCH 1 of 3] Move locking into pluggable schedulers.

Quoted 10 lines · Ian Campbell

Quoted 5 lines
struct csched_private {
+ /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;
Given that every scheduler plugin is going to need this lock perhaps it could be provided globally (but still have the responsibility for using it fall on the plugin)?

Makes sense to me, and it should be something pretty easy to do, if you were thinking of just moving the lock to general code.
I'm saying this because both credit and credit2 has much more payload in their `struct csched_private', and if we also want to get rid of the struct for them as well, that would be a different story! :-)

Quoted 5 lines

I was mainly thinking of the sedf case where you add and maintain the whole structure for just that lock. Perhaps you have future plans which involve having to do that anyway in which case maybe my suggestion doesn't make sense.

I know and I agree, that 1-field-struct is just as ugly as hell! Reason why I went for it are homogeneity with the current code of all the schedulers, and yes, also, what you're saying above, i.e., it might turn useful in future to have some scheduler-wide repository in sedf as it is now for credit*. But no, I don't have _specific_ plans yet, so your comment do make sense.

Anyway, even if we'd stay with what's in this patch, I think this at least need some commenting...

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2011-11-23 17:30 UTC #4 lore raw

Re: [RFC/RFT][PATCH 1 of 3] Move locking into pluggable schedulers.

Quoted 11 lines · Dario Faggioli

Quoted 10 lines · Ian Campbell
Quoted 5 lines
struct csched_private {
+ /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;
Given that every scheduler plugin is going to need this lock perhaps it could be provided globally (but still have the responsibility for using it fall on the plugin)?

Quoted 5 lines

Makes sense to me, and it should be something pretty easy to do, if you were thinking of just moving the lock to general code.
I'm saying this because both credit and credit2 has much more payload in their `struct csched_private', and if we also want to get rid of the struct for them as well, that would be a different story! :-)

No, I just meant the lock.

Quoted 19 lines

Quoted 6 lines
I was mainly thinking of the sedf case where you add and maintain the whole structure for just that lock. Perhaps you have future plans which involve having to do that anyway in which case maybe my suggestion doesn't make sense.
I know and I agree, that 1-field-struct is just as ugly as hell! Reason why I went for it are homogeneity with the current code of all the schedulers, and yes, also, what you're saying above, i.e., it might turn useful in future to have some scheduler-wide repository in sedf as it is now for credit*. But no, I don't have _specific_ plans yet, so your comment do make sense.
Anyway, even if we'd stay with what's in this patch, I think this at least need some commenting...
Thanks and Regards,
Dario

George Dunlap

2011-12-06 10:34 UTC #5 lore raw

Re: [RFC/RFT][PATCH 1 of 3] Move locking into pluggable schedulers.

In reply to Ian Campbell

Quoted 6 lines · Ian Campbell

Quoted 5 lines · Dario Faggioli
Pluggable schedulers' code knows what (and when) to lock better than generic code, let's delegate to them all the concurrency related issues.
Signed-off-by Dario Faggioli <[email protected]>

Quoted 15 lines

xen/common/sched_credit.c struct csched_dom { · line 167
Quoted 11 lines
diff -r 84b3e46aa7d2 xen/common/sched_credit.c
xen/common/sched_credit.c +1 -0
@@ -161,6 +161,7 @@ struct csched_dom {
  * System-wide private data
  */
 struct csched_private {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;
Given that every scheduler plugin is going to need this lock perhaps it could be provided globally (but still have the responsibility for using it fall on the plugin)?

Sorry for the long delay in responding... I don't really like this idea. For one thing, that would make the generic scheduler code responsible for making per-cpupool locks, which could look messy, and adds to code complexity. There's already code to allocate per-pool data structures for the schedulers -- it seems like just leveraging that would be easiest. I also think that logically, having each scheduler in charge of its own locking makes more sense; having the generic scheduling code do stuff on behalf of the pluggable scheduler is what caused this problem in the first place.

If we think having a one-item struct is ugly, could we just use spinlock_t as the type we allocate / cast to in the sedf scheduler?

-George

Dario Faggioli

author

2011-12-06 16:35 UTC #6 lore raw

Re: [RFC/RFT][PATCH 1 of 3] Move locking into pluggable schedulers.

Quoted 9 lines · George Dunlap

Quoted 3 lines
Given that every scheduler plugin is going to need this lock perhaps it could be provided globally (but still have the responsibility for using it fall on the plugin)?
Sorry for the long delay in responding... I don't really like this idea. For one thing, that would make the generic scheduler code responsible for making per-cpupool locks, which could look messy, and adds to code complexity.

Right. I was looking into this and facing right this issue, i.e., how to make that lock a per-cpupool thing, and wasn't quite succeeding in doing that in a clean way.

Quoted 5 lines

I also think that logically, having each scheduler in charge of its own locking makes more sense; having the generic scheduling code do stuff on behalf of the pluggable scheduler is what caused this problem in the first place.

Indeed.

Quoted 3 lines

If we think having a one-item struct is ugly, could we just use spinlock_t as the type we allocate / cast to in the sedf scheduler?

Well, I put that struct there in the first place so I definitely can live with it. I certainly don't think it is the most beautiful piece of code I've ever wrote but, considering I'd have to dynamically allocate the lock anyway (for the reasons above), and access it through ops->sched_data, having it pointing directly to the lock is not that much better than the struct to me.

Therefore, I think I'll let you decide here, and will keep or kill the struct basing on what you think is nicer! :-)

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

Dario Faggioli

author

2011-12-07 14:49 UTC #7 lore raw

PATCH v2 1/1 Rework locking for sched_adjust.

In reply to Ian Campbell

Hi everyone,

Here it is v2 of the lock reworking around and within sched-adjust.

With respect to the first posting [1]:
- I _did_not_ move the per-pluggable scheduler lock toward schedule.c, as agreed with George during review;
- I fixed the bug in sedf spotted by Juergen the way he suggested;
- I've finally been able to test it under all the three schedulers, and it is doing its job, at least here;

Notice the series "collapsed" in one signle patch, as it was being hard to find a breakdown of it that does not introduce regressions and/or transient deadlock situations worse than the ones it's trying to cure... I hope it's still readable and comfortable to review. :-)

Thanks to everyone who provided his feedback on the first version of this.

Regards,
Dario

[1] http://xen.1045712.n5.nabble.com/RFC-RFT-PATCH-0-of-3-rework-locking-in-sched-adjust-td5016899.html

 xen/common/sched_credit.c  |   10 ++++++--
 xen/common/sched_credit2.c |   21 ++++++++++---------
 xen/common/sched_sedf.c    |  156 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------
 xen/common/schedule.c      |   34 +-------------------------------
 4 files changed, 140 insertions(+), 81 deletions(-)

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dario Faggioli

author

2011-12-07 15:02 UTC #8 lore raw

PATCH v2 1/1 Rework locking for sched_adjust.

The main idea is to move (as much as possible) locking logic from generic code to the various pluggable schedulers.

While at it, the following is also accomplished:
- pausing all the non-current VCPUs of a domain while changing its scheduling parameters is not effective in avoiding races and it is prone to deadlock, so that is removed.
- sedf needs a global lock for preventing races while adjusting domains' scheduling parameters (as it is for credit and credit2), so that is added.

Signed-off-by Dario Faggioli <[email protected]>

xen/common/sched_credit.c +7 -3

diff -r 38eb74c01d9d xen/common/sched_credit.c
@@ -161,6 +161,7 @@ struct csched_dom {
  * System-wide private data
  */
 struct csched_private {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;

@@ -800,6 +801,10 @@ csched_dom_cntl(
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
 
+    /* Protect both get and put branches with the pluggable scheduler
+     * lock. Runq lock not needed anywhere in here. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         op->u.credit.weight = sdom->weight;

@@ -809,8 +814,6 @@ csched_dom_cntl(
     {
         ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
 
-        spin_lock_irqsave(&prv->lock, flags);
-
         if ( op->u.credit.weight != 0 )
         {
             if ( !list_empty(&sdom->active_sdom_elem) )

@@ -824,9 +827,10 @@ csched_dom_cntl(
         if ( op->u.credit.cap != (uint16_t)~0U )
             sdom->cap = op->u.credit.cap;
 
-        spin_unlock_irqrestore(&prv->lock, flags);
     }
 
+    spin_unlock_irqrestore(&prv->lock, flags);
+
     return 0;
 }

xen/common/sched_credit2.c +11 ~~-10~~

diff -r 38eb74c01d9d xen/common/sched_credit2.c
@@ -1384,6 +1384,10 @@ csched_dom_cntl(
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
 
+    /* Must hold csched_priv lock to read and update sdom,
+     * runq lock to update csvcs. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         op->u.credit2.weight = sdom->weight;

@@ -1397,10 +1401,6 @@ csched_dom_cntl(
             struct list_head *iter;
             int old_weight;
 
-            /* Must hold csched_priv lock to update sdom, runq lock to
-             * update csvcs. */
-            spin_lock_irqsave(&prv->lock, flags);
-
             old_weight = sdom->weight;
 
             sdom->weight = op->u.credit2.weight;

@@ -1411,22 +1411,23 @@ csched_dom_cntl(
                 struct csched_vcpu *svc = list_entry(iter, struct csched_vcpu, sdom_elem);
 
                 /* NB: Locking order is important here.  Because we grab this lock here, we
-                 * must never lock csched_priv.lock if we're holding a runqueue
-                 * lock. */
-                vcpu_schedule_lock_irq(svc->vcpu);
+                 * must never lock csched_priv.lock if we're holding a runqueue lock.
+                 * Also, calling vcpu_schedule_lock() is enough, since IRQs have already
+                 * been disabled. */
+                vcpu_schedule_lock(svc->vcpu);
 
                 BUG_ON(svc->rqd != RQD(ops, svc->vcpu->processor));
 
                 svc->weight = sdom->weight;
                 update_max_weight(svc->rqd, svc->weight, old_weight);
 
-                vcpu_schedule_unlock_irq(svc->vcpu);
+                vcpu_schedule_unlock(svc->vcpu);
             }
-
-            spin_unlock_irqrestore(&prv->lock, flags);
         }
     }
 
+    spin_unlock_irqrestore(&prv->lock, flags);
+
     return 0;
 }

xen/common/sched_sedf.c +120 ~~-36~~

285 lines not on this page · raw message

xen/common/schedule.c +2 ~~-32~~

diff -r 38eb74c01d9d xen/common/schedule.c
@@ -1005,7 +1005,6 @@ int sched_id(void)
 /* Adjust scheduling parameter for a given domain. */
 long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
 {
-    struct vcpu *v;
     long ret;
     
     if ( (op->sched_id != DOM2OP(d)->sched_id) ||

@@ -1013,40 +1012,11 @@ long sched_adjust(struct domain *d, stru
           (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
         return -EINVAL;
 
-    /*
-     * Most VCPUs we can simply pause. If we are adjusting this VCPU then
-     * we acquire the local schedule_lock to guard against concurrent updates.
-     *
-     * We only acquire the local schedule lock after we have paused all other
-     * VCPUs in this domain. There are two reasons for this:
-     * 1- We don't want to hold up interrupts as pausing a VCPU can
-     *    trigger a tlb shootdown.
-     * 2- Pausing other VCPUs involves briefly locking the schedule
-     *    lock of the CPU they are running on. This CPU could be the
-     *    same as ours.
-     */
-
-    for_each_vcpu ( d, v )
-    {
-        if ( v != current )
-            vcpu_pause(v);
-    }
-
-    if ( d == current->domain )
-        vcpu_schedule_lock_irq(current);
-
+    /* NB: the pluggable scheduler code needs to take care
+     * of locking by itself. */
     if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 )
         TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id);
 
-    if ( d == current->domain )
-        vcpu_schedule_unlock_irq(current);
-
-    for_each_vcpu ( d, v )
-    {
-        if ( v != current )
-            vcpu_unpause(v);
-    }
-
     return ret;
 }

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

George Dunlap

2011-12-14 10:24 UTC #9 lore raw

Re: [PATCHv2 1 of 1] Rework locking for sched_adjust.

Dario, this patch won't apply due to wordwrap damage. Try using "hg email", or sending as an attachment.

Thanks,
-George

Quoted 464 lines · Dario Faggioli

The main idea is to move (as much as possible) locking logic from generic code to the various pluggable schedulers.

Signed-off-by Dario Faggioli <[email protected]>

diff -r 38eb74c01d9d xen/common/sched_credit.c

xen/common/sched_credit.c +7 -3

@@ -161,6 +161,7 @@ struct csched_dom {
  * System-wide private data
  */
 struct csched_private {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
     spinlock_t lock;
     struct list_head active_sdom;
     uint32_t ncpus;

@@ -800,6 +801,10 @@ csched_dom_cntl(
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
 
+    /* Protect both get and put branches with the pluggable scheduler
+     * lock. Runq lock not needed anywhere in here. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         op->u.credit.weight = sdom->weight;

@@ -809,8 +814,6 @@ csched_dom_cntl(
     {
         ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
 
-        spin_lock_irqsave(&prv->lock, flags);
-
         if ( op->u.credit.weight != 0 )
         {
             if ( !list_empty(&sdom->active_sdom_elem) )

@@ -824,9 +827,10 @@ csched_dom_cntl(
         if ( op->u.credit.cap != (uint16_t)~0U )
             sdom->cap = op->u.credit.cap;
 
-        spin_unlock_irqrestore(&prv->lock, flags);
     }
 
+    spin_unlock_irqrestore(&prv->lock, flags);
+
     return 0;
 }

diff -r 38eb74c01d9d xen/common/sched_credit2.c

xen/common/sched_credit2.c +11 ~~-10~~

@@ -1384,6 +1384,10 @@ csched_dom_cntl(
     struct csched_private *prv = CSCHED_PRIV(ops);
     unsigned long flags;
 
+    /* Must hold csched_priv lock to read and update sdom,
+     * runq lock to update csvcs. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         op->u.credit2.weight = sdom->weight;

@@ -1397,10 +1401,6 @@ csched_dom_cntl(
             struct list_head *iter;
             int old_weight;
 
-            /* Must hold csched_priv lock to update sdom, runq lock to
-             * update csvcs. */
-            spin_lock_irqsave(&prv->lock, flags);
-
             old_weight = sdom->weight;
 
             sdom->weight = op->u.credit2.weight;

@@ -1411,22 +1411,23 @@ csched_dom_cntl(
                 struct csched_vcpu *svc = list_entry(iter, struct csched_vcpu, sdom_elem);
 
                 /* NB: Locking order is important here.  Because we grab this lock here, we
-                 * must never lock csched_priv.lock if we're holding a runqueue
-                 * lock. */
-                vcpu_schedule_lock_irq(svc->vcpu);
+                 * must never lock csched_priv.lock if we're holding a runqueue lock.
+                 * Also, calling vcpu_schedule_lock() is enough, since IRQs have already
+                 * been disabled. */
+                vcpu_schedule_lock(svc->vcpu);
 
                 BUG_ON(svc->rqd != RQD(ops, svc->vcpu->processor));
 
                 svc->weight = sdom->weight;
                 update_max_weight(svc->rqd, svc->weight, old_weight);
 
-                vcpu_schedule_unlock_irq(svc->vcpu);
+                vcpu_schedule_unlock(svc->vcpu);
             }
-
-            spin_unlock_irqrestore(&prv->lock, flags);
         }
     }
 
+    spin_unlock_irqrestore(&prv->lock, flags);
+
     return 0;
 }

diff -r 38eb74c01d9d xen/common/sched_sedf.c

xen/common/sched_sedf.c +120 ~~-36~~

@@ -61,6 +61,11 @@ struct sedf_dom_info {
     struct domain  *domain;
 };
 
+struct sedf_priv_info {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
+    spinlock_t lock;
+};
+
 struct sedf_vcpu_info {
     struct vcpu *vcpu;
     struct list_head list;

@@ -115,6 +120,8 @@ struct sedf_cpu_info {
     s_time_t         current_slice_expires;
 };
 
+#define SEDF_PRIV(_ops) \
+    ((struct sedf_priv_info *)((_ops)->sched_data))
 #define EDOM_INFO(d)   ((struct sedf_vcpu_info *)((d)->sched_priv))
 #define CPU_INFO(cpu)  \
     ((struct sedf_cpu_info *)per_cpu(schedule_data, cpu).sched_priv)

@@ -772,6 +779,31 @@ static struct task_slice sedf_do_extra_s
 }
 
 
+static int sedf_init(struct scheduler *ops)
+{
+    struct sedf_priv_info *prv;
+
+    prv = xzalloc(struct sedf_priv_info);
+    if ( prv == NULL )
+        return -ENOMEM;
+
+    ops->sched_data = prv;
+    spin_lock_init(&prv->lock);
+
+    return 0;
+}
+
+
+static void sedf_deinit(const struct scheduler *ops)
+{
+    struct sedf_priv_info *prv;
+
+    prv = SEDF_PRIV(ops);
+    if ( prv != NULL )
+        xfree(prv);
+}
+
+
 /* Main scheduling function
    Reasons for calling this function are:
    -timeslice for the current period used up

@@ -1320,22 +1352,15 @@ static void sedf_dump_cpu_state(const st
 
 
 /* Adjusts periods and slices of the domains accordingly to their weights. */
-static int sedf_adjust_weights(struct cpupool *c, struct xen_domctl_scheduler_op *cmd)
+static int sedf_adjust_weights(struct cpupool *c, int nr_cpus, int *sumw, s_time_t *sumt)
 {
     struct vcpu *p;
     struct domain      *d;
-    unsigned int        cpu, nr_cpus = cpumask_last(&cpu_online_map) + 1;
-    int                *sumw = xzalloc_array(int, nr_cpus);
-    s_time_t           *sumt = xzalloc_array(s_time_t, nr_cpus);
+    unsigned int        cpu;
 
-    if ( !sumw || !sumt )
-    {
-        xfree(sumt);
-        xfree(sumw);
-        return -ENOMEM;
-    }
-
-    /* Sum across all weights. */
+    /* Sum across all weights. Notice that no runq locking is needed
+     * here: the caller holds sedf_priv_info.lock and we're not changing
+     * anything that is accessed during scheduling. */
     rcu_read_lock(&domlist_read_lock);
     for_each_domain_in_cpupool( d, c )
     {

@@ -1365,7 +1390,9 @@ static int sedf_adjust_weights(struct cp
     }
     rcu_read_unlock(&domlist_read_lock);
 
-    /* Adjust all slices (and periods) to the new weight. */
+    /* Adjust all slices (and periods) to the new weight. Unlike above, we
+     * need to take thr runq lock for the various VCPUs: we're modyfing
+     * slice and period which are referenced during scheduling. */
     rcu_read_lock(&domlist_read_lock);
     for_each_domain_in_cpupool( d, c )
     {

@@ -1375,20 +1402,20 @@ static int sedf_adjust_weights(struct cp
                 continue;
             if ( EDOM_INFO(p)->weight )
             {
+                /* Interrupts already off */
+                vcpu_schedule_lock(p);
                 EDOM_INFO(p)->period_orig = 
                     EDOM_INFO(p)->period  = WEIGHT_PERIOD;
                 EDOM_INFO(p)->slice_orig  =
                     EDOM_INFO(p)->slice   = 
                     (EDOM_INFO(p)->weight *
                      (WEIGHT_PERIOD - WEIGHT_SAFETY - sumt[cpu])) / sumw[cpu];
+                vcpu_schedule_unlock(p);
             }
         }
     }
     rcu_read_unlock(&domlist_read_lock);
 
-    xfree(sumt);
-    xfree(sumw);
-
     return 0;
 }

@@ -1396,19 +1423,45 @@ static int sedf_adjust_weights(struct cp
 /* set or fetch domain scheduling parameters */
 static int sedf_adjust(const struct scheduler *ops, struct domain *p, struct xen_domctl_scheduler_op *op)
 {
+    struct sedf_priv_info *prv = SEDF_PRIV(ops);
+    unsigned long flags;
+    unsigned int nr_cpus = cpumask_last(&cpu_online_map) + 1;
+    int *sumw = xzalloc_array(int, nr_cpus);
+    s_time_t *sumt = xzalloc_array(s_time_t, nr_cpus);
     struct vcpu *v;
-    int rc;
+    int rc = 0;
 
     PRINT(2,"sedf_adjust was called, domain-id %i new period %"PRIu64" "
           "new slice %"PRIu64"\nlatency %"PRIu64" extra:%s\n",
           p->domain_id, op->u.sedf.period, op->u.sedf.slice,
           op->u.sedf.latency, (op->u.sedf.extratime)?"yes":"no");
 
+    /* Serialize against the pluggable scheduler lock to protect from
+     * concurrent updates. We need to take the runq lock for the VCPUs
+     * as well, since we are touching extraweight, weight, slice and
+     * period. As in sched_credit2.c, runq locks nest inside the
+     * pluggable scheduler lock. */
+    spin_lock_irqsave(&prv->lock, flags);
+
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_putinfo )
     {
+        /* These are used in sedf_adjust_weights() but have to be allocated in
+         * this function, as we need to avoid nesting xmem_pool_alloc's lock
+         * within our prv->lock. */
+        if ( !sumw || !sumt )
+        {
+            /* Check for errors here, the _getinfo branch doesn't care */
+            rc = -ENOMEM;
+            goto out;
+        }
+
         /* Check for sane parameters. */
         if ( !op->u.sedf.period && !op->u.sedf.weight )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            goto out;
+        }
+
         if ( op->u.sedf.weight )
         {
             if ( (op->u.sedf.extratime & EXTRA_AWARE) &&

@@ -1417,59 +1470,78 @@ static int sedf_adjust(const struct sche
                 /* Weight-driven domains with extratime only. */
                 for_each_vcpu ( p, v )
                 {
+                    /* (Here and everywhere in the following) IRQs are already off,
+                     * hence vcpu_spin_lock() is the one. */
+                    vcpu_schedule_lock(v);
                     EDOM_INFO(v)->extraweight = op->u.sedf.weight;
                     EDOM_INFO(v)->weight = 0;
                     EDOM_INFO(v)->slice = 0;
                     EDOM_INFO(v)->period = WEIGHT_PERIOD;
+                    vcpu_schedule_unlock(v);
                 }
             }
             else
             {
                 /* Weight-driven domains with real-time execution. */
-                for_each_vcpu ( p, v )
+                for_each_vcpu ( p, v ) {
+                    vcpu_schedule_lock(v);
                     EDOM_INFO(v)->weight = op->u.sedf.weight;
+                    vcpu_schedule_unlock(v);
+                }
             }
         }
         else
         {
+            /*
+             * Sanity checking: note that disabling extra weight requires
+             * that we set a non-zero slice.
+             */
+            if ( (op->u.sedf.period > PERIOD_MAX) ||
+                 (op->u.sedf.period < PERIOD_MIN) ||
+                 (op->u.sedf.slice  > op->u.sedf.period) ||
+                 (op->u.sedf.slice  < SLICE_MIN) )
+            {
+                rc = -EINVAL;
+                goto out;
+            }
+
             /* Time-driven domains. */
             for_each_vcpu ( p, v )
             {
-                /*
-                 * Sanity checking: note that disabling extra weight requires
-                 * that we set a non-zero slice.
-                 */
-                if ( (op->u.sedf.period > PERIOD_MAX) ||
-                     (op->u.sedf.period < PERIOD_MIN) ||
-                     (op->u.sedf.slice  > op->u.sedf.period) ||
-                     (op->u.sedf.slice  < SLICE_MIN) )
-                    return -EINVAL;
+                vcpu_schedule_lock(v);
                 EDOM_INFO(v)->weight = 0;
                 EDOM_INFO(v)->extraweight = 0;
                 EDOM_INFO(v)->period_orig = 
                     EDOM_INFO(v)->period  = op->u.sedf.period;
                 EDOM_INFO(v)->slice_orig  = 
                     EDOM_INFO(v)->slice   = op->u.sedf.slice;
+                vcpu_schedule_unlock(v);
             }
         }
 
-        rc = sedf_adjust_weights(p->cpupool, op);
+        rc = sedf_adjust_weights(p->cpupool, nr_cpus, sumw, sumt);
         if ( rc )
-            return rc;
+            goto out;
 
         for_each_vcpu ( p, v )
         {
+            vcpu_schedule_lock(v);
             EDOM_INFO(v)->status  = 
                 (EDOM_INFO(v)->status &
                  ~EXTRA_AWARE) | (op->u.sedf.extratime & EXTRA_AWARE);
             EDOM_INFO(v)->latency = op->u.sedf.latency;
             extraq_check(v);
+            vcpu_schedule_unlock(v);
         }
     }
     else if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
         if ( p->vcpu[0] == NULL )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            goto out;
+        }
+
         op->u.sedf.period    = EDOM_INFO(p->vcpu[0])->period;
         op->u.sedf.slice     = EDOM_INFO(p->vcpu[0])->slice;
         op->u.sedf.extratime = EDOM_INFO(p->vcpu[0])->status & EXTRA_AWARE;

@@ -1477,14 +1549,23 @@ static int sedf_adjust(const struct sche
         op->u.sedf.weight    = EDOM_INFO(p->vcpu[0])->weight;
     }
 
-    PRINT(2,"sedf_adjust_finished\n");
-    return 0;
+out:
+    spin_unlock_irqrestore(&prv->lock, flags);
+
+    xfree(sumt);
+    xfree(sumw);
+
+    PRINT(2,"sedf_adjust_finished with return code %d\n", rc);
+    return rc;
 }
 
+static struct sedf_priv_info _sedf_priv;
+
 const struct scheduler sched_sedf_def = {
-    .name     = "Simple EDF Scheduler",
-    .opt_name = "sedf",
-    .sched_id = XEN_SCHEDULER_SEDF,
+    .name           = "Simple EDF Scheduler",
+    .opt_name       = "sedf",
+    .sched_id       = XEN_SCHEDULER_SEDF,
+    .sched_data     = &_sedf_priv,
 
     .init_domain    = sedf_init_domain,
     .destroy_domain = sedf_destroy_domain,

@@ -1498,6 +1579,9 @@ const struct scheduler sched_sedf_def = 
     .alloc_domdata  = sedf_alloc_domdata,
     .free_domdata   = sedf_free_domdata,
 
+    .init           = sedf_init,
+    .deinit         = sedf_deinit,
+
     .do_schedule    = sedf_do_schedule,
     .pick_cpu       = sedf_pick_cpu,
     .dump_cpu_state = sedf_dump_cpu_state,

diff -r 38eb74c01d9d xen/common/schedule.c

xen/common/schedule.c +2 ~~-32~~

@@ -1005,7 +1005,6 @@ int sched_id(void)
 /* Adjust scheduling parameter for a given domain. */
 long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
 {
-    struct vcpu *v;
     long ret;
 
     if ( (op->sched_id != DOM2OP(d)->sched_id) ||

@@ -1013,40 +1012,11 @@ long sched_adjust(struct domain *d, stru
           (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
         return -EINVAL;
 
-    /*
-     * Most VCPUs we can simply pause. If we are adjusting this VCPU then
-     * we acquire the local schedule_lock to guard against concurrent updates.
-     *
-     * We only acquire the local schedule lock after we have paused all other
-     * VCPUs in this domain. There are two reasons for this:
-     * 1- We don't want to hold up interrupts as pausing a VCPU can
-     *    trigger a tlb shootdown.
-     * 2- Pausing other VCPUs involves briefly locking the schedule
-     *    lock of the CPU they are running on. This CPU could be the
-     *    same as ours.
-     */
-
-    for_each_vcpu ( d, v )
-    {
-        if ( v != current )
-            vcpu_pause(v);
-    }
-
-    if ( d == current->domain )
-        vcpu_schedule_lock_irq(current);
-
+    /* NB: the pluggable scheduler code needs to take care
+     * of locking by itself. */
     if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 )
         TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id);
 
-    if ( d == current->domain )
-        vcpu_schedule_unlock_irq(current);
-
-    for_each_vcpu ( d, v )
-    {
-        if ( v != current )
-            vcpu_unpause(v);
-    }
-
     return ret;
 }

Dario Faggioli

author

2011-12-07 15:04 UTC #10 lore raw

Re: [PATCHv2 0 of 1] Rework locking for sched_adjust.

In reply to Dario Faggioli

Quoted 2 lines · Dario Faggioli

Hi everyone,

Ok, apparently Evolution (with maybe a little help from me too! :-P) messed up with threading... For the next time, I'll take a serious look at that thing called patchbomb, I promise! :-P

Sorry for this,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dario Faggioli

author

2011-11-23 15:09 UTC #11 lore raw

PATCH 1/1 Remove VCPU pausing while adjusting domain scheduling parameters.

In reply to Dario Faggioli

Pausing all the non-current VCPUs of a domain while changing its scheduling parameters is dangerous (what if two VCPUs try to pause all the other VCPUs but themselves at the same time?). Moreover, it does not appear to be needed, since:
- races are (or should be) avoided within each pluggable scheduler by means of proper locking;
- changes in values using during actual scheduling are (or should be) prevented by runq locking.

Just remove it.

Signed-off-by Dario Faggioli <[email protected]>

xen/common/schedule.c +2 ~~-19~~

diff -r 6b5e2eb81706 xen/common/schedule.c
@@ -1005,7 +1005,6 @@ int sched_id(void)
 /* Adjust scheduling parameter for a given domain. */
 long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
 {
-    struct vcpu *v;
     long ret;
     
     if ( (op->sched_id != DOM2OP(d)->sched_id) ||

@@ -1013,27 +1012,11 @@ long sched_adjust(struct domain *d, stru
           (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
         return -EINVAL;
 
-    /*
-     * Most VCPUs we can simply pause. If we are adjusting this VCPU then
-     * concurrent updates shall be prevented within the actual pluggable
-     * scheduler.
-     */
-
-    for_each_vcpu ( d, v )
-    {
-        if ( v != current )
-            vcpu_pause(v);
-    }
-
+    /* NB: the pluggable scheduler code needs to take care
+     * of locking by itself. */
     if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 )
         TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id);
 
-    for_each_vcpu ( d, v )
-    {
-        if ( v != current )
-            vcpu_unpause(v);
-    }
-
     return ret;
 }

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dario Faggioli

author

2011-11-23 15:11 UTC #12 lore raw

PATCH 1/1 Introduce proper locking in sedf.

In reply to Dario Faggioli

Add a global scheduler lock in sedf, as we have in credit and credit2, mostly for preventing races while adjusting scheduling parameters. Also, add runq locking within sedf_adjust_weights since we are touching scheduling sensitive values in there.

Signed-off-by Dario Faggioli <[email protected]>

xen/common/sched_sedf.c +93 ~~-26~~

diff -r 17653f5c2995 xen/common/sched_sedf.c
@@ -61,6 +61,11 @@ struct sedf_dom_info {
     struct domain  *domain;
 };
 
+struct sedf_priv_info {
+    /* lock for the whole pluggable scheduler, nests inside cpupool_lock */
+    spinlock_t lock;
+};
+
 struct sedf_vcpu_info {
     struct vcpu *vcpu;
     struct list_head list;

@@ -115,6 +120,8 @@ struct sedf_cpu_info {
     s_time_t         current_slice_expires;
 };
 
+#define SEDF_PRIV(_ops) \
+    ((struct sedf_priv_info *)((_ops)->sched_data))
 #define EDOM_INFO(d)   ((struct sedf_vcpu_info *)((d)->sched_priv))
 #define CPU_INFO(cpu)  \
     ((struct sedf_cpu_info *)per_cpu(schedule_data, cpu).sched_priv)

@@ -772,6 +779,31 @@ static struct task_slice sedf_do_extra_s
 }
 
 
+static int sedf_init(struct scheduler *ops)
+{
+    struct sedf_priv_info *prv;
+
+    prv = xzalloc(struct sedf_priv_info);
+    if ( prv == NULL )
+        return -ENOMEM;
+
+    ops->sched_data = prv;
+    spin_lock_init(&prv->lock);
+
+    return 0;
+}
+
+
+static void sedf_deinit(const struct scheduler *ops)
+{
+    struct sedf_priv_info *prv;
+
+    prv = SEDF_PRIV(ops);
+    if ( prv != NULL )
+        xfree(prv);
+}
+
+
 /* Main scheduling function
    Reasons for calling this function are:
    -timeslice for the current period used up

@@ -1247,11 +1279,18 @@ static void sedf_dump_domain(struct vcpu
 /* dumps all domains on the specified cpu */
 static void sedf_dump_cpu_state(const struct scheduler *ops, int i)
 {
+    struct sedf_priv_info *prv = SEDF_PRIV(ops);
     struct list_head      *list, *queue, *tmp;
     struct sedf_vcpu_info *d_inf;
     struct domain         *d;
     struct vcpu    *ed;
     int loop = 0;
+    unsigned long flags;
+
+    /* Lock the scheduler, we want to be sure we dump a
+     * consistent set of parameters.
+     */
+    spin_lock_irqsave(&prv->lock, flags);
  
     printk("now=%"PRIu64"\n",NOW());
     queue = RUNQ(i);

@@ -1316,6 +1355,8 @@ static void sedf_dump_cpu_state(const st
         }
     }
     rcu_read_unlock(&domlist_read_lock);
+
+    spin_unlock_irqrestore(&prv->lock, flags);
 }

@@ -1335,7 +1376,9 @@ static int sedf_adjust_weights(struct cp
         return -ENOMEM;
     }
 
-    /* Sum across all weights. */
+    /* Sum across all weights. Notice that no runq locking is needed
+     * here: the caller holds sedf_priv_info->lock and we're not changing
+     * anything that is accessed during scheduling. */
     rcu_read_lock(&domlist_read_lock);
     for_each_domain_in_cpupool( d, c )
     {

@@ -1365,7 +1408,9 @@ static int sedf_adjust_weights(struct cp
     }
     rcu_read_unlock(&domlist_read_lock);
 
-    /* Adjust all slices (and periods) to the new weight. */
+    /* Adjust all slices (and periods) to the new weight. Unlike above, we
+     * need to take thr runq lock for the various VCPUs: we're modyfing
+     * slice and period which are referenced during scheduling. */
     rcu_read_lock(&domlist_read_lock);
     for_each_domain_in_cpupool( d, c )
     {

@@ -1375,12 +1420,15 @@ static int sedf_adjust_weights(struct cp
                 continue;
             if ( EDOM_INFO(p)->weight )
             {
+                /* Interrupts already off */
+                vcpu_schedule_lock(p);
                 EDOM_INFO(p)->period_orig = 
                     EDOM_INFO(p)->period  = WEIGHT_PERIOD;
                 EDOM_INFO(p)->slice_orig  =
                     EDOM_INFO(p)->slice   = 
                     (EDOM_INFO(p)->weight *
                      (WEIGHT_PERIOD - WEIGHT_SAFETY - sumt[cpu])) / sumw[cpu];
+                vcpu_schedule_unlock(p);
             }
         }
     }

@@ -1396,6 +1444,8 @@ static int sedf_adjust_weights(struct cp
 /* set or fetch domain scheduling parameters */
 static int sedf_adjust(const struct scheduler *ops, struct domain *p, struct xen_domctl_scheduler_op *op)
 {
+    struct sedf_priv_info *prv = SEDF_PRIV(ops);
+    unsigned long flags;
     struct vcpu *v;
     int rc = 0;

@@ -1404,11 +1454,12 @@ static int sedf_adjust(const struct sche
           p->domain_id, op->u.sedf.period, op->u.sedf.slice,
           op->u.sedf.latency, (op->u.sedf.extratime)?"yes":"no");
 
-    /* Serialize everything against runq lock to prevent concurrent update
-     * (notice all non-current VCPUs of the domain have been paused in the
-     * caller). */
-    if ( p == current->domain )
-        vcpu_schedule_lock_irq(current);
+    /* Serialize against the pluggable scheduler lock to protect from
+     * concurrent updates. We need to take the runq lock for the VCPUs
+     * as well, since we are touching extraweight, weight, slice and
+     * period. As in sched_credit2.c, runq locks nest inside the
+     * pluggable scheduler lock. */
+    spin_lock_irqsave(&prv->lock, flags);
 
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_putinfo )
     {

@@ -1427,43 +1478,52 @@ static int sedf_adjust(const struct sche
                 /* Weight-driven domains with extratime only. */
                 for_each_vcpu ( p, v )
                 {
+                    /* (Here and everywhere in the following) IRQs are already off,
+                     * hence vcpu_spin_lock() is the one. */
+                    vcpu_schedule_lock(v);
                     EDOM_INFO(v)->extraweight = op->u.sedf.weight;
                     EDOM_INFO(v)->weight = 0;
                     EDOM_INFO(v)->slice = 0;
                     EDOM_INFO(v)->period = WEIGHT_PERIOD;
+                    vcpu_schedule_unlock(v);
                 }
             }
             else
             {
                 /* Weight-driven domains with real-time execution. */
-                for_each_vcpu ( p, v )
+                for_each_vcpu ( p, v ) {
+                    vcpu_schedule_lock(v);
                     EDOM_INFO(v)->weight = op->u.sedf.weight;
+                    vcpu_schedule_lock(v);
+                }
             }
         }
         else
         {
+            /*
+             * Sanity checking: note that disabling extra weight requires
+             * that we set a non-zero slice.
+             */
+            if ( (op->u.sedf.period > PERIOD_MAX) ||
+                 (op->u.sedf.period < PERIOD_MIN) ||
+                 (op->u.sedf.slice  > op->u.sedf.period) ||
+                 (op->u.sedf.slice  < SLICE_MIN) )
+            {
+                rc = -EINVAL;
+                goto out;
+            }
+
             /* Time-driven domains. */
             for_each_vcpu ( p, v )
             {
-                /*
-                 * Sanity checking: note that disabling extra weight requires
-                 * that we set a non-zero slice.
-                 */
-                if ( (op->u.sedf.period > PERIOD_MAX) ||
-                     (op->u.sedf.period < PERIOD_MIN) ||
-                     (op->u.sedf.slice  > op->u.sedf.period) ||
-                     (op->u.sedf.slice  < SLICE_MIN) )
-                {
-                    rc = -EINVAL;
-                    goto out;
-                }
-
+                vcpu_schedule_lock(v);
                 EDOM_INFO(v)->weight = 0;
                 EDOM_INFO(v)->extraweight = 0;
                 EDOM_INFO(v)->period_orig = 
                     EDOM_INFO(v)->period  = op->u.sedf.period;
                 EDOM_INFO(v)->slice_orig  = 
                     EDOM_INFO(v)->slice   = op->u.sedf.slice;
+                vcpu_schedule_unlock(v);
             }
         }

@@ -1473,11 +1533,13 @@ static int sedf_adjust(const struct sche
 
         for_each_vcpu ( p, v )
         {
+            vcpu_schedule_lock(v);
             EDOM_INFO(v)->status  = 
                 (EDOM_INFO(v)->status &
                  ~EXTRA_AWARE) | (op->u.sedf.extratime & EXTRA_AWARE);
             EDOM_INFO(v)->latency = op->u.sedf.latency;
             extraq_check(v);
+            vcpu_schedule_lock(v);
         }
     }
     else if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )

@@ -1496,17 +1558,19 @@ static int sedf_adjust(const struct sche
     }
 
 out:
-    if ( p == current->domain )
-        vcpu_schedule_unlock_irq(current);
+    spin_unlock_irqrestore(&prv->lock, flags);
 
     PRINT(2,"sedf_adjust_finished with return code %d\n", rc);
     return rc;
 }
 
+static struct sedf_priv_info _sedf_priv;
+
 const struct scheduler sched_sedf_def = {
-    .name     = "Simple EDF Scheduler",
-    .opt_name = "sedf",
-    .sched_id = XEN_SCHEDULER_SEDF,
+    .name           = "Simple EDF Scheduler",
+    .opt_name       = "sedf",
+    .sched_id       = XEN_SCHEDULER_SEDF,
+    .sched_data     = &_sedf_priv,
     
     .init_domain    = sedf_init_domain,
     .destroy_domain = sedf_destroy_domain,

@@ -1520,6 +1584,9 @@ const struct scheduler sched_sedf_def = 
     .alloc_domdata  = sedf_alloc_domdata,
     .free_domdata   = sedf_free_domdata,
 
+    .init           = sedf_init,
+    .deinit         = sedf_deinit,
+
     .do_schedule    = sedf_do_schedule,
     .pick_cpu       = sedf_pick_cpu,
     .dump_cpu_state = sedf_dump_cpu_state,

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Juergen Gross

2011-12-06 08:38 UTC #13 lore raw

In reply to Dario Faggioli

Quoted 5 lines · Dario Faggioli

Hi everyone,
This series changes how locks are dealt with while adjusting domains' scheduling parameters.

Quoted 7 lines

I've done and am still doing tests for credit and credit2, and it's surviving to all I threw at it up to now. Unfortunately, I can't test the sedf part yet, since it is not working on my test boxes due to other issues (which I'm also trying to track down). I'm sending the series out anyway, so that at least you can have a look at it, and maybe give a spin on the sedf part, if you don't have anything more interesting to do. ;-P

Sorry, does not work with sched=sedf:

nehalem1 login: (XEN) Xen BUG at spinlock.c:47
(XEN) ----[ Xen-4.2-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 0
(XEN) RIP: e008:[<ffff82c480124e84>] check_lock+0x44/0x50
(XEN) RFLAGS: 0000000000010046 CONTEXT: hypervisor

(XEN) rax: 0000000000000000   rbx: ffff83033eb17868   rcx: 0000000000000001
(XEN) rdx: 0000000000000000   rsi: 0000000000000001   rdi: ffff83033eb1786c
(XEN) rbp: ffff82c4802afc10   rsp: ffff82c4802afc10   r8:  0000000000000004
(XEN) r9:  000000000000003c   r10: ffff82c4802285b0   r11: 0000000000000212
(XEN) r12: ffff83033eb16000   r13: 0000000000000010   r14: ffff82c4802e3c60
(XEN) r15: ffff83033eb17868   cr0: 0000000080050033   cr4: 00000000000026f0
(XEN) cr3: 000000032b937000   cr2: 00007f09a92342c0
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen stack trace from rsp=ffff82c4802afc10:
(XEN)    ffff82c4802afc28 ffff82c480124ec1 0000000000000010 ffff82c4802afc68
(XEN)    ffff82c48012c085 7400000000000002 0000000000000010 0000000000000010
(XEN)    0000000000000020 ffff82c4802e3c60 ffff83033eaf8fb0 ffff82c4802afca8
(XEN)    ffff82c48012c557 ffff82c4802afd18 0000000000000010 0000000000000003
(XEN)    0000000000000004 ffff82c4802e3c60 ffff83033eaf8fb0 ffff82c4802afcc8
(XEN)    ffff82c48012c660 ffff82c480124ec1 0000000000000004 ffff82c4802afd58
(XEN)    ffff82c48011f8ab ffff82c480124ec1 ffff83033ea97048 ffff82c4802afd18
(XEN)    ffff82c480121084 ffff82c4802afe48 ffff83033ea92000 ffff83033eb11f60
(XEN)    0000000000000296 ffff82c4802afd38 ffff82c480121aab 00000004bf2fb000
(XEN)    ffff83033ea92000 000000000064b004 ffff83033ea92000 0000000000000000
(XEN)    00007fff37431340 ffff82c4802afd88 ffff82c480120fec ffff82c480125104
(XEN)    fffffffffffffff3 ffff82c4802afd88 fffffffffffffff3 ffff82c4802afef8
(XEN)    ffff82c4801037e3 000000000061f004 0000000000000000 000000000061f004
(XEN)    0000000000000001 ffff82c4802afef8 ffff82c480126cb8 ffff82c4802afdf8
(XEN)    ffff82c480106f0e ffff002000000000 00000000000c0000 00000000ffffffff
(XEN)    0000000000000000 0000000000000000 00000000000bf7b3 00000018d63a6d18
(XEN)    0000000300000004 0000000000000000 0000000000000000 0000000000000000
(XEN)    ffff82c4802afe88 0000000800000010 00007f09a9890000 0000000000000004
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000a00000001
(XEN)    0000000000000000 00007fff374314e0 00007fff37431540 000000000061e050
(XEN) Xen call trace:
(XEN)    [<ffff82c480124e84>] check_lock+0x44/0x50
(XEN)    [<ffff82c480124ec1>] _spin_lock+0x11/0x5d
(XEN)    [<ffff82c48012c085>] xmem_pool_alloc+0x138/0x4d2
(XEN)    [<ffff82c48012c557>] _xmalloc+0x138/0x230
(XEN)    [<ffff82c48012c660>] _xzalloc+0x11/0x2d
(XEN)    [<ffff82c48011f8ab>] sedf_adjust+0x37c/0x9b2
(XEN)    [<ffff82c480120fec>] sched_adjust+0x5f/0xb7
(XEN)    [<ffff82c4801037e3>] do_domctl+0xf32/0x1a9f
(XEN)    [<ffff82c48021f128>] syscall_enter+0xc8/0x122

(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Xen BUG at spinlock.c:47
(XEN) ****************************************
(XEN)

Signature · 5 lines

Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: [email protected]
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Dario Faggioli

author

2011-12-06 10:10 UTC #14 lore raw

Quoted 2 lines · Juergen Gross

Sorry, does not work with sched=sedf:

Hi Juergen,

Thanks for reporting. I'm reworking this as per Ian's suggestions, but I'll check this out. Can I as how you made this happening? Just trying to change a sedf-domain scheduling parameter I guess... Is that right?

Thanks again and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Juergen Gross

2011-12-06 11:03 UTC #15 lore raw

Hi Dario,

Quoted 8 lines · Dario Faggioli

Quoted 2 lines · Juergen Gross
Sorry, does not work with sched=sedf:
Hi Juergen,
Thanks for reporting. I'm reworking this as per Ian's suggestions, but I'll check this out. Can I as how you made this happening? Just trying to change a sedf-domain scheduling parameter I guess... Is that right?

I booted with sched=sedf and tried to change Domain-0 parameters with:

xl sched-sedf -d Domain-0 -w 10

Juergen

Signature · 5 lines

Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: [email protected]
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

George Dunlap

2011-12-06 12:30 UTC #16 lore raw

In reply to Juergen Gross

Quoted 1 line · Juergen Gross

(XEN) Xen BUG at spinlock.c:47

[snip]

Quoted 10 lines

(XEN) Xen call trace:
(XEN) [<ffff82c480124e84>] check_lock+0x44/0x50 (XEN) [<ffff82c480124ec1>] _spin_lock+0x11/0x5d (XEN) [<ffff82c48012c085>] xmem_pool_alloc+0x138/0x4d2 (XEN) [<ffff82c48012c557>] _xmalloc+0x138/0x230 (XEN) [<ffff82c48012c660>] _xzalloc+0x11/0x2d (XEN) [<ffff82c48011f8ab>] sedf_adjust+0x37c/0x9b2 (XEN) [<ffff82c480120fec>] sched_adjust+0x5f/0xb7 (XEN) [<ffff82c4801037e3>] do_domctl+0xf32/0x1a9f (XEN) [<ffff82c48021f128>] syscall_enter+0xc8/0x122

Hmm, looks like the problem is that we assert that locks must be called with IRQs enabled all the time, or never. From xen/common/spinlock.c:

* We partition locks into IRQ-safe (always held with IRQs disabled) and
* IRQ-unsafe (always held with IRQs enabled) types. The convention for
* every lock must be consistently observed else we can deadlock in
* IRQ-context rendezvous functions (a rendezvous which gets every CPU
* into IRQ context before any CPU is released from the rendezvous).

sedf_adj() grabs the private lock with irqs disabled, then calls sedf_adjust_weights(), which calls xmalloc to allocate some local scratch space, which ultimately grabs a non-IRQ spinlock.

Not sure the best thing to do here...

-George

Juergen Gross

2011-12-06 12:39 UTC #17 lore raw

Quoted 24 lines · George Dunlap

Quoted 1 line · Juergen Gross
(XEN) Xen BUG at spinlock.c:47
[snip]
Quoted 10 lines
(XEN) Xen call trace:
(XEN)    [<ffff82c480124e84>] check_lock+0x44/0x50
(XEN)    [<ffff82c480124ec1>] _spin_lock+0x11/0x5d
(XEN)    [<ffff82c48012c085>] xmem_pool_alloc+0x138/0x4d2
(XEN)    [<ffff82c48012c557>] _xmalloc+0x138/0x230
(XEN)    [<ffff82c48012c660>] _xzalloc+0x11/0x2d
(XEN)    [<ffff82c48011f8ab>] sedf_adjust+0x37c/0x9b2
(XEN)    [<ffff82c480120fec>] sched_adjust+0x5f/0xb7
(XEN)    [<ffff82c4801037e3>] do_domctl+0xf32/0x1a9f
(XEN)    [<ffff82c48021f128>] syscall_enter+0xc8/0x122
Hmm, looks like the problem is that we assert that locks must be called with IRQs enabled all the time, or never. From xen/common/spinlock.c:
* We partition locks into IRQ-safe (always held with IRQs disabled) and
* IRQ-unsafe (always held with IRQs enabled) types. The convention for
* every lock must be consistently observed else we can deadlock in
* IRQ-context rendezvous functions (a rendezvous which gets every CPU
* into IRQ context before any CPU is released from the rendezvous).

Quoted 5 lines

sedf_adj() grabs the private lock with irqs disabled, then calls sedf_adjust_weights(), which calls xmalloc to allocate some local scratch space, which ultimately grabs a non-IRQ spinlock.
Not sure the best thing to do here...

sedf_adjust_weights() is called only from sedf_adj(). The easiest solution would be to xmalloc the scratch space in sedf_adj() before grabbing the lock and passing the xmalloced area to sedf_adjust_weights().

Juergen

Signature · 5 lines

Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: [email protected]
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Dario Faggioli

author

2011-12-06 16:39 UTC #18 lore raw

Quoted 4 lines · Juergen Gross

sedf_adjust_weights() is called only from sedf_adj(). The easiest solution would be to xmalloc the scratch space in sedf_adj() before grabbing the lock and passing the xmalloced area to sedf_adjust_weights().

Done, and it seems to be working... It might be a bit weird to read, but I couldn't
find any other equally easy and effective solution so.

Thanks a lot and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

George Dunlap

2011-12-06 12:24 UTC #19 lore raw

In reply to Dario Faggioli

Quoted 13 lines · Dario Faggioli

Hi everyone,
This series changes how locks are dealt with while adjusting domains' scheduling parameters.
I've done and am still doing tests for credit and credit2, and it's surviving to all I threw at it up to now. Unfortunately, I can't test the sedf part yet, since it is not working on my test boxes due to other issues (which I'm also trying to track down). I'm sending the series out anyway, so that at least you can have a look at it, and maybe give a spin on the sedf part, if you don't have anything more interesting to do. ;-P

Quoted 2 lines

Juergen series on xl scheduling support attached to this mail, in the form of a single patch, for testing convenience.

Hey Dario, sorry for the long delay in responding.

Overall the patch series looks good to me. The only issue I see is that it looks like you introduce a regression between patch 2 and 3 -- that is, if you apply patches 1 and 2, but not 3, then the lock you grab in sched_sedf.c:sedf_adjust() won't protect against concurrent accesses from other vcpus; nor will it correctly handle updates to the per-vcpu EDOM_INFO() variables.

Regressions in mid-patch series are bad because it can mess up bisection.

I think a better way of breaking it down would be:
* Add scheduler global locks, and have them called in the pluggable scheduler *_adjust() function. This is redundant, but shouldn't be harmful.
* Atomically remove both the pause and the lock in schedule.c, and add the scheduling locks around the per-vcpu EDOM_INFO variables in sched_sedf.c, all in one patch. This makes the patch bigger, but it avoids a regression. The two things are related anwyay: the reason you now need scheduling locks around EDOM_INFO variables is because you're getting rid of the pausing and the lock in schedule.c.

Thoughts?

-George

Quoted 23 lines

--
xen/common/sched_credit.c | 10 +++++++---
xen/common/sched_credit2.c | 21 +++++++++++----------
xen/common/sched_sedf.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------- xen/common/schedule.c | 34 ++--------------------------------
4 files changed, 130 insertions(+), 66 deletions(-)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Dario Faggioli

author

2011-12-06 16:46 UTC #20 lore raw

Quoted 2 lines · George Dunlap

Hey Dario, sorry for the long delay in responding.

Hi, and no problem at all :-)

Quoted 2 lines

Regressions in mid-patch series are bad because it can mess up bisection.

I strongly agree. I didn't think much about that for this series since the mechanism
I was trying to fix was (especially for sedf) already broken in various ways but,
indeed, patches could be better split.

Quoted 9 lines

I think a better way of breaking it down would be:
* Add scheduler global locks, and have them called in the pluggable scheduler *_adjust() function. This is redundant, but shouldn't be harmful.
* Atomically remove both the pause and the lock in schedule.c, and add the scheduling locks around the per-vcpu EDOM_INFO variables in sched_sedf.c, all in one patch. This makes the patch bigger, but it avoids a regression.

Ok, I'll go for this.

Quoted 4 lines

The two things are related anwyay: the reason
you now need scheduling locks around EDOM_INFO variables is because you're getting rid of the pausing and the lock in schedule.c.

Yeah, what I was trying to do was leaving a clear footstep about why pausing was going and, yes, also making patches smaller, but again, I see your point and will follow your advice. :-)

Thanks and Regards,
Dario

Signature · 5 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)