fix msecs_to_jiffies() to not return values greater than MAX_JIFFY_OFFSET
PATCH 1/1 fix msecs_to_jiffies() to not return values greater than MAX_JIFFY_OFFSET
The documentation of msecs_to_jiffies() says:
* - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET) *
* - 'too large' values [that would result in larger than
* MAX_JIFFY_OFFSET values] mean 'infinite timeout' too.
But when you pass in e.g. MAX_JIFFY_OFFSET + 1000 for HZ = 1000 it will not return MAX_JIFFY_OFFSET, but the bigger value. This makes sure that the value returned from this function can never be bigger than MAX_JIFFY_OFFSET. Also use DIV_ROUND_UP() in one place where that is open coded.
--- kernel/time.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
kernel/time.c +5 -3
@@ -433,6 +433,7 @@ EXPORT_SYMBOL(ns_to_timeval);*/ unsigned long msecs_to_jiffies (const unsigned int m) { +unsigned long r;/* * Negative value, means infinite timeout: */
@@ -445,7 +446,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)* round multiple of HZ, divide with the factor between them, * but round upwards: */ -return (m + (MSEC_PER_SEC /HZ ) -1 ) / (MSEC_PER_SEC /HZ ); + r =DIV_ROUND_UP (m,MSEC_PER_SEC /HZ );#elif HZ >MSEC_PER_SEC && !(HZ %MSEC_PER_SEC )/* * HZ is larger than 1000, and HZ is a nice round multiple of
@@ -457,7 +458,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ; -return m * (HZ /MSEC_PER_SEC ); + r = m * (HZ /MSEC_PER_SEC );#else /* * Generic case - multiply, round and divide. But first
@@ -467,9 +468,10 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (HZ >MSEC_PER_SEC && m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ; -return (MSEC_TO_HZ_MUL32 * m +MSEC_TO_HZ_ADJ32 ) + r = (MSEC_TO_HZ_MUL32 * m +MSEC_TO_HZ_ADJ32 ) >>MSEC_TO_HZ_SHR32 ;#endif + return min_t(unsigned long, r, MAX_JIFFY_OFFSET); }EXPORT_SYMBOL (msecs_to_jiffies);
Signature · 1 line
1.7.3.2
Quoted 9 lines · Rolf Eike Beer
The documentation of
msecs_to_jiffies()says:* - negative values mean 'infinite timeout' (
MAX_JIFFY_OFFSET) *
* - 'too large' values [that would result in larger than
*MAX_JIFFY_OFFSETvalues] mean 'infinite timeout' too.But when you pass in e.g.
MAX_JIFFY_OFFSET+ 1000 for HZ = 1000 it will not returnMAX_JIFFY_OFFSET, but the bigger value. This makes sure that the value
That's only true for 32 bit.
Quoted 19 lines
returned from this function can never be bigger than
MAX_JIFFY_OFFSET. Also useDIV_ROUND_UP()in one place where that is open coded.unsigned long
msecs_to_jiffies(const unsigned int m){kernel/time.c +2
-1+unsigned long r;/* * Negative value, means infinite timeout: */ @@ -445,7 +446,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)* round multiple of HZ, divide with the factor between them, * but round upwards: */ -return (m + (MSEC_PER_SEC /HZ ) -1 ) / (MSEC_PER_SEC /HZ ); + r =DIV_ROUND_UP (m,MSEC_PER_SEC /HZ );#elif HZ >MSEC_PER_SEC && !(HZ %MSEC_PER_SEC )/* * HZ is larger than 1000, and HZ is a nice round multiple of
kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 461
Quoted 5 lines
kernel/time.c +1
-1@@ -457,7 +458,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ; -return m * (HZ /MSEC_PER_SEC ); + r = m * (HZ /MSEC_PER_SEC );
For this case the jiffies_to_msec() check should be sufficient.
kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 469
Quoted 6 lines
kernel/time.c +0
-0#else /* * Generic case - multiply, round and divide. But first @@ -467,9 +468,10 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (HZ >MSEC_PER_SEC && m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ;
Hmm, this check is silly. MUL32 is chosen, so that we cannot overflow.
kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 471
Quoted 2 lines
kernel/time.c +1
-1-return (MSEC_TO_HZ_MUL32 * m +MSEC_TO_HZ_ADJ32 ) + r = (MSEC_TO_HZ_MUL32 * m +MSEC_TO_HZ_ADJ32 )
>> MSEC_TO_HZ_SHR32;
kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 476
Quoted 4 lines
kernel/time.c +1
-0#endif + return min_t(unsigned long, r, MAX_JIFFY_OFFSET); }EXPORT_SYMBOL (msecs_to_jiffies);
I start to wonder whether we really need these three variants or whether we just could go with that MUL/SHIFT based implementation and a final check for MAX_JIFFY_OFFSET. That would boil down to:
unsigned long msecs_to_jiffies(const unsigned int m)
{
u64 res = (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32) >> MSEC_TO_HZ_SHR32;
return min_t(unsigned long, (unsigned long)res, MAX_JIFFY_OFFSET);
}That'd avoid the whole division and msecs_to_jiffies() is not really a high precision function.
Thanks,
tglx
Am Dienstag, 29. März 2011, 21:42:59 schrieb Thomas Gleixner:
Quoted 9 lines · Rolf Eike Beer
kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 471
Quoted 6 lines
kernel/time.c +0
-0@@ -467,9 +468,10 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (HZ >MSEC_PER_SEC && m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ;Hmm, this check is silly. MUL32 is chosen, so that we cannot overflow.
IMHO the second part of the check should be enough, no? If m is greater than j2m(MAX) then it's irrelevant what's up else, it's too big. Period.
Eike
Am Dienstag, 29. März 2011, 21:42:59 schrieb Thomas Gleixner:
Quoted 84 lines · Rolf Eike Beer
Quoted 9 lines
The documentation of
msecs_to_jiffies()says:
* - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET) *
* - 'too large' values [that would result in larger than
*MAX_JIFFY_OFFSETvalues] mean 'infinite timeout' too.But when you pass in e.g.
MAX_JIFFY_OFFSET+ 1000 for HZ = 1000 it will not returnMAX_JIFFY_OFFSET, but the bigger value. This makes sure that the valueThat's only true for 32 bit.
Quoted 29 lines
returned from this function can never be bigger than
MAX_JIFFY_OFFSET. Also useDIV_ROUND_UP()in one place where that is open coded.unsigned long
msecs_to_jiffies(const unsigned int m){kernel/time.c +2
-1+unsigned long r;/* * Negative value, means infinite timeout: */ @@ -445,7 +446,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)* round multiple of HZ, divide with the factor between them, * but round upwards: */ -return (m + (MSEC_PER_SEC /HZ ) -1 ) / (MSEC_PER_SEC /HZ ); + r =DIV_ROUND_UP (m,MSEC_PER_SEC /HZ );#elif HZ >
MSEC_PER_SEC&& !(HZ %MSEC_PER_SEC)kernel/time.c +0
-0/* * HZ is larger than 1000, and HZ is a nice round multiple of kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 463
Quoted 7 lines
kernel/time.c +1
-1@@ -457,7 +458,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ; -return m * (HZ /MSEC_PER_SEC ); + r = m * (HZ /MSEC_PER_SEC );For this case the
jiffies_to_msec()check should be sufficient.kernel/time.c unsigned long msecs_to_jiffies(const unsigned int · line 472
Quoted 12 lines
(quoted diff) +0
-0#elsekernel/time.c +0
-0/* * Generic case - multiply, round and divide. But first @@ -467,9 +468,10 @@ unsigned long msecs_to_jiffies(const unsigned int m)if (HZ >MSEC_PER_SEC && m >jiffies_to_msecs (MAX_JIFFY_OFFSET ))return MAX_JIFFY_OFFSET ;Hmm, this check is silly. MUL32 is chosen, so that we cannot overflow.
kernel/time.c unsigned long msecs_to_jiffies(const unsigned int m) · line 477
Quoted 3 lines
kernel/time.c +1
-1-return (MSEC_TO_HZ_MUL32 * m +MSEC_TO_HZ_ADJ32 ) + r = (MSEC_TO_HZ_MUL32 * m +MSEC_TO_HZ_ADJ32 )>>
MSEC_TO_HZ_SHR32;Quoted 7 lines
kernel/time.c +1
-0#endif + return min_t(unsigned long, r, MAX_JIFFY_OFFSET);}
EXPORT_SYMBOL(msecs_to_jiffies);I start to wonder whether we really need these three variants or whether we just could go with that MUL/SHIFT based implementation and a final check for
MAX_JIFFY_OFFSET. That would boil down to:unsigned long
msecs_to_jiffies(const unsigned int m)
Quoted 8 lines
{ u64 res = (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32) >> MSEC_TO_HZ_SHR32; return min_t(unsigned long, (unsigned long)res, MAX_JIFFY_OFFSET); }That'd avoid the whole division and
msecs_to_jiffies()is not really a high precision function.
Ping? Is anyone going to either take my patch or do it's own reworking of that function?
Eike