| Message ID | 147520406186.22544.1059342691539063899.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
> +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.
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.
> +
> + 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 ( ).
> + 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.
Thanks,
Ian.
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. > >> +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. > > 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. > >> + >> + 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 ( ). > >> + 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? Or would you rather have Dario re-send it to make sure the changes get implemented correctly? -George
George Dunlap writes ("Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2"):
> On 30/09/16 11:30, Ian Jackson wrote:
> >> + 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? Or would you rather have Dario re-send it to
> make sure the changes get implemented correctly?
Please feel free to fix it up during checking, with my ack.
Ian.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 606d71a..5a70564 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5229,6 +5229,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) { @@ -5266,11 +5279,8 @@ 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; + rc = sched_ratelimit_check(gc, scinfo->ratelimit_us); + if (rc) { goto out; } if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) { @@ -5297,6 +5307,57 @@ 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; + + 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 98bfc3a..de5996e 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -834,6 +834,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),
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 v1: * added the appropriate LIBXL_HAVE_<foo>, as requested during review. * coding style fixes put in previous patch, as requested during review. --- tools/libxl/libxl.c | 71 ++++++++++++++++++++++++++++++++++++++++--- tools/libxl/libxl.h | 11 +++++++ tools/libxl/libxl_types.idl | 4 ++ 3 files changed, 81 insertions(+), 5 deletions(-)