[PATCH] binary or instead of logical in timer sync

[PATCH] binary or instead of logical in timer sync

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

Hi, I found this little bug and here's a patch.

The reading of a timer is determined if 1. the hypervisor is not 
currently updating it (where it sets the LSB of the version) or 2. the 
kernel didn't finish reading it before the hypervisor updated it (the 
kernel version doesn't match the hypervisor version).

But the current code doesn't test the above case. Instead, by using a 
binary or instead of a logical one, it would only repeat if the 
hypervisor was updating  __and__ we read the version before it started 
updating (or we read it before we started updating, but the hypervisor 
finished updating between the first part of the or and the second check).


I'm not use to thunderbird so I'm sending both an attachment and an 
inline cut and paste.

-- Steve

Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff -r 9632ececc8f4 linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c
--- a/linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c	Wed Aug 02 10:13:30 2006 +0100
+++ b/linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c	Wed Aug 02 22:54:09 2006 -0400
@@ -292,7 +292,7 @@ static void update_wallclock(void)
 		shadow_tv.tv_sec  = s->wc_sec;
 		shadow_tv.tv_nsec = s->wc_nsec;
 		rmb();
-	} while ((s->wc_version & 1) | (shadow_tv_version ^ s->wc_version));
+	} while ((s->wc_version & 1) || (shadow_tv_version ^ s->wc_version));
 
 	if (!independent_wallclock)
 		__update_wallclock(shadow_tv.tv_sec, shadow_tv.tv_nsec);
@@ -319,7 +319,7 @@ static void get_time_values_from_xen(voi
 		dst->tsc_to_nsec_mul   = src->tsc_to_system_mul;
 		dst->tsc_shift         = src->tsc_shift;
 		rmb();
-	} while ((src->version & 1) | (dst->version ^ src->version));
+	} while ((src->version & 1) || (dst->version ^ src->version));
 
 	dst->tsc_to_usec_mul = dst->tsc_to_nsec_mul / 1000;
 }



[-- Attachment #2: timer-bug.patch --]
[-- Type: text/x-patch, Size: 966 bytes --]

diff -r 9632ececc8f4 linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c
--- a/linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c	Wed Aug 02 10:13:30 2006 +0100
+++ b/linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c	Wed Aug 02 22:54:09 2006 -0400
@@ -292,7 +292,7 @@ static void update_wallclock(void)
 		shadow_tv.tv_sec  = s->wc_sec;
 		shadow_tv.tv_nsec = s->wc_nsec;
 		rmb();
-	} while ((s->wc_version & 1) | (shadow_tv_version ^ s->wc_version));
+	} while ((s->wc_version & 1) || (shadow_tv_version ^ s->wc_version));
 
 	if (!independent_wallclock)
 		__update_wallclock(shadow_tv.tv_sec, shadow_tv.tv_nsec);
@@ -319,7 +319,7 @@ static void get_time_values_from_xen(voi
 		dst->tsc_to_nsec_mul   = src->tsc_to_system_mul;
 		dst->tsc_shift         = src->tsc_shift;
 		rmb();
-	} while ((src->version & 1) | (dst->version ^ src->version));
+	} while ((src->version & 1) || (dst->version ^ src->version));
 
 	dst->tsc_to_usec_mul = dst->tsc_to_nsec_mul / 1000;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] binary or instead of logical in timer sync

[Keir Fraser]

On 3 Aug 2006, at 04:17, Steven Rostedt wrote:

> The reading of a timer is determined if 1. the hypervisor is not 
> currently updating it (where it sets the LSB of the version) or 2. the 
> kernel didn't finish reading it before the hypervisor updated it (the 
> kernel version doesn't match the hypervisor version).
>
> But the current code doesn't test the above case. Instead, by using a 
> binary or instead of a logical one, it would only repeat if the 
> hypervisor was updating  __and__ we read the version before it started 
> updating (or we read it before we started updating, but the hypervisor 
> finished updating between the first part of the or and the second 
> check).

I don't believe there is a bug here. Are you suggesting that the binary 
or, used within a logical predicate, behaves as a logical and? That 
doesn't make sense.

The only reason for using binary operators in those predicates is to 
avoid extra branches in the generated code which would probably be 
generated to follow the short-circuiting semantics of the logical 
operators. In fact, I think a smart optimising compiler would generate 
the *same* object code regardless of whether we use binary/logical or 
(but I don't believe gcc is that smart yet!).

  -- Keir

Re: [PATCH] binary or instead of logical in timer sync

[Steven Rostedt]

Keir Fraser wrote:
>
> On 3 Aug 2006, at 04:17, Steven Rostedt wrote:
>
>> The reading of a timer is determined if 1. the hypervisor is not 
>> currently updating it (where it sets the LSB of the version) or 2. 
>> the kernel didn't finish reading it before the hypervisor updated it 
>> (the kernel version doesn't match the hypervisor version).
>>
>> But the current code doesn't test the above case. Instead, by using a 
>> binary or instead of a logical one, it would only repeat if the 
>> hypervisor was updating  __and__ we read the version before it 
>> started updating (or we read it before we started updating, but the 
>> hypervisor finished updating between the first part of the or and the 
>> second check).
>
> I don't believe there is a bug here. Are you suggesting that the 
> binary or, used within a logical predicate, behaves as a logical and? 
> That doesn't make sense.

Crap, you're right.  I was debugging a problem with a bad timer, and saw 
that a binary or was being used for a logical case and just assumed that 
it was a bug.  Since it is common to see bugs like this using  & instead 
of &&.  So being late (and very hot here) I jumped the gun and posted 
the patch.

>
> The only reason for using binary operators in those predicates is to 
> avoid extra branches in the generated code which would probably be 
> generated to follow the short-circuiting semantics of the logical 
> operators. In fact, I think a smart optimising compiler would generate 
> the *same* object code regardless of whether we use binary/logical or 
> (but I don't believe gcc is that smart yet!).
>  

With some sleep behind me I see your point.  You're using the binary or 
to let the math determine the branch instead of logical jumps.

Makes sense,  sorry for the noise.

Hmm, perhaps a comment there is in order so another tired, hot and 
sticky programmer doesn't make the same mistake as I did :(


-- Steve

Re: Re: [PATCH] binary or instead of logical in timer sync

[Markus Armbruster]

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:

[...]
> The only reason for using binary operators in those predicates is to
> avoid extra branches in the generated code which would probably be
> generated to follow the short-circuiting semantics of the logical
> operators. In fact, I think a smart optimising compiler would generate
> the *same* object code regardless of whether we use binary/logical or
> (but I don't believe gcc is that smart yet!).

Manual optimizations like use of bitwise rather than logical
operations may speed up the program (depending on how dumb or confused
the optimizer is), but they certainly slow down the poor maintenance
programmer.  Bit-wise where I expect logical makes me hesistate and
check, because it's an unusual pattern, and in my experience often
wrong.

As with all optimizations that uglify the code, use it only when you
*know* it actually optimizes something worth optimizing.  Knowing
requires measuring.

Okay, I'll step off my soapbox now.