Xen devel - Aug 2008 - [PATCH RFC] do_settime is backwards?!

While digging through the time code, I found something very strange
in do_settime:

    x = (secs * 1000000000ULL) + (u64)nsecs - system_time_base;
    y = do_div(x, 1000000000);

    spin_lock(&wc_lock);
    wc_sec  = _wc_sec  = (u32)x;
    wc_nsec = _wc_nsec = (u32)y;
    spin_unlock(&wc_lock);

The value "x" appears to be the number of nanoseconds, while
the value "y" is the value in seconds.  The assignments to 
wc_sec and wc_nsec seem backwards, though...

I hope I''ve overlooked some detail, but just in case I am
right here''s a patch to reverse the assignments.

How did this ever work?

Signed-off-by: Rik van Riel <riel@redhat.com>
--- 

diff -up xen/arch/x86/time.c.backwards xen/arch/x86/time.c
--- xen/arch/x86/time.c.backwards	2008-08-06 17:33:26.000000000 -0400
+++ xen/arch/x86/time.c	2008-08-06 17:33:45.000000000 -0400
@@ -823,8 +823,8 @@ void do_settime(unsigned long secs, unsi
     y = do_div(x, 1000000000);
 
     spin_lock(&wc_lock);
-    wc_sec  = _wc_sec  = (u32)x;
-    wc_nsec = _wc_nsec = (u32)y;
+    wc_sec  = _wc_sec  = (u32)y;
+    wc_nsec = _wc_nsec = (u32)x;
     spin_unlock(&wc_lock);
 
     rcu_read_lock(&domlist_read_lock);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

This patch fixes an apparent bug in do_settime() that was found while
auditing the code to fix an unrelated bug.

Signed-off-by: Rik van Riel <riel@redhat.com>
--- 
I still hope I am wrong :)

On Wed, 6 Aug 2008 17:37:23 -0400
Rik van Riel <riel@redhat.com> wrote:
> I hope I''ve overlooked some detail, but just in case I am
> right here''s a patch to reverse the assignments.
> 
> How did this ever work?
Of course, that is still not enough.  A u32 has some
more bits than a billion, so the wc_nsec variable
could be off by as much as 3 seconds...


diff -up xen/arch/x86/time.c.backwards xen/arch/x86/time.c
--- xen/arch/x86/time.c.backwards	2008-08-06 17:33:26.000000000 -0400
+++ xen/arch/x86/time.c	2008-08-06 17:43:46.000000000 -0400
@@ -819,12 +819,16 @@ void do_settime(unsigned long secs, unsi
     u32 y, _wc_sec, _wc_nsec;
     struct domain *d;
 
+    /* Calculate nanoseconds */
     x = (secs * 1000000000ULL) + (u64)nsecs - system_time_base;
+    /* Calculate seconds. */
     y = do_div(x, 1000000000);
+    /* Leave the remainder for the nanosecond field. */
+    x -= (y * 1000000000);
 
     spin_lock(&wc_lock);
-    wc_sec  = _wc_sec  = (u32)x;
-    wc_nsec = _wc_nsec = (u32)y;
+    wc_sec  = _wc_sec  = (u32)y;
+    wc_nsec = _wc_nsec = (u32)x;
     spin_unlock(&wc_lock);
 
     rcu_read_lock(&domlist_read_lock);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Check out the definition of do_div which works strangely.
I was confused by that also, but the code in question
does work (I think!).
> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com]On Behalf Of 
> Rik van Riel
> Sent: Wednesday, August 06, 2008 3:47 PM
> To: Rik van Riel
> Cc: xen-devel@lists.xensource.com
> Subject: [Xen-devel] Re: [PATCH RFC] do_settime is backwards?!
> 
> 
> This patch fixes an apparent bug in do_settime() that was found while
> auditing the code to fix an unrelated bug.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
> I still hope I am wrong :)
> 
> On Wed, 6 Aug 2008 17:37:23 -0400
> Rik van Riel <riel@redhat.com> wrote:
> 
> > I hope I''ve overlooked some detail, but just in case I am
> > right here''s a patch to reverse the assignments.
> >
> > How did this ever work?
> 
> Of course, that is still not enough.  A u32 has some
> more bits than a billion, so the wc_nsec variable
> could be off by as much as 3 seconds...
> 
> 
> diff -up xen/arch/x86/time.c.backwards xen/arch/x86/time.c
> --- xen/arch/x86/time.c.backwards	2008-08-06 
> 17:33:26.000000000 -0400
> +++ xen/arch/x86/time.c	2008-08-06 17:43:46.000000000 -0400
> @@ -819,12 +819,16 @@ void do_settime(unsigned long secs, unsi
>      u32 y, _wc_sec, _wc_nsec;
>      struct domain *d;
>  
> +    /* Calculate nanoseconds */
>      x = (secs * 1000000000ULL) + (u64)nsecs - system_time_base;
> +    /* Calculate seconds. */
>      y = do_div(x, 1000000000);
> +    /* Leave the remainder for the nanosecond field. */
> +    x -= (y * 1000000000);
>  
>      spin_lock(&wc_lock);
> -    wc_sec  = _wc_sec  = (u32)x;
> -    wc_nsec = _wc_nsec = (u32)y;
> +    wc_sec  = _wc_sec  = (u32)y;
> +    wc_nsec = _wc_nsec = (u32)x;
>      spin_unlock(&wc_lock);
>  
>      rcu_read_lock(&domlist_read_lock);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 6 Aug 2008 15:51:02 -0600
"Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:
> Check out the definition of do_div which works strangely.
> I was confused by that also, but the code in question
> does work (I think!).
OK, so the original code of:

    x = (secs * 1000000000ULL) + (u64)nsecs - system_time_base;
    y = do_div(x, 1000000000);

Leaves "x" as the number of seconds and "y" as the
remainder, in nanoseconds.

Good to know that I overlooked something and the
original code was right after all!

-- 
All rights reversed.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel