Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2

[Dario Faggioli <dario.faggioli@citrix.com>]

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

On Fri, 2016-09-30 at 11:33 +0100, George Dunlap wrote:
> On 30/09/16 11:30, Ian Jackson wrote:
> > 
> > Dario Faggioli writes ("[PATCH v2 09/10] libxl: allow to set the
> > ratelimit value online for Credit2"):
> > > 
> > > This is the remaining part of the plumbing (the libxl
> > > one) necessary to be able to change the value of the
> > > ratelimit_us parameter online, for Credit2 (like it is
> > > already for Credit1).
> > 
> > Thanks.  I have some coding style nits, all but one of which are
> > utterly trivial.  If it hadn't been for the `rc=0' one I would have
> > acked this patch as-is.
> > 
Ok, thanks for looking this quickly. :-)

> > > +int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t
> > > poolid,
> > > +                                   libxl_sched_credit2_params
> > > *scinfo)
> > > +{
> > > +    struct xen_sysctl_credit2_schedule sparam;
> > > +    int r, rc;
> > > +    GC_INIT(ctx);
> > > +
> > > +    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
> > > +    if (rc) {
> > > +        goto out;
> > > +    }
> > 
> > When you respin this, I think IWBNI you could change this to
> >       if (rc)
> >           goto out;
> > or
> >       if (rc) goto out;
> > both of which are encouraged by the coding style, and are more
> > common
> > inside libxl.
> > 
Right!

> > An earlier hunk in this patch does not adjust from a similar
> > construct
> > where the { } are becoming newly unnecessary.  You may want to
> > adjust
> > that too, but I won't object if you don't feel like it.
> > 
Sure.

> > > +    sparam.ratelimit_us = scinfo->ratelimit_us;
> > > +
> > > +    r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
> > > +    if ( r < 0 ) {
> >            ^     ^
> > Coding style: libxl does not use the spaces just inside the ( ).
> > 
Bah, how did this got here! :-/

> > > 
> > > +        LOGE(ERROR, "Setting Credit2 scheduler parameters");
> > > +        rc = ERROR_FAIL;
> > > +        goto out;
> > > +    }
> > > +
> > > +    scinfo->ratelimit_us = sparam.ratelimit_us;
> > > +
> > > + out:
> > > +    GC_FREE;
> > > +    return rc;
> > 
> > This should have an explicit assignment rc=0 before the out
> > label.  As
> > it is, it will happen to be 0 anyway, so that's a stylistic fix.
> 
> If I'm happy with it as-is, and I fix these up before I check them
> in,
> can I put your Ack on it?  
>
So, just FTR, it doesn't just "happen to be 0 anyway", that was on
purpose. :-)

I.e., I think the function is small and simple enough for me to take
advantage of the knowledge that, when we get to the out: label, we've
necessarily called sched_ratelimit_check(), and saved the return code
in 'rc', and that such return code is either 0, or the proper error
value we want to return.

That being said, it's no big deal, and I'm fine whatever the code ends
up looking.

> Or would you rather have Dario re-send it to
> make sure the changes get implemented correctly?
> 
Again, I'm fine either way. For your convenience, I'm attaching here
two variants of this patch. Both have the coding style fixes suggested.
One (the one with '-rc' in the name), includes the 'rc = 0', the other
doesn't.

Feel free to pickup whichever you find best/more convenient/etc, or
feel free to ignore them, if they're not useful. I'm fine in any case.
:-)

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.1.2: libxl-allow-disabling-ratelimit-online.patch --]
[-- Type: text/x-patch, Size: 5841 bytes --]

From: Dario Faggioli <dario.faggioli@citrix.com>

libxl: allow to set the ratelimit value online for Credit2

This is the remaining part of the plumbing (the libxl
one) necessary to be able to change the value of the
ratelimit_us parameter online, for Credit2 (like it is
already for Credit1).

Note that, so far, we were rejecting (for Credit1) a
new value of zero, despite it is a pretty nice way to
ask for the rate limiting to be disabled, and the
hypervisor is already capable of dealing with it in
that way.

