From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Keith Packard" Subject: Re: [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2] Date: Tue, 19 Jun 2018 17:36:45 -0700 Message-ID: <87lgbaqoo2.fsf@keithp.com> References: <20180615025256.10657-1-keithp@keithp.com> <20180615025256.10657-5-keithp@keithp.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0574955945==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jason Ekstrand Cc: ML mesa-dev , Maling list - DRI developers List-Id: dri-devel@lists.freedesktop.org --===============0574955945== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Jason Ekstrand writes: > I still don't really like this but I agree that this code really should n= ot > be getting hit very often so it's probably not too bad. Maybe one day > we'll be able to drop the non-syncobj paths entirely. Wouldn't that be > nice. I agree. What I want to have is kernel-side syncobj support for all of the WSI fences that we need here. I thought about using syncobjs and signaling them from user-space, but realized that we couldn't eliminate the non-syncobj path quite yet as that requires a new enough kernel. And, if we want to get to kernel-native WSI syncobjs, that would mean we'd eventually have three code paths. I think it's better to have one 'legacy' path, which works on all kernels, and then one 'modern' path which does what we want. > In the mean time, this is probably fine. I did see one issue below > with time conversions that should be fixed though. Always a hard thing to get right. >> +static uint64_t anv_get_absolute_timeout(uint64_t timeout) >> +{ >> + if (timeout =3D=3D 0) >> + return 0; >> + uint64_t current_time =3D gettime_ns(); >> + >> + timeout =3D MIN2(INT64_MAX - current_time, timeout); >> + >> + return (current_time + timeout); >> +} >> > > This does not have the same behavior as the code it replaces. In > particular, the old code saturates at INT64_MAX whereas this code does not > do so properly anymore. If UINT64_MAX gets passed into timeout, I believe > that will be implicitly casted to signed and the MIN will yield -1 which > gets casted back to unsigned and you get UINT64_MAX again. It won't not get implicitly casted to signed; the implicit cast works the other way where OP implicitly casts the operand to unsigned for types of the same 'rank' (where 'rank' is not quite equivalent to size). Here's a fine article on this: https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+c= onversion+rules However, this is a rather obscure part of the ISO standard, and I don't think we should expect that people reading the code know it well. Adding a cast to make it clear what we want is a good idea. How about this? static uint64_t anv_get_absolute_timeout(uint64_t timeout) { if (timeout =3D=3D 0) return 0; uint64_t current_time =3D gettime_ns(); uint64_t max_timeout =3D (uint64_t) INT64_MAX - current_time; timeout =3D MIN2(max_timeout, timeout); return (current_time + timeout); } > This is a problem because the value we pass into the kernel ioctls is > signed. The behavior of the kernel *should* be infinite when timeout > < 0 but, on some older kernels, -1 is treated as 0 and you get no > timeout. Yeah, we definitely want to cap the values to INT64_MAX. >> - else if (current_ns + _timeout > INT64_MAX) >> > > I suspect we need to cast INT64_MAX to a uint64_t here to ensure we don't > accidentally get an implicit conversion to signed. Again, it's not necessary given the C conversion rules, but it is a good way to clarify the intent. =2D-=20 =2Dkeith --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEw4O3eCVWE9/bQJ2R2yIaaQAAABEFAlspoZ0ACgkQ2yIaaQAA ABHU4w/+OJgGJdFRiBB7mBIDLherzPblWSyzoK2qk00m9/ihZbUXT8+mKSda44oZ i3enZRUCLTfVSnN0KDKZg7NitxHLXsUqKa+GsGDibNKL/8n9v1pEZuO1NO6fYWip fazpZNwmMH+iTw/JwmSZPvOkMXkk4L2ASbO4GnqcrEelcW+ENJL3HqGiLP4HOyCw EK5YolVzSOf3vG3Dl87wgjWXc+5yMZBr/MQ6nWx2xGzLrilgaq15DklCMSWIQiEJ SPwxQ/2QyQQLojEp4qUkzg4KusZzSzPejD5YkggVRk9uHSdsjin6ExbYzL+XZiTn X9nBjoSQRx7QnVkPepXq584Px7AncCtUnLyI/GuaW8txvNHu6XKfN3yd89+eJoFV NrHm/mTGJE8dZZ1DhLgLtjHftTHSqkYZz5+qQFUqQR/pMfGqgRiOffP1T2Jfoo9W x9lLSogv0Z2xUzJsCBKSK/mWl4nnxy2Ki+sCvLcLPWIANaPopDDqvcgmP2s2Ytej U/D6WzcbVOLY/qgLShV8+KHEYnAnAcHm/Aot/od31+TdwNKE1vagLZ42q2KtrB/u sRZNhrTbn0DK1ihly1xxauibwfzOH+Ed3hGfX8RYVP/nouSg3C3dM5tR8kG0d9c+ LJuDAFVyucb3jaZklN/nb6awsmoxc4mrePxrY1Sp1u+IDuchUqA= =/Q0b -----END PGP SIGNATURE----- --=-=-=-- --===============0574955945== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0574955945==--