From mboxrd@z Thu Jan 1 00:00:00 1970 From: Meng Xu Subject: Re: [PATCH v1 3/4] libxl: add rt scheduler Date: Mon, 25 Aug 2014 11:55:26 -0400 Message-ID: References: <1408921125-21470-1-git-send-email-mengxu@cis.upenn.edu> <1408921125-21470-4-git-send-email-mengxu@cis.upenn.edu> <20140825131740.GC32384@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3657135195701706835==" Return-path: In-Reply-To: <20140825131740.GC32384@zion.uk.xensource.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: Wei Liu Cc: Ian Campbell , Sisu Xi , Stefano Stabellini , George Dunlap , Dario Faggioli , Ian Jackson , "xen-devel@lists.xen.org" , Meng Xu , Jan Beulich , Chao Wang , Chong Li , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org --===============3657135195701706835== Content-Type: multipart/alternative; boundary=047d7b2e4e3e27d0c60501763753 --047d7b2e4e3e27d0c60501763753 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Wei, Thank you very much for your quick review! I really appreciate it. :-) In summary, I correct those code you pointed out. > > > + LOGE(ERROR, "Allocate sdom array fails\n"); > > + return ERROR_INVAL; > > + } > > + > > + rc =3D xc_sched_rt_domain_get(CTX->xch, domid, sdom, num_vcpus); > > + if (rc !=3D 0) { > > + LOGE(ERROR, "getting domain sched rt"); > > + return ERROR_FAIL; > > + } > > + > > + /* FIXME: how to guarantee libxl_*_dispose be called exactly once? > */ > > + libxl_domain_sched_params_init(scinfo); > > + > > libxl_*_dispose should be idempotent (as least we intent to make it so). > And it's caller's responsibility to ensure it's called once if it wishes > to. > =E2=80=8BI see. Right now, it is disposed by the caller in xl.=E2=80=8B > > + if (vcpu_index < 0 || vcpu_index > num_vcpus) > > + { > > Libxl coding style normally has { in the same line of "if" "for" etc. > Please > check and fix other occurences. > =E2=80=8BA quick question: Should I use the style "if () { " for all files in the tool stack or just in libxl files? If I need to use this style in all tool stack files, I will check and modify them. (Right now, I just made the correction for this exact patch.) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > > index bfeb3bc..4657056 100644 > > --- a/tools/libxl/libxl.h > > +++ b/tools/libxl/libxl.h > > @@ -1240,6 +1240,13 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx= , > uint32_t poolid, > > #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT -1 > > #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1 > > > > +#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1 > > +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_DEFAULT -1 > > +#define LIBXL_DOMAIN_SCHED_PARAM_NUM_VCPUS_DEFAULT -1 > > +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1 > > +/* Consistent with XEN_LEGACY_MAX_VCPUS xen/arch-x86/xen.h*/ > > +#define LIBXL_XEN_LEGACY_MAX_VCPUS 32 > > + > > Where is this macro used? I cannot find it in this one and following xl > patch. > > =E2=80=8BMy bad. This should exist any more since we dynamically allocate v= cpus structure based on the number of vcpus in a domain to display vcpus' information. Delete it now. Thank you again for your useful comments, Wei! Best, Meng --=20 ----------- Meng Xu PhD Student in Computer and Information Science University of Pennsylvania --047d7b2e4e3e27d0c60501763753 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi = Wei,

=
Thank you very much = for your quick review! I really appreciate it. :-)

In summary, I correct those co= de you pointed out.=C2=A0
=C2=A0
<= div class=3D"gmail_extra">

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGE(ERROR, "Allocate sdom array fai= ls\n");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ERROR_INVAL;
> +=C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 rc =3D xc_sched_rt_domain_get(CTX->xch, domid, sdom,= num_vcpus);
> +=C2=A0 =C2=A0 if (rc !=3D 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGE(ERROR, "getting domain sched rt= ");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ERROR_FAIL;
> +=C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 /* FIXME: how to guarantee libxl_*_dispose be called ex= actly once? */
> +=C2=A0 =C2=A0 libxl_domain_sched_params_init(scinfo);
> +

libxl_*_dispose should be idempotent (as least we intent to make it s= o).
And it's caller's responsibility to ensure it's called once if = it wishes
to.

=E2=80=8BI see. Right now, it is disposed by the calle= r in xl.=E2=80=8B


> +=C2=A0 =C2=A0 if (vcpu_index < 0 || vcpu_index > num_vcpus)
> +=C2=A0 =C2=A0 {

Libxl coding style normally has { in the same line of "if&= quot; "for" etc. Please
check and fix other occurences.

=E2=80=8BA quick question:=C2=A0
Should I use the style "if () { " for all files in the= tool stack or just in libxl files?

If I need to use this style in = all tool stack files, I will check and modify them. (Right now, I just made= the correction for this exact patch.)
=C2=A0
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index bfeb3bc..4657056 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1240,6 +1240,13 @@ int libxl_sched_credit_params_set(libxl_ctx *ct= x, uint32_t poolid,
>=C2=A0 #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT=C2=A0 =C2=A0-1<= br> >=C2=A0 #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
>
> +#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT=C2=A0 =C2=A0 =C2=A0-1=
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_DEFAULT=C2=A0 =C2=A0 =C2=A0 =C2= =A0-1
> +#define LIBXL_DOMAIN_SCHED_PARAM_NUM_VCPUS_DEFAULT=C2=A0 -1
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
> +/* Consistent with XEN_LEGACY_MAX_VCPUS xen/arch-x86/xen.h*/
> +#define LIBXL_XEN_LEGACY_MAX_VCPUS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 32
> +

Where is this macro used? I cannot find it in this one and foll= owing xl
patch.


=E2=80=8BMy bad. This should exist any more since we dynamically allocate= vcpus structure based on the number of vcpus in a domain to display vcpus&= #39; information. =C2=A0
Delete it now.=C2=A0=

Thank you again for your= useful comments, Wei!=C2=A0

Best,

Meng



--


-----------
Meng Xu
PhD Student in Computer and Information= Science
University of Pennsylvania
--047d7b2e4e3e27d0c60501763753-- --===============3657135195701706835== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============3657135195701706835==--