Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[Dario Faggioli]

On Mon, 2016-08-22 at 10:28 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 14/24] libxl: allow to set the
> ratelimit value online for Credit2"):
> ...
> > 
> > -rc = xc_sched_credit_params_set(ctx->xch, poolid, );
> > -if ( rc < 0 ) {
> > -LOGE(ERROR, "setting sched credit param");
> > -GC_FREE;
> > -return ERROR_FAIL;
> > +r = xc_sched_credit_params_set(ctx->xch, poolid, );
> > +if ( r < 0 ) {
> > +LOGE(ERROR, "Setting Credit scheduler parameters");
> > +rc = ERROR_FAIL;
> > +goto out;
> 
> I had to read this three times to figure out what the change was.
> 
> It is good that you are fixing the coding style but can you please
> put
> it in a separate patch ?
> 
Done in v2.

> But I wonder whether there will still be lots of rather formulaic
> code
> that could profitably be generalised somehow.  I'd appreciate your
> views on whether that would be possible, and whether it would be a
> good idea..
> 
I've checked, as promised. TBH, as George said already, I don't see
much more room for factoring or generalizing. Certainly, not in libxl.

In xl (that would be next patch, though), I especially dislike
those main_sched_credit(), main_sched_credit2(), etc., but I think
that, given how the interface looks like, there is again few that we
can do (there's some level of generalization and indirection already,
actually).

So, again, apart from splitting coding style and functional changes, I
don't see other ways for improving this patch. If you have more
concrete ides, please share them. :-)

Thanks and Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)

signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[George Dunlap]

On 17/08/16 18:19, Dario Faggioli wrote:
> 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
> 
> While there, fix the error handling path (make it
> compliant with libxl's codying style) in Credit1
> rate limiting related functions.
> 
> Signed-off-by: Dario Faggioli 

With the LIBXL_HAVE #define:

Reviewed-by: George Dunlap 



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

Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[George Dunlap]

On 22/08/16 10:28, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 14/24] libxl: allow to set the ratelimit value 
> online for Credit2"):
> ...
>> -rc = xc_sched_credit_params_set(ctx->xch, poolid, );
>> -if ( rc < 0 ) {
>> -LOGE(ERROR, "setting sched credit param");
>> -GC_FREE;
>> -return ERROR_FAIL;
>> +r = xc_sched_credit_params_set(ctx->xch, poolid, );
>> +if ( r < 0 ) {
>> +LOGE(ERROR, "Setting Credit scheduler parameters");
>> +rc = ERROR_FAIL;
>> +goto out;
> 
> I had to read this three times to figure out what the change was.
> 
> It is good that you are fixing the coding style but can you please put
> it in a separate patch ?
> 
> In general I was surprised at how large this patch, and the
> corresponding xl one, was.  Perhaps after the coding style fixes are
> split off the functional patches will be smaller.
> 
> But I wonder whether there will still be lots of rather formulaic code
> that could profitably be generalised somehow.  I'd appreciate your
> views on whether that would be possible, and whether it would be a
> good idea..

FWIW, I find that the massive use of #defines to define entire functions
or large code blocks (particularly blocks which define variables or hide
code flowss) make it very difficult to tell what's going on.  When there
is risk of code getting out of sync, then it may well be worth the cost;
but in a case like this, when there is no inter-dependency, and little
shared functionality, it doesn't make sense to unify things more than
Dario already has (e.g., by making a function to check the parameter value).

 -George


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

Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[Dario Faggioli]

On Mon, 2016-08-22 at 10:21 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 14/24] 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).
>
> I think this should have a HAVE #define.
> 
Ah, sure. Sorry I've forgotten, I'll add it in v2.

Thanks and Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[Ian Jackson]

