From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen: sched: rtds: refactor code Date: Fri, 13 May 2016 09:41:41 +0200 Message-ID: <1463125301.18789.21.camel@citrix.com> References: <1462980016-4854-1-git-send-email-tiche@seas.upenn.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7280329808741228337==" Return-path: Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b17k7-0007hk-H8 for xen-devel@lists.xenproject.org; Fri, 13 May 2016 07:42:39 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Meng Xu , Tianyang Chen Cc: "xen-devel@lists.xenproject.org" , George Dunlap List-Id: xen-devel@lists.xenproject.org --===============7280329808741228337== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-lP4/MXnoj9y2njlXbiDL" --=-lP4/MXnoj9y2njlXbiDL Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2016-05-12 at 23:34 -0400, Meng Xu wrote: > On Wed, May 11, 2016 at 11:20 AM, Tianyang Chen > wrote: > >=20 > > No functional change: > > =C2=A0-Various coding style fix > > =C2=A0-Added comments for UPDATE_LIMIT_SHIFT. > >=20 Hey, Tianyang, thanks for this. > > Use non-atomic bit-ops: > > =C2=A0-Vcpu flags are checked and cleared atomically. Performance can b= e > > =C2=A0 improved with corresponding non-atomic versions since schedule.c > > =C2=A0 already has spin_locks in place. > >=20 And this too. However, I think the patch should be split at least in two, one doing the style/comments cleanups, the other doing the atomic/non-atomic switch. > > Suggested-by: Dario Faggioli > It's better to add the link to the thread that has the suggestion. >=20 Yes, suggested-by tags are not especially useful, IMHO. I'm fine with you using it, but as Meng says, a link would be more helpful. If you send a series, which will have a cover letter, put the link(s) in the cover letter rather than in the changelog, as that's an important information when reviewing, much less when looking at git-log in 5 years time. :-) > > @@ -930,7 +936,7 @@ burn_budget(const struct scheduler *ops, struct > > rt_vcpu *svc, s_time_t now) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( svc->cur_budget <=3D 0 ) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0svc->cur_budget = =3D 0; > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0set_bit(__RTDS_deplete= d, &svc->flags); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0__set_bit(__RTDS_deple= ted, &svc->flags); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* TRACE */ > > @@ -955,7 +961,7 @@ burn_budget(const struct scheduler *ops, struct > > rt_vcpu *svc, s_time_t now) > > =C2=A0 * lock is grabbed before calling this function > The comment says "lock is grabbed before calling this function". > IIRC,=C2=A0=C2=A0we use __ to represent that we grab the lock before call= this > function. > Then this change violates the convention. >=20 Yeah, but it was a bad convention, IMO, at least for sched_*.c files.=C2=A0 In fact, it is debatable whether one or two underscore is what should really be used. Also, that's not the only convention that lead to the introduction of this king of prefix (for example, I've seen many times '__' used for indicating some sort of 'raw' part of the operation... there are examples of this in Xen too)... Which means one never knows which one is the valid convention at some point, about '__'! :-O But even if we leave this aside, and consider at the '__'=3D=3D>locked rule, well, pretty much all functions in sched_rt.c are called with the proper locks held already. Basically, since in many occasione, the lock has been taken in schedule.c before calling the hook, all the functions eve called by an hook --end maybe even the hook itself-- should have the '__' prefix, which defeats the purpose of the convention itself! So, on this, I agree with Tianyang, and I think removing the '__' is a good thing for this source file. If there are places where we want to particularly stress the fact that locks should have be taken already, then either add a comment or a '_locked' suffix to the function name. But that should be the exception rather than the rule and, out of the top of my head, I don't recall any cases in sched_rt.c where that would be tremendously helpful... Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-lP4/MXnoj9y2njlXbiDL 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 v2 iQIcBAABCAAGBQJXNYVDAAoJEBZCeImluHPu2r8QAJ6I/lhKjxHGkjyDdevspdai 1rmehfxTeDSJwP94YhiHkXDIlEBqvyRC6j52PFBk1wuYD3FVb1kgVOuPPZKihl4Q hEJpgmVtWwxAHiyL5DNNIwzfEFQtny/VHVK+MbYeObkHzBfA1jYJSQdg0kPVefNY dR55t+tGbtMsd5iCAnCmpbBgwTJ8tylBjzfZnhJcHh/0BZcGjHmPMvJcDXX4WSXh mDjDs2HbdwOLsS7c5t4PxZ9/7YEAIttccbe79RnNwWPP3dHlZTUazfKyd7/Wiufx 68u8y3O/11eRVr6UByQeALreBfigYrleI2a6eXhzuKDLAyhyRYsEwTTazby1zBRS Cyws6uPexnbFfPVtVl0luYw34hdMgkbvLCuCzDVw+J62qeFwXsG7CosRsRX0YwKK 9HxB1w3f204E0MwP2uA7KMCKyMu0SXJj4eWBcC/p6Zkhy7sWh+asMl2ikdkB0ne8 kDWxNF5QygUNexeAKeVTa2vADuK1yxN6Vg3BD2ALtu7gKF7OyXrWlp8e30ggqOhK BfYUH9v9kOj9e9M9asd9xH2vPcBSaXDRg9GTayjY2G8w+mgVs7lxKGjKzcWRq2re odcnk9WMxlaY1Nn4vCmIgXLbITip6zx6eTd/c3kWV3RoA6aoACq0Oe+OOpaDR4yb szawY7A1RB7HPvmHO9dX =Y8Ih -----END PGP SIGNATURE----- --=-lP4/MXnoj9y2njlXbiDL-- --===============7280329808741228337== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============7280329808741228337==--