From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chong Li Subject: Re: [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler Date: Fri, 7 Aug 2015 12:34:41 -0500 Message-ID: References: <1436590356-3706-1-git-send-email-chong.li@wustl.edu> <1436590356-3706-4-git-send-email-chong.li@wustl.edu> <1438074910.2889.26.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1438074910.2889.26.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: Chong Li , Wei Liu , Sisu Xi , George Dunlap , Ian Jackson , xen-devel , Ian Campbell , Meng Xu , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org On Tue, Jul 28, 2015 at 4:15 AM, Dario Faggioli wrote: > On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote: > >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index e9a2d26..9f7f859 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> >> +static int sched_rtds_validate_params(libxl__gc *gc, int period, >> + int budget, uint32_t *sdom_period, >> + uint32_t *sdom_budget) >> +{ >> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { >> + if (period < 1) { >> + LOG(ERROR, "VCPU period is not set or out of range, " >> + "valid values are larger than or equal to 1"); >> > That's probably a nit, but, if period is not set, as the error message > says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In > which case you do not enter this branch, and you do not print this > message, do you? > > What I mean is that, it looks to me that it's not accurate for the > message to say "period not set", or am I overlooking something? If the period is empty (which means its value == 0), we consider it as "period not set". Actually, I think maybe we can just delete this "not set" warning, because in xl, we've already done a sanity check to make sure period is not empty. > >> + return 1; >> > So, 1 is error, 0 is ok. That's fine, as this is an internal function, > but please, add a quick doc comment above it. Sure. > > >> + return 1; >> + } > >> + } >> + } else { >> + num_vcpus = max_vcpuid + 1; >> + GCNEW_ARRAY(vcpus, num_vcpus); >> + rc = sched_rtds_validate_params(gc, >> + scinfo->vcpus[0].period, scinfo->vcpus[0].budget, >> + &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget); >> + if (rc) >> + return ERROR_INVAL; >> + for (i = 0; i < num_vcpus; i++) { >> + vcpus[i].vcpuid = i; >> + vcpus[i].s.rtds.period = scinfo->vcpus[0].period; >> + vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget; >> + } >> + } >> + >> + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid, >> + vcpus, num_vcpus); >> + if (rc == 1) { >> + printf("WARN: too small period or budget may " >> + "result in large scheduling overhead\n"); >> + rc = 0; >> > As said, do the logging in Xen directly, and ditch this. Do I use "printk" function in Xen to output warning message? > >> + } else if (rc != 0) { >> + LOGE(ERROR, "setting vcpu sched rtds"); >> + return ERROR_FAIL; >> + } >> + >> + return rc; >> +} >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h >> index a1c5d15..040a248 100644 >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -200,6 +200,16 @@ >> #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1 >> >> /* >> + * libxl_vcpu_sched_params is used to store per-vcpu params >> +*/ >> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1 >> + >> +/* >> + * libxl_sched_params is used to store the array of per-vcpu params >> +*/ >> +#define LIBXL_HAVE_SCHED_PARAMS 1 >> + >> > I may be misremembering, but did not we say that one of these > LIBXL_HAVE_* was enough? > > If we want to have two, I'd much rather have a generic one > (LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is > supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS). > > This way, if/when we'll add support for per-VCPU parameters in Credit, > we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS. > > But that's just an idea, it's best to see what tools maintainers think > of this. > > BTW, be a little more verbose in commenting these macros. Just have a > look and take inspiration from the already existing ones. Ok. I'll rewrite it. Thanks, Chong > > Regards, > Dario > -- > <> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) -- Chong Li Department of Computer Science and Engineering Washington University in St.louis