Dario Faggioli writes ("[PATCH 14/24] libxl: allow to set the ratelimit value 
online for Credit2"):
...
> -rc = xc_sched_credit_params_set(ctx->xch, poolid, );
> -if ( rc < 0 ) {
> -LOGE(ERROR, "setting sched credit param");
> -GC_FREE;
> -return ERROR_FAIL;
> +r = xc_sched_credit_params_set(ctx->xch, poolid, );
> +if ( r < 0 ) {
> +LOGE(ERROR, "Setting Credit scheduler parameters");
> +rc = ERROR_FAIL;
> +goto out;

I had to read this three times to figure out what the change was.

It is good that you are fixing the coding style but can you please put
it in a separate patch ?

In general I was surprised at how large this patch, and the
corresponding xl one, was.  Perhaps after the coding style fixes are
split off the functional patches will be smaller.

But I wonder whether there will still be lots of rather formulaic code
that could profitably be generalised somehow.  I'd appreciate your
views on whether that would be possible, and whether it would be a
good idea..

For now I will stop my review here.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[Ian Jackson]

Dario Faggioli writes ("[PATCH 14/24] 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).

I think this should have a HAVE #define.

Ian.

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

[Xen-devel] [PATCH 14/24] libxl: allow to set the ratelimit value online for Credit2

[Dario Faggioli]

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

While there, fix the error handling path (make it
compliant with libxl's codying style) in Credit1
rate limiting related functions.

Signed-off-by: Dario Faggioli 
---
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: George Dunlap 
---
 tools/libxl/libxl.c |  111 ++-
 tools/libxl/libxl.h |4 ++
 tools/libxl/libxl_types.idl |4 ++
 3 files changed, 95 insertions(+), 24 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6a50e49..d6a8d02 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5229,69 +5229,132 @@ 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)
 {
 struct xen_sysctl_credit_schedule sparam;
-int rc;
+int r, rc;
 GC_INIT(ctx);
 
-rc = xc_sched_credit_params_get(ctx->xch, poolid, );
-if (rc != 0) {
-LOGE(ERROR, "getting sched credit param");
-GC_FREE;
-return ERROR_FAIL;
+r = xc_sched_credit_params_get(ctx->xch, poolid, );
+if (r < 0) {
+LOGE(ERROR, "getting Credit scheduler parameters");
+rc = ERROR_FAIL;
+goto out;
 }
 
 scinfo->tslice_ms = sparam.tslice_ms;
 scinfo->ratelimit_us = sparam.ratelimit_us;
 
+rc = 0;
+ out:
 GC_FREE;
-return 0;
+return rc;
 }
 
 int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
   libxl_sched_credit_params *scinfo)
 {
 struct xen_sysctl_credit_schedule sparam;
-int rc=0;
+int r, rc;
 GC_INIT(ctx);
 
 if (scinfo->tslice_ms <  XEN_SYSCTL_CSCHED_TSLICE_MIN
 || scinfo->tslice_ms > XEN_SYSCTL_CSCHED_TSLICE_MAX) {
 LOG(ERROR, "Time slice out of range, valid range is from %d to %d",
 XEN_SYSCTL_CSCHED_TSLICE_MIN, XEN_SYSCTL_CSCHED_TSLICE_MAX);
-GC_FREE;
-return ERROR_INVAL;
+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);
-GC_FREE;
-return ERROR_INVAL;
+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");
-GC_FREE;
-return ERROR_INVAL;
+rc = ERROR_INVAL;
+goto out;
 }
 
 sparam.tslice_ms = scinfo->tslice_ms;
 sparam.ratelimit_us = scinfo->ratelimit_us;
 
-rc = xc_sched_credit_params_set(ctx->xch, poolid, );
-if ( rc < 0 ) {
-LOGE(ERROR, "setting sched credit param");
-GC_FREE;
-return ERROR_FAIL;
+r = xc_sched_credit_params_set(ctx->xch, poolid, );
+if ( r < 0 ) {
+LOGE(ERROR, "Setting Credit scheduler parameters");
+rc = ERROR_FAIL;
+goto out;
 }
 
 scinfo->tslice_ms = sparam.tslice_ms;
 scinfo->ratelimit_us = sparam.ratelimit_us;
 
+ out:
 GC_FREE;
-return 0;
+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, );
+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,
+