From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler Date: Fri, 5 Feb 2016 16:19:30 +0000 Message-ID: <20160205161930.GL23178@citrix.com> References: <1454626244-5511-1-git-send-email-lichong659@gmail.com> <1454626244-5511-4-git-send-email-lichong659@gmail.com> <20160205144439.GA23178@citrix.com> <1454687983.9227.446.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1454687983.9227.446.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@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, ian.campbell@eu.citrix.com, Meng Xu , Chong Li , dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org On Fri, Feb 05, 2016 at 04:59:43PM +0100, Dario Faggioli wrote: > On Fri, 2016-02-05 at 14:44 +0000, Wei Liu wrote: > > On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote: > > > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > > > functions to support per-VCPU settings. > > > = > > = > > I will need Dario or George to review the logic of the code. > > = > Sure, it's on my short TODO list. It's either going to be today or > Monday. > = > > If some of the comments below don't make sense, just ask. I'm sure I > > make stupid comments at times. > > = > Yeah, I'm sure you've said plenty of stupid things! ;-P ;-P > = Yeah. My trick is that when I say too many stupid things people don't know which one to remember so I'm safe. :-) > > > +{ > > > +=A0=A0=A0=A0if (period !=3D LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT)= { > > > +=A0=A0=A0=A0=A0=A0=A0=A0if (period < 1) { > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0LOG(ERROR, "VCPU period is out o= f range, " > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0"valid values are larger than or equal to > > > 1"); > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return 1; /* error scheduling pa= rameter */ > > = > > Though this is internal function I would very like it to stick to > > CODING_STYLE in libxl. In this particular case, the error handling > > should be using goto and the return value should be a ERROR_* value. > > = > > BTW there is no upper bound check for this value? Just asking -- I > > don't > > know enough to judge. > > = > It's checked in the hypervisor. As usual, in these cases, checking in > tools as well would make things more robust, allow better error > reporting, etc, _BUT_ it would require to keep the limits in sync, > which is undesirable. > = > So, as long as type-related confusion is not a possibility, I would be > ok with no checks here in libxl. > = > And just to be sure that we are on the safe side wrt that: in Xen these > values are uint32, should we use uint32 here as well (in the idl, > instead of 'integer')? > = > > > +=A0=A0=A0=A0} > > > +=A0=A0=A0=A0max_vcpuid =3D info.max_vcpu_id; > > > + > > > +=A0=A0=A0=A0if (scinfo->num_vcpus > 0) { > > > +=A0=A0=A0=A0=A0=A0=A0=A0num_vcpus =3D scinfo->num_vcpus; > > > +=A0=A0=A0=A0=A0=A0=A0=A0GCNEW_ARRAY(vcpus, num_vcpus); > > > +=A0=A0=A0=A0=A0=A0=A0=A0for (i =3D 0; i < num_vcpus; i++) { > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (scinfo->vcpus[i].vcpuid < 0 = || > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scinfo->= vcpus[i].vcpuid > max_vcpuid) { > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0LOG(ERROR, "VCPU ind= ex is out of range, " > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0"valid values are within range from 0 > > > to %d", > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0max_vcpuid); > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return ERROR_INVAL; > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0} > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0vcpus[i].vcpuid =3D scinfo->vcpu= s[i].vcpuid; > > > + > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0rc =3D sched_rtds_validate_param= s(gc, > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scinfo->= vcpus[i].period, scinfo- > > > >vcpus[i].budget, > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0&vcpus[i= ].s.rtds.period, > > > &vcpus[i].s.rtds.budget); > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (rc) > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return ERROR_INVAL; > > > +=A0=A0=A0=A0=A0=A0=A0=A0} > > > +=A0=A0=A0=A0} else { > > > +=A0=A0=A0=A0=A0=A0=A0=A0num_vcpus =3D max_vcpuid + 1; > > > +=A0=A0=A0=A0=A0=A0=A0=A0GCNEW_ARRAY(vcpus, num_vcpus); > > > +=A0=A0=A0=A0=A0=A0=A0=A0if (sched_rtds_validate_params(gc, scinfo- > > > >vcpus[0].period, > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scinfo->vcpus[0].budget, > > = > > This doesn't make sense. You take this path because scinfo->num_vcpus = > > is > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > > = > IIRC, the idea here may be that this is how we set all the vcpus > parameters to the same values... But I'll get back to this when > properly reviewing the series. > = It's one thing that when ->num_vcpus =3D=3D 0 you allocate array, it's another when the array is non-NULL but num_vcpus =3D=3D 0. Such usage is bad. What if I need to iterate through the array at some point? How do you know if it is really a NULL array? Wei. > Thanks and Regards, > Dario > -- = > <> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) > =