It's possible that the values used in and returned from jiffies_to_usecs() are
incorrect because of truncation when variables of type u64 are involved. So a
function specific to that type is used instead.
Diff'd against: linux/kernel/git/stable/linux-2.6.23.y.git
Signed-off-by: Jonathan Lim <[email protected]>
--- a/kernel/tsacct.c 2007-12-28 11:58:05.182065029 -0800
+++ b/kernel/tsacct.c 2007-12-28 11:57:37.949013675 -0800
@@ -71,6 +71,17 @@ void bacct_add_tsk(struct taskstats *sta
#ifdef CONFIG_TASK_XACCT
+static inline u64 jiffies_to_usecs_u64(const u64 j)
+{
+#if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
+ return (USEC_PER_SEC / HZ) * j;
+#elif HZ > USEC_PER_SEC && !(HZ % USEC_PER_SEC)
+ return (j + (HZ / USEC_PER_SEC) - 1)/(HZ / USEC_PER_SEC);
+#else
+ return (j * USEC_PER_SEC) / HZ;
+#endif
+}
+
#define KB 1024
#define MB (1024*KB)
/*
@@ -81,8 +92,8 @@ void xacct_add_tsk(struct taskstats *sta
struct mm_struct *mm;
/* convert pages-jiffies to Mbyte-usec */
- stats->coremem = jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
- stats->virtmem = jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
+ stats->coremem = jiffies_to_usecs_u64(p->acct_rss_mem1) * PAGE_SIZE / MB;
+ stats->virtmem = jiffies_to_usecs_u64(p->acct_vm_mem1) * PAGE_SIZE / MB;
mm = get_task_mm(p);
if (mm) {
/* adjust to KB unit */
Jonathan Lim wrote:
> It's possible that the values used in and returned from jiffies_to_usecs() are
> incorrect because of truncation when variables of type u64 are involved. So a
> function specific to that type is used instead.
Much worse than that. There are internal overflows in the conversions.
See the patch I recently submitted to -mm.
-hpa
On Fri, 28 Dec 2007 13:26:07 -0800 (PST) Jonathan Lim <[email protected]> wrote:
> It's possible that the values used in and returned from jiffies_to_usecs() are
> incorrect because of truncation when variables of type u64 are involved. So a
> function specific to that type is used instead.
>
> Diff'd against: linux/kernel/git/stable/linux-2.6.23.y.git
>
> Signed-off-by: Jonathan Lim <[email protected]>
>
> --- a/kernel/tsacct.c 2007-12-28 11:58:05.182065029 -0800
> +++ b/kernel/tsacct.c 2007-12-28 11:57:37.949013675 -0800
> @@ -71,6 +71,17 @@ void bacct_add_tsk(struct taskstats *sta
>
> #ifdef CONFIG_TASK_XACCT
>
> +static inline u64 jiffies_to_usecs_u64(const u64 j)
> +{
> +#if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
> + return (USEC_PER_SEC / HZ) * j;
> +#elif HZ > USEC_PER_SEC && !(HZ % USEC_PER_SEC)
> + return (j + (HZ / USEC_PER_SEC) - 1)/(HZ / USEC_PER_SEC);
> +#else
> + return (j * USEC_PER_SEC) / HZ;
> +#endif
> +}
> +
> #define KB 1024
> #define MB (1024*KB)
> /*
> @@ -81,8 +92,8 @@ void xacct_add_tsk(struct taskstats *sta
> struct mm_struct *mm;
>
> /* convert pages-jiffies to Mbyte-usec */
> - stats->coremem = jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
> - stats->virtmem = jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
> + stats->coremem = jiffies_to_usecs_u64(p->acct_rss_mem1) * PAGE_SIZE / MB;
> + stats->virtmem = jiffies_to_usecs_u64(p->acct_vm_mem1) * PAGE_SIZE / MB;
> mm = get_task_mm(p);
> if (mm) {
> /* adjust to KB unit */
Fair enough. But I guess that new function should be a kernel-wide thing
because surely other users will turn up.
Peter has been working on the accuracy of some of these conversion
functions and might need to know about this change?
Andrew Morton wrote:
> On Fri, 28 Dec 2007 13:26:07 -0800 (PST) Jonathan Lim <[email protected]> wrote:
>
>> It's possible that the values used in and returned from jiffies_to_usecs() are
>> incorrect because of truncation when variables of type u64 are involved. So a
>> function specific to that type is used instead.
>>
>> Diff'd against: linux/kernel/git/stable/linux-2.6.23.y.git
>>
>> Signed-off-by: Jonathan Lim <[email protected]>
>>
>> --- a/kernel/tsacct.c 2007-12-28 11:58:05.182065029 -0800
>> +++ b/kernel/tsacct.c 2007-12-28 11:57:37.949013675 -0800
>> @@ -71,6 +71,17 @@ void bacct_add_tsk(struct taskstats *sta
>>
>> #ifdef CONFIG_TASK_XACCT
>>
>> +static inline u64 jiffies_to_usecs_u64(const u64 j)
>> +{
>> +#if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
>> + return (USEC_PER_SEC / HZ) * j;
>> +#elif HZ > USEC_PER_SEC && !(HZ % USEC_PER_SEC)
>> + return (j + (HZ / USEC_PER_SEC) - 1)/(HZ / USEC_PER_SEC);
>> +#else
>> + return (j * USEC_PER_SEC) / HZ;
>> +#endif
>> +}
>> +
>> #define KB 1024
>> #define MB (1024*KB)
>> /*
>> @@ -81,8 +92,8 @@ void xacct_add_tsk(struct taskstats *sta
>> struct mm_struct *mm;
>>
>> /* convert pages-jiffies to Mbyte-usec */
>> - stats->coremem = jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
>> - stats->virtmem = jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
>> + stats->coremem = jiffies_to_usecs_u64(p->acct_rss_mem1) * PAGE_SIZE / MB;
>> + stats->virtmem = jiffies_to_usecs_u64(p->acct_vm_mem1) * PAGE_SIZE / MB;
>> mm = get_task_mm(p);
>> if (mm) {
>> /* adjust to KB unit */
>
> Fair enough. But I guess that new function should be a kernel-wide thing
> because surely other users will turn up.
>
> Peter has been working on the accuracy of some of these conversion
> functions and might need to know about this change?
Yes, the function should be coded using the new #defines produced by
timeconst.h; that way you end up avoiding a possible overflow in the
multiplication.
I believe all three cases can be folded, then, to:
return (j*HZ_TO_USEC_NUM + HZ_TO_USEC_DEN-1) / HZ_TO_USEC_DEN;
I would also like to observe that the roundoff behaviour of the function
above is inconsistent; in case 2 it will round up, but in case 3 it
will round down. The line proposed above has round up behaviour.
-hpa
On Wed Jan 2 16:36:47 2008, [email protected] wrote:
>
> Andrew Morton wrote:
> > On Fri, 28 Dec 2007 13:26:07 -0800 (PST) Jonathan Lim <[email protected]> wrote:
> >
> >> It's possible that the values used in and returned from jiffies_to_usecs()
> >> are incorrect because of truncation when variables of type u64 are
> >> involved. So a function specific to that type is used instead.
> >>
> >> Diff'd against: linux/kernel/git/stable/linux-2.6.23.y.git
> >>
> >> Signed-off-by: Jonathan Lim <[email protected]>
> >>
> >> --- a/kernel/tsacct.c 2007-12-28 11:58:05.182065029 -0800
> >> +++ b/kernel/tsacct.c 2007-12-28 11:57:37.949013675 -0800
> >> @@ -71,6 +71,17 @@ void bacct_add_tsk(struct taskstats *sta
> >>
> >> #ifdef CONFIG_TASK_XACCT
> >>
> >> +static inline u64 jiffies_to_usecs_u64(const u64 j)
> >> +{
> >> +#if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
> >> + return (USEC_PER_SEC / HZ) * j;
> >> +#elif HZ > USEC_PER_SEC && !(HZ % USEC_PER_SEC)
> >> + return (j + (HZ / USEC_PER_SEC) - 1)/(HZ / USEC_PER_SEC);
> >> +#else
> >> + return (j * USEC_PER_SEC) / HZ;
> >> +#endif
> >> +}
> >> +
> >> #define KB 1024
> >> #define MB (1024*KB)
> >> /*
> >> @@ -81,8 +92,8 @@ void xacct_add_tsk(struct taskstats *sta
> >> struct mm_struct *mm;
> >>
> >> /* convert pages-jiffies to Mbyte-usec */
> >> - stats->coremem = jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
> >> - stats->virtmem = jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
> >> + stats->coremem = jiffies_to_usecs_u64(p->acct_rss_mem1) * PAGE_SIZE / MB;
> >> + stats->virtmem = jiffies_to_usecs_u64(p->acct_vm_mem1) * PAGE_SIZE / MB;
> >> mm = get_task_mm(p);
> >> if (mm) {
> >> /* adjust to KB unit */
> >
> > Fair enough. But I guess that new function should be a kernel-wide thing
> > because surely other users will turn up.
> >
> > Peter has been working on the accuracy of some of these conversion
> > functions and might need to know about this change?
>
> Yes, the function should be coded using the new #defines produced by
> timeconst.h; that way you end up avoiding a possible overflow in the
> multiplication.
>
> I believe all three cases can be folded, then, to:
>
> return (j*HZ_TO_USEC_NUM + HZ_TO_USEC_DEN-1) / HZ_TO_USEC_DEN;
>
> I would also like to observe that the roundoff behaviour of the function
> above is inconsistent; in case 2 it will round up, but in case 3 it
> will round down. The line proposed above has round up behaviour.
>
> -hpa
Peter,
Would you be willing to include the u64 function as part of your patch to make
it available kernel-wide? It just needs:
u64 inline jiffies_to_usecs_u64(const u64 j)
and for the symbol to be exported. Thanks.
Jonathan
Jonathan Lim wrote:
>
> Peter,
>
> Would you be willing to include the u64 function as part of your patch to make
> it available kernel-wide? It just needs:
>
> u64 inline jiffies_to_usecs_u64(const u64 j)
>
> and for the symbol to be exported. Thanks.
>
It should be a separate patch (new functionality versus change of
implementation); I'd just do a small patch on top of mine.
-hpa
It's possible that the values used in and returned from jiffies_to_usecs() are
incorrect because of truncation when variables of type u64 are involved. So a
function specific to that type is used instead.
Updated from previous submission with feedback from Peter Anvin.
Diff'd against: linux/kernel/git/torvalds/linux-2.6.git
Signed-off-by: Jonathan Lim <[email protected]>
--- a/include/linux/jiffies.h 2008-01-07 16:09:17.737040981 -0800
+++ b/include/linux/jiffies.h 2008-01-07 16:09:17.693033003 -0800
@@ -36,7 +36,7 @@
/* LATCH is used in the interval timer and ftape setup. */
#define LATCH ((CLOCK_TICK_RATE + HZ/2) / HZ) /* For divider */
-/* Suppose we want to devide two numbers NOM and DEN: NOM/DEN, then we can
+/* Suppose we want to divide two numbers NOM and DEN: NOM/DEN, then we can
* improve accuracy by shifting LSH bits, hence calculating:
* (NOM << LSH) / DEN
* This however means trouble for large NOM, because (NOM << LSH) may no
@@ -198,7 +198,7 @@ extern unsigned long preset_lpj;
* operator if the result is a long long AND at least one of the
* operands is cast to long long (usually just prior to the "*" so as
* not to confuse it into thinking it really has a 64-bit operand,
- * which, buy the way, it can do, but it takes more code and at least 2
+ * which, by the way, it can do, but it takes more code and at least 2
* mpys).
* We also need to be aware that one second in nanoseconds is only a
@@ -263,6 +263,7 @@ extern unsigned long preset_lpj;
*/
extern unsigned int jiffies_to_msecs(const unsigned long j);
extern unsigned int jiffies_to_usecs(const unsigned long j);
+extern u64 jiffies_64_to_usecs(const u64 j);
extern unsigned long msecs_to_jiffies(const unsigned int m);
extern unsigned long usecs_to_jiffies(const unsigned int u);
extern unsigned long timespec_to_jiffies(const struct timespec *value);
--- a/kernel/time.c 2008-01-07 16:09:17.841059839 -0800
+++ b/kernel/time.c 2008-01-07 16:09:17.825056938 -0800
@@ -267,6 +267,12 @@ unsigned int inline jiffies_to_usecs(con
}
EXPORT_SYMBOL(jiffies_to_usecs);
+u64 inline jiffies_64_to_usecs(const u64 j)
+{
+ return (j*HZ_TO_USEC_NUM + HZ_TO_USEC_DEN-1) / HZ_TO_USEC_DEN;
+}
+EXPORT_SYMBOL(jiffies_64_to_usecs);
+
/**
* timespec_trunc - Truncate timespec to a granularity
* @t: Timespec
--- a/kernel/tsacct.c 2008-01-07 16:09:17.833058389 -0800
+++ b/kernel/tsacct.c 2008-01-07 16:09:17.793051136 -0800
@@ -85,8 +85,8 @@ void xacct_add_tsk(struct taskstats *sta
struct mm_struct *mm;
/* convert pages-jiffies to Mbyte-usec */
- stats->coremem = jiffies_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
- stats->virtmem = jiffies_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
+ stats->coremem = jiffies_64_to_usecs(p->acct_rss_mem1) * PAGE_SIZE / MB;
+ stats->virtmem = jiffies_64_to_usecs(p->acct_vm_mem1) * PAGE_SIZE / MB;
mm = get_task_mm(p);
if (mm) {
/* adjust to KB unit */