Therefore, we change things so that it is possible to
do so, both for Credit1 and Credit2

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v2:
 * coding style, reported during review.

Changes from v1:
 * added the appropriate LIBXL_HAVE_<foo>, as requested during review.
 * coding style fixes put in previous patch, as requested during review.

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d2552f9..57c052c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5256,6 +5256,19 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+static int sched_ratelimit_check(libxl__gc *gc, int ratelimit)
+{
+    if (ratelimit != 0 &&
+        (ratelimit <  XEN_SYSCTL_SCHED_RATELIMIT_MIN ||
+         ratelimit > XEN_SYSCTL_SCHED_RATELIMIT_MAX)) {
+        LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
+            XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
 int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
                                   libxl_sched_credit_params *scinfo)
 {
@@ -5293,13 +5306,9 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
         rc = ERROR_INVAL;
         goto out;
     }
-    if (scinfo->ratelimit_us <  XEN_SYSCTL_SCHED_RATELIMIT_MIN
-        || scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
-        LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
-            XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
-        rc = ERROR_INVAL;
-        goto out;
-    }
+    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+    if (rc) goto out;
+
     if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
         LOG(ERROR, "Ratelimit cannot be greater than timeslice");
         rc = ERROR_INVAL;
@@ -5319,12 +5328,60 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
     scinfo->tslice_ms = sparam.tslice_ms;
     scinfo->ratelimit_us = sparam.ratelimit_us;
 
+ out:
+    GC_FREE;
+    return rc;
+}
+
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo)
+{
+    struct xen_sysctl_credit2_schedule sparam;
+    int r, rc;
+    GC_INIT(ctx);
+
+    r = xc_sched_credit2_params_get(ctx->xch, poolid, &sparam);
+    if (r < 0) {
+        LOGE(ERROR, "getting Credit2 scheduler parameters");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    scinfo->ratelimit_us = sparam.ratelimit_us;
+
     rc = 0;
  out:
     GC_FREE;
     return rc;
 }
 
+
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo)
+{
+    struct xen_sysctl_credit2_schedule sparam;
+    int r, rc;
+    GC_INIT(ctx);
+
+    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+    if (rc) goto out;
+
+    sparam.ratelimit_us = scinfo->ratelimit_us;
+
+    r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
+    if (r < 0) {
+        LOGE(ERROR, "Setting Credit2 scheduler parameters");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    scinfo->ratelimit_us = sparam.ratelimit_us;
+
+ out:
+    GC_FREE;
+    return rc;
+}
+
 static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
                                     libxl_domain_sched_params *scinfo)
 {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cfa540..969a089 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -281,6 +281,13 @@
 #define LIBXL_HAVE_QEMU_MONITOR_COMMAND 1
 
 /*
+ * LIBXL_HAVE_SCHED_CREDIT2_PARAMS indicates the existance of a
+ * libxl_sched_credit2_params structure, containing Credit2 scheduler
+ * wide parameters (i.e., the ratelimiting value).
+ */
+#define LIBXL_HAVE_SCHED_CREDIT2_PARAMS 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
@@ -1992,6 +1999,10 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
                                   libxl_sched_credit_params *scinfo);
 int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
                                   libxl_sched_credit_params *scinfo);
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo);
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo);
 
 /* Scheduler Per-domain parameters */
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a02446f..a32c751 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -838,6 +838,10 @@ libxl_sched_credit_params = Struct("sched_credit_params", [
     ("ratelimit_us", integer),
     ], dispose_fn=None)
 
