From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4 7/7] tools: enable Cache QoS Monitoring feature for libxl/libxc Date: Wed, 4 Dec 2013 09:14:52 +0100 Message-ID: <1386144892.3926.34.camel@Abyss> References: <1386060479-17205-1-git-send-email-dongxiao.xu@intel.com> <1386060479-17205-8-git-send-email-dongxiao.xu@intel.com> <1386073021.5338.320.camel@Solace> <40776A41FC278F40B59438AD47D147A9118CD7AD@SHSMSX104.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8035258267803852843==" Return-path: In-Reply-To: <40776A41FC278F40B59438AD47D147A9118CD7AD@SHSMSX104.ccr.corp.intel.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: "Xu, Dongxiao" Cc: "keir@xen.org" , "Ian.Campbell@citrix.com" , "stefano.stabellini@eu.citrix.com" , "andrew.cooper3@citrix.com" , "Ian.Jackson@eu.citrix.com" , "xen-devel@lists.xen.org" , "JBeulich@suse.com" , "dgdegra@tycho.nsa.gov" List-Id: xen-devel@lists.xenproject.org --===============8035258267803852843== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-KeSwrIdkREtytYojVugu" --=-KeSwrIdkREtytYojVugu Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On mer, 2013-12-04 at 02:44 +0000, Xu, Dongxiao wrote: > > -----Original Message----- > > From: Dario Faggioli [mailto:dario.faggioli@citrix.com] > > Would it be possible to split this patch in 3, one for libxc, one for > > libxl and one for xl? >=20 > Originally the patch is split (by functional), and later I merged them ac= cording to Andrew's suggestion. > I think merge is okay since the logic is simple and straightforward. >=20 Oh, sorry for not noticing that. My personal preference is to always split, even if logic is trivial, but again, that's just me. :-) > > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > > > index c7dceda..fdca92d 100644 > > > --- a/tools/libxl/libxl.h > > > +++ b/tools/libxl/libxl.h > > > @@ -285,6 +285,7 @@ > > > > > > #include > > > #include <_libxl_list.h> > > > +#include > > > > > Is this really necessary? I think it shouldn't... is alread= y > > included in "libxl_internal.h", which you are including yourself below, > > so... >=20 > libxl.h will reference "sysctl_cqminfo_t", which is defined in xenctrl.h. > I didn't see libxl_internal.h is included in libxl.h, can you help to poi= nt it out? >=20 I see. No, all I said is that your new file, libxl_pqos.c, already includes xenctrl.h, and hence I was asking whether that is not enough and if not, why, and you just answered. TBH, it still looks wrong to me. It does not happen for any other similar situations and data type (or at least situations and data type that look similar enough to me). What we do there, is defining a libxl counterpart of the xc_* type, use it in the libxl interface and translate between the twos _inside_ the libxl function, which is implemented somewhere where libxl_internal.h, and then xenctrl.h, is included, and have no problem seeing the xc_* type declaration. Look, for example at libxl_get_physinfo(), xc_physinfo(), xc_physinfo_t. Or you think your case is somewhat different? If yes, how? > > > +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, const char * q= os_type) > > > +{ > > > + int rc; > > > + uint32_t flags =3D 0; > > > + > > > + if (!strncmp(qos_type, "cqm", 3)) > > > + flags |=3D XEN_DOMCTL_pqos_cqm; > > > + else { > > > + rc =3D -EINVAL; > > > + LIBXL__LOG(ctx, XTL_ERROR, "%s", msg[EINVAL]); > > > > > I think new code should use the LOG() / LOGE() variant of the logging > > macros. >=20 > I saw a lot of existing code still uses LIBXL__LOG() function. > Besides, if we use LOGE(), we need to pass another variable "gc" to the f= unction... > Is this a guideline that we will stick to LOG()/LOGE() and deprecate call= ing of LIBXL__LOG() in later code? >=20 I don't think it's written anywhere yet, but I'm not sure. Anyway, I've seen similar requests on this list (even got one! :-P) from some time now. As per the new gc parameter, that's not true, all you need is adding a GC_INIT(ctx); call at the beginning of the function and a GC_FREE at the end. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-KeSwrIdkREtytYojVugu Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iEYEABECAAYFAlKe5HwACgkQk4XaBE3IOsSSzwCffLu/4xMIY5QRTamtvHOU20ga 80oAoIoF3YBgHU1/sJRws9vPwHwtlc6z =tTGc -----END PGP SIGNATURE----- --=-KeSwrIdkREtytYojVugu-- --===============8035258267803852843== 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 --===============8035258267803852843==--