From patchwork Fri Sep 30 12:37:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Dario Faggioli X-Patchwork-Id: 9358189 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id EC5546075E for ; Fri, 30 Sep 2016 12:39:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DB8AC2A003 for ; Fri, 30 Sep 2016 12:39:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D06432A007; Fri, 30 Sep 2016 12:39:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id BAB072A003 for ; Fri, 30 Sep 2016 12:39:54 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bpx42-0001sF-Ac; Fri, 30 Sep 2016 12:37:18 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bpx41-0001s9-Ai for xen-devel@lists.xenproject.org; Fri, 30 Sep 2016 12:37:17 +0000 Received: from [85.158.139.211] by server-7.bemta-5.messagelabs.com id 76/E1-05127-C7C5EE75; Fri, 30 Sep 2016 12:37:16 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrFKsWRWlGSWpSXmKPExsXitHSDvW5VzLt wg7XPrSy+b5nM5MDocfjDFZYAxijWzLyk/IoE1oz+ec8ZCx6tZqz42tLE3MDYv4Sxi5GTQ0Ig RGLph23MIDavgKHE5nenweLCApESPavmMYHYbAIGEm927GXtYuTgEBEIk2icpwkSZhYolPjeu gislUVAVWLu8WdsIDangJ3ExutrweJCAj8ZJW4dtQex+QUkJW59+cgM0VstcfHGPTaIE7Qlev b+gDpBUOLkzCcsEL1qEjPmXmaFqOGWuH16KvMERv5ZSNpnIWmBiDtIvJn1B8rWlGjd/psdwta WWLbwNTOEHSHR1bASyk6UWN+xHKpeUWJK90Oo+miJ5rk3oOIpEtcnzWfEVGMrsW7de6gaG4lN VxdA1chLbH87h3kBI98qRo3i1KKy1CJdQ1O9pKLM9IyS3MTMHF1DA1O93NTi4sT01JzEpGK95 PzcTYzAeGQAgh2MDds9DzFKcjApifLKHn0bLsSXlJ9SmZFYnBFfVJqTWnyIUYODQ2DC2bnTma RY8vLzUpUkeJWj34ULCRalpqdWpGXmABMGTKkEB4+SCK8cSJq3uCAxtzgzHSJ1ilFRSpyXFyQ hAJLIKM2Da4MlqUuMslLCvIxARwnxFKQW5WaWoMq/YhTnYFQS5o0CmcKTmVcCN/0V0GImoMX5 R9+ALC5JREhJNTDOEDt55NIiPTbpFS0G8QVfq595Sb//8uPxqZXxN3vvnbjQJHaEOVzGV3PWk eUpZ/XOCARMXdy8IDRW6P4tP433P79ov0xNSxdbs/Bbp5TKvsua9z6WKJ68zGN0hKNO9e+95v eC5Z01PM5BeX/ifzoX8om88vK6k3xPtfHOwQln7opGC5ht9dqgxFKckWioxVxUnAgAuSYg9k0 DAAA= X-Env-Sender: prvs=074d4ead7=dario.faggioli@citrix.com X-Msg-Ref: server-3.tower-206.messagelabs.com!1475239033!58907516!1 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 8.84; banners=-,-,- X-VirusChecked: Checked Received: (qmail 23376 invoked from network); 30 Sep 2016 12:37:14 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-3.tower-206.messagelabs.com with RC4-SHA encrypted SMTP; 30 Sep 2016 12:37:14 -0000 X-IronPort-AV: E=Sophos;i="5.31,272,1473120000"; d="asc'?scan'208";a="389931064" Message-ID: <1475239024.5315.87.camel@citrix.com> From: Dario Faggioli To: George Dunlap , Ian Jackson Date: Fri, 30 Sep 2016 14:37:04 +0200 In-Reply-To: References: <147520253247.22544.10673844222866363947.stgit@Solace.fritz.box> <147520406186.22544.1059342691539063899.stgit@Solace.fritz.box> <22510.16049.351726.440830@mariner.uk.xensource.com> Organization: Citrix Inc. X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) MIME-Version: 1.0 X-DLP: MIA2 Cc: George Dunlap , xen-devel@lists.xenproject.org, Wei Liu Subject: Re: [Xen-devel] [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2 X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 From: Dario Faggioli 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 --- Cc: Ian Jackson Cc: Wei Liu Cc: George Dunlap --- 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_, 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),