+libxl_sched_credit2_params = Struct("sched_credit2_params", [
+    ("ratelimit_us", integer),
+    ], dispose_fn=None)
+
 libxl_domain_remus_info = Struct("domain_remus_info",[
     ("interval",     integer),
     ("allow_unsafe", libxl_defbool),

[-- Attachment #1.1.3: libxl-allow-disabling-ratelimit-online_rc.patch --]
[-- Type: text/x-patch, Size: 5811 bytes --]

From: Dario Faggioli <dario.faggioli@citrix.com>

libxl: allow to set the ratelimit value online for Credit2

This is the remaining part of the plumbing (the libxl
one) necessary to be able to change the value of the
ratelimit_us parameter online, for Credit2 (like it is
already for Credit1).

Note that, so far, we were rejecting (for Credit1) a
new value of zero, despite it is a pretty nice way to
ask for the rate limiting to be disabled, and the
hypervisor is already capable of dealing with it in
that way.

Therefore, we change things so that it is possible to
do so, both for Credit1 and Credit2

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v2:
 * coding style, reported during review;
 * explicitly set rc to 0 on succesful path, as suggested during review.

Changes from v1:
 * added the appropriate LIBXL_HAVE_<foo>, as requested during review.
 * coding style fixes put in previous patch, as requested during review.

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d2552f9..a3b5e6d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5256,6 +5256,19 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+static int sched_ratelimit_check(libxl__gc *gc, int ratelimit)
+{
+    if (ratelimit != 0 &&
+        (ratelimit <  XEN_SYSCTL_SCHED_RATELIMIT_MIN ||
+         ratelimit > XEN_SYSCTL_SCHED_RATELIMIT_MAX)) {
+        LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
+            XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
 int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
                                   libxl_sched_credit_params *scinfo)
 {
@@ -5293,13 +5306,9 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
         rc = ERROR_INVAL;
         goto out;
     }
-    if (scinfo->ratelimit_us <  XEN_SYSCTL_SCHED_RATELIMIT_MIN
-        || scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
-        LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
-            XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
-        rc = ERROR_INVAL;
-        goto out;
-    }
+    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+    if (rc) goto out;
+
     if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
         LOG(ERROR, "Ratelimit cannot be greater than timeslice");
         rc = ERROR_INVAL;
@@ -5325,6 +5334,56 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
     return rc;
 }
 
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo)
+{
+    struct xen_sysctl_credit2_schedule sparam;
+    int r, rc;
+    GC_INIT(ctx);
+
+    r = xc_sched_credit2_params_get(ctx->xch, poolid, &sparam);
+    if (r < 0) {
+        LOGE(ERROR, "getting Credit2 scheduler parameters");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    scinfo->ratelimit_us = sparam.ratelimit_us;
+
+    rc = 0;
+ out:
+    GC_FREE;
+    return rc;
+}
+
+
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo)
+{
+    struct xen_sysctl_credit2_schedule sparam;
+    int r, rc;
+    GC_INIT(ctx);
+
+    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+    if (rc) goto out;
+
+    sparam.ratelimit_us = scinfo->ratelimit_us;
+
+    r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
+    if (r < 0) {
+        LOGE(ERROR, "Setting Credit2 scheduler parameters");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    scinfo->ratelimit_us = sparam.ratelimit_us;
+
+    rc = 0;
+ out:
+    GC_FREE;
+    return rc;
+}
+
 static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
                                     libxl_domain_sched_params *scinfo)
 {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cfa540..969a089 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -281,6 +281,13 @@
 #define LIBXL_HAVE_QEMU_MONITOR_COMMAND 1
 
 /*
+ * LIBXL_HAVE_SCHED_CREDIT2_PARAMS indicates the existance of a
+ * libxl_sched_credit2_params structure, containing Credit2 scheduler
+ * wide parameters (i.e., the ratelimiting value).
+ */
+#define LIBXL_HAVE_SCHED_CREDIT2_PARAMS 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
@@ -1992,6 +1999,10 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
                                   libxl_sched_credit_params *scinfo);
 int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
                                   libxl_sched_credit_params *scinfo);
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo);
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+                                   libxl_sched_credit2_params *scinfo);
 
 /* Scheduler Per-domain parameters */
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a02446f..a32c751 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -838,6 +838,10 @@ libxl_sched_credit_params = Struct("sched_credit_params", [
     ("ratelimit_us", integer),
     ], dispose_fn=None)
 
+libxl_sched_credit2_params = Struct("sched_credit2_params", [
+    ("ratelimit_us", integer),
+    ], dispose_fn=None)
+
 libxl_domain_remus_info = Struct("domain_remus_info",[
     ("interval",     integer),
     ("allow_unsafe", libxl_defbool),

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

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

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