<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 19, 2018 at 5:36 PM, Keith Packard <span dir="ltr"><<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
> I still don't really like this but I agree that this code really should not<br>
> be getting hit very often so it's probably not too bad.  Maybe one day<br>
> we'll be able to drop the non-syncobj paths entirely.  Wouldn't that be<br>
> nice.<br>
<br>
</span>I agree. What I want to have is kernel-side syncobj support for all of<br>
the WSI fences that we need here.<br>
<br>
I thought about using syncobjs and signaling them from user-space, but<br>
realized that we couldn't eliminate the non-syncobj path quite yet as<br>
that requires a new enough kernel. And, if we want to get to<br>
kernel-native WSI syncobjs, that would mean we'd eventually have three<br>
code paths. I think it's better to have one 'legacy' path, which works<br>
on all kernels, and then one 'modern' path which does what we want.<br>
<span class=""><br>
> In the mean time, this is probably fine.  I did see one issue below<br>
> with time conversions that should be fixed though.<br>
<br>
</span>Always a hard thing to get right.<br>
<span class=""><br>
>> +static uint64_t anv_get_absolute_timeout(<wbr>uint64_t timeout)<br>
>> +{<br>
>> +   if (timeout == 0)<br>
>> +      return 0;<br>
>> +   uint64_t current_time = gettime_ns();<br>
>> +<br>
>> +   timeout = MIN2(INT64_MAX - current_time, timeout);<br>
>> +<br>
>> +   return (current_time + timeout);<br>
>> +}<br>
>><br>
><br>
> This does not have the same behavior as the code it replaces.  In<br>
> particular, the old code saturates at INT64_MAX whereas this code does not<br>
> do so properly anymore.  If UINT64_MAX gets passed into timeout, I believe<br>
> that will be implicitly casted to signed and the MIN will yield -1 which<br>
> gets casted back to unsigned and you get UINT64_MAX again.<br>
<br>
</span>It won't not get implicitly casted to signed; the implicit cast works<br>
the other way where <signed> OP <unsigned> implicitly casts the <signed><br>
operand to unsigned for types of the same 'rank' (where 'rank' is not<br>
quite equivalent to size). Here's a fine article on this:<br>
<br>
<a href="https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules" rel="noreferrer" target="_blank">https://wiki.sei.cmu.edu/<wbr>confluence/display/c/INT02-C.+<wbr>Understand+integer+conversion+<wbr>rules</a><br>
<br>
However, this is a rather obscure part of the ISO standard, and I don't<br>
think we should expect that people reading the code know it well.</blockquote><div><br></div><div>This is why I insert casts like mad in these scenarios. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Adding<br>
a cast to make it clear what we want is a good idea. How about this?<br>
<br>
        static uint64_t anv_get_absolute_timeout(<wbr>uint64_t timeout)<br>
        {<br>
           if (timeout == 0)<br>
              return 0;<br>
           uint64_t current_time = gettime_ns();<br>
           uint64_t max_timeout = (uint64_t) INT64_MAX - current_time;<br></blockquote><div><br></div><div>I suppose we probably shouldn't worry about current_time being greater than INT64_MAX?  I guess if that happens we have pretty big problems...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
           timeout = MIN2(max_timeout, timeout);<br>
<br>
           return (current_time + timeout);<br>
<span class="">        }<br></span></blockquote><div><br></div><div>Yeah, I think that's better.</div><div><br></div><div><font color="#888888">--Jason</font><br></div></div><br></div></div>