lkml.org 
[lkml]   [2009]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC][PATCH] Dynamic Tick: Allow 32-bit machines to sleep formorethan2.15 seconds

John Stultz wrote:
>>>> One final question, I noticed in clocksource.h that the definition of
>>>> function cyc2ns returns a type of s64, however, in the function itself a
>>>> variable of type u64 is used and returned. Should this function be
>>>> modified as follows?
>>>>
>>>> static inline s64 cyc2ns(struct clocksource *cs, cycle_t cycles)
>>>> {
>>>> - u64 ret = (u64)cycles;
>>>> + s64 ret = (s64)cycles;
>>>> ret = (ret * cs->mult) >> cs->shift;
>>>> return ret;
>>>> }
>>> Damn. So this brings up an issue I had missed prior.
>> Any comments on whether this should be u64 versus s64?
>
> I'd leave it alone for now. I'm concerns that in large multiplies, if
> its a s64 the sign might get extended down by the shift. I need to look
> at it in more detail though.

I have been thinking about this some more and I do agree that there is a
chance that the multiply could overflow if the "cycles" and "mult" are
large. From the perspective of the timekeeping_max_deferment() function
this would be very likely for 64-bit clocksources when the mask will be
equal to (2^64)-1. Therefore, how about modifying the function as
follows in order to catch any occurrences of overflow?

Let me know if this is aligned with your thinking or if I am barking up
the wrong tree here.

Cheers
Jon


diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 507235a..8204373 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -316,8 +316,32 @@ static inline void clocksource_disable(struct
clocksource *cs)
*/
static inline s64 cyc2ns(struct clocksource *cs, cycle_t cycles)
{
- s64 ret = (s64)cycles;
- ret = (ret * cs->mult) >> cs->shift;
+ s64 ret;
+ u64 upper, lower, overflow;
+
+ /*
+ * Split the calculation into two halves to ensure
+ * that we can catch any overflow that may occur.
+ */
+ upper = ((cycles >> 32) * cs->mult) >> cs->shift;
+ lower = ((cycles & 0xFFFFFFFF) * cs->mult) >> cs->shift;
+
+ /*
+ * Check to see if the result will overflow. If
+ * overflow is non-zero then the result is greater
+ * than 63-bits which is the max positive value
+ * for a signed result.
+ */
+ overflow = (upper + (lower >> 32)) >> 31;
+
+ /*
+ * If the result overflows, return the max value we can.
+ */
+ if (overflow)
+ ret = LONG_MAX;
+ else
+ ret = (s64)((upper << 32) + lower);
+
return ret;
}

--
1.6.1

\
 
 \ /
  Last update: 2009-05-15 18:39    [W:0.065 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site