From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH for 4.7 4/4] xen: adopt .deinit_pdata and improve timer handling Date: Mon, 9 May 2016 15:22:10 +0200 Message-ID: <1462800130.5968.10.camel@citrix.com> References: <146231184906.25631.6550047090421454264.stgit@Solace.fritz.box> <146231201861.25631.15476137738176988146.stgit@Solace.fritz.box> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3274653084106437799==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1azl8a-0007vp-He for xen-devel@lists.xenproject.org; Mon, 09 May 2016 13:22:16 +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 Cc: "xen-devel@lists.xenproject.org" , Tianyang Chen , Wei Liu , George Dunlap List-Id: xen-devel@lists.xenproject.org --===============3274653084106437799== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-0CVHqlHvEP3Y1DlCaRhW" --=-0CVHqlHvEP3Y1DlCaRhW Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, 2016-05-07 at 23:19 +0200, Meng Xu wrote: > On Tue, May 3, 2016 at 5:46 PM, Dario Faggioli > wrote: > >=20 > >=20 > > The scheduling hooks API is now used properly, and no > > initialization or de-initialization happen in > > alloc/free_pdata any longer. > >=20 > > In fact, just like it is for Credit2, there is no real > > need for implementing alloc_pdata and free_pdata. > >=20 > > This also made it possible to improve the replenishment > > timer handling logic, such that now the timer is always > > kept on one of the pCPU of the scheduler it's servicing. > > Before this commit, in fact, even if the pCPU where the > > timer happened to be initialized at creation time was > > moved to another cpupool, the timer stayed there, > > potentially inferfearing with the new scheduler of the > > pCPU itself. > >=20 > > Signed-off-by: Dario Faggioli > > -- > Reviewed-and-Tested-by: Meng Xu >=20 Thanks! :-) > I do have a minor comment about the patch, although it is not > important at all and it is not really about this patch... >=20 > >=20 > > @@ -614,7 +612,8 @@ rt_deinit(struct scheduler *ops) > > =C2=A0{ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct rt_private *prv =3D rt_priv(ops); > >=20 > > -=C2=A0=C2=A0=C2=A0=C2=A0kill_timer(prv->repl_timer); > > +=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(prv->repl_timer->status =3D=3D TIMER_ST= ATUS_invalid || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv-= >repl_timer->status =3D=3D TIMER_STATUS_killed); > I found in xen/timer.h, the comment after the definition of the > TIMER_STATUS_invalid is >=20 > =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=A0=C2=A0=C2=A0#define TIMER_STATUS_invalid=C2=A0=C2=A00 /* Should= never see > this.=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ >=20 > This comment is a little contrary to how the status is used here. > Actually, what does it exactly means by "Should never see this"? >=20 > This _invalid status is used in timer.h and it is the status when a > timer is initialized by init_timer(). >=20 As far as my understanding goes, this means that a timer, during its operations, should never be found in this state. In fact, this mark a situation where the timer has been allocated but never initialized, and there are ASSERT()s around to enforce that. However, if what one wants is _exactly_ to check whether the timer has been allocated ut not initialized, I don't see why I can't use this. > So I'm thinking maybe this comment can be better improved to avoid > confusion? >=20 I don't think things are confusing, neither right now, nor after this patch, but I'm open to others' opinion. :-) 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) --=-0CVHqlHvEP3Y1DlCaRhW 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 iQIcBAABCAAGBQJXMI8CAAoJEBZCeImluHPu1cgQAO2KW2wIdO6qx5JbIx3PDh// TRYyxnjRcBpcYzGEMPA4QVlWW8qrqNJTFia0XbHcLjw8dz588MZh9/QlAlpIUe7Y wAjz2jH9nb2lDUPi/tOo/WixH+/uLaRkBfYqJd3FCf7ArI9gf6v3nWhY9nSdT/q5 fNNs9FY2kBDi/QzdeF7VEqzqzix2lnOHD2jNnlDTZIHdm0GvdRRkMeVyk6ZtRC6Q IiQUsuUp1U9X3D20VR05/jn3Urt2zaEE9Va+7vHLYSLMIVNjBRKxied3MVG8bWmy zdU1yfLDkOfOa5Y+KOkuLq07wwBHdp6oBzQw9rtFlaYPmfOtpXSwxwzNQkdnjBgo 3Pr91ZQ/9WTzGUI1S6g24sU4feBET9zF4F5iaOx0d3KGbRVcFF3QGFdMArpIugvz lhhXdxS2M4nNsGTbjdGaTehscz2Lo/tMNZntXFr+Jsxa3jnEUK64OURCJbhRA9Dn T+L29q45PBht7NmBNyIAxwN32d1L8+MWgFrY9G6qp/yVwUg9ESpaLoCiQPfv4LRI d3c5HMG5ZNcoyvgxTaXRko752xIgYL8VUsUG1+yV4cjcjobQWYdEyz113tLa2/Lk WiN61sL7pfwUTOnsakPi7/etxBXIXGPiXGNWpkj+6SVSiqUMjnFkmgigP+BWFCrL CSKkenviAhZnhJq1qX3S =zEuG -----END PGP SIGNATURE----- --=-0CVHqlHvEP3Y1DlCaRhW-- --===============3274653084106437799== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============3274653084106437799==--