From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [for-4.8 PATCH] xen: credit2: fix wrong assert in runq_tickle(). Date: Tue, 22 Nov 2016 12:52:04 +0100 Message-ID: <1479815524.2712.19.camel@citrix.com> References: <147981141529.15399.3103284119825700755.stgit@Solace.fritz.box> <5834384A02000078001209E3@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3304387615268533866==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c99cU-0006td-6M for xen-devel@lists.xenproject.org; Tue, 22 Nov 2016 11:52:14 +0000 In-Reply-To: <5834384A02000078001209E3@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: xen-devel@lists.xenproject.org, Wei Liu , George Dunlap List-Id: xen-devel@lists.xenproject.org --===============3304387615268533866== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-07T0uaJHREMLNgltC5XK" --=-07T0uaJHREMLNgltC5XK Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2016-11-22 at 04:21 -0700, Jan Beulich wrote: > > > > On 22.11.16 at 11:43, wrote: > > Signed-off-by: Dario Faggioli >=20 > Reviewed-by: Jan Beulich > Thanks a lot for looking at the patch. > with two remarks: > > [...] >=20 > If you moved this past the if() following this comment, the ipid =3D=3D > -1 > case would already be taken care of, simplifying the code. >=20 True. Though about it, but was two minded. I think the ASSERT is more 'descriptive' if it's kept as close as possible to the for (and in fact, I now even regret having put a blank line in between). But I see your point and agree that it's looking awkward when you see it together with the following if. So, yes, all in all, I think I like your idea better. > And then, having looked back at the commit mentioned in the > description, that one resulted in two constructs like (taking the > code as it looks now) >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0i= f ( cpumask_test_cpu(cpu, &rqd->idle) ) > =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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0__cpumask_clear_cpu(cpu, &rqd->idle); > =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... >=20 > Is there a reason this can't or shouldn't be >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0i= f ( __cpumask_test_and_clear_cpu(cpu, &rqd->idle) ) > =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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0... > ? >=20 It can indeed. You can find it done as it is right now in other places, in scheduling code, though, I guess for cache betterness, as Juergen is saying. I remember not being sure which way to go, and eventually leaning toward this one. I'm still not sure what's best, but it'd be a cleanup/optimization, and would IMO require, if done, more than just that (e.g., comments should be improved). So I'd be inclined to consider this 4.9 material... But thanks for bringing it up. :-) So, I'm resending this patch with the ASSERT moved below the if, and I'm keeping your Reviewed-by. Hope that's ok. Thanks again 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) --=-07T0uaJHREMLNgltC5XK 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 iQIcBAABCAAGBQJYNDFlAAoJEBZCeImluHPu2/MP/2mVyHbrb7dxeoSyarw7hVb6 u4LwlvWxXQ5tJF1iyfdOhR0pQtAtJ/eq17jjOG+zhoeMqmNdqs3JANRzXzeuByPD 3TBZ9S0gQgvCccF7M35M6/WBnU8rREkgsOPBO85FzJp4IJcbijOu8KUdOi53du66 cbVuzq5MOJULskHqlBbJd76P1KajEnGlwQrcnKfxyax5Ofs8s4tWyQV/Obd4XXjw jpnP3xb1nNGchBRyCfWhcXARq0wgHoG3BZqSnlRylbHj4gm1DTgqYpDTNFwQZYmP JD1PcEatrjx9xFDUz5CgXuUEmC/a9E9xzabjdMMkY5lITxavo0IrFFCa2HLs+MBr UUCjYEhV/Uqcf1bDMt5BAVsUOvRT9PgI0Led49SKyAoztAqSw/BamIZJgtG5lXcD KeBniBWNCfpUx2K5WDe98P0+7vH2+grPM07tJwHLHdTjMEulKURWS1Nc2+SdLWvu ToICdmG1D8xDJDXN28xuExzd6oWGsN9ohWwcZ+EPKiXlk5rJ794VKklcxVbTk8/w nQgJNQdb8/HQ0nLl8f4vGPaNufIZzb9XelQnpwRfzrdMTllU3wPsJd3WLf/gmTeY DBkLE9NReTtmMDNSaVjx5AtkQgM0PVu5gpmYJdkSshMRefCZDXt2zO1MGNAnAi6A OoeULxbGXEhz6Vd4PEY2 =FM92 -----END PGP SIGNATURE----- --=-07T0uaJHREMLNgltC5XK-- --===============3304387615268533866== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============3304387615268533866==--