binary or instead of logical in timer sync

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

diff -r 9632ececc8f4 linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c
@@ -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;
}

diff -r 9632ececc8f4 linux-2.6-xen-sparse/arch/i386/kernel/time-xen.c
@@ -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;
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

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

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.

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

Keir Fraser <[email protected]> writes:

[...]

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.