Re: [ovs-dev] [PATCH 3/5] test-hash: Don't check bit 2048.

[Joe Stringer]

On 25 May 2017 at 13:55, Ben Pfaff  wrote:
> On Thu, May 25, 2017 at 01:08:21PM -0700, Joe Stringer wrote:
>> On 25 May 2017 at 10:36, Ben Pfaff  wrote:
>> > On Tue, May 23, 2017 at 04:02:14PM -0700, Joe Stringer wrote:
>> >> When running 256B hash check, we currently iterate from 0 up to and
>> >> including bit 2048, which is beyond the range of bits that 256B holds.
>> >> For bit 2048, set_bit128() doesn't set a bit due to the range check.
>> >> Simplify the code by dropping the handling of bit 2048.
>> >>
>> >> Signed-off-by: Joe Stringer 
>> >
>> > Hmm, weird code.
>> >
>> > Looking at check_word_hash(), I think the goal here is to test that the
>> > hash of all-bits-0 is different from the hash for any single bit being
>> > set.  That does seem like a valuable check.  Do you think that there is
>> > a better way to still accomplish that goal for the larger cases?
>>
>> I think that the above is part of it, but it's also validating that
>> for murmurhash operating on 64-bit chunks at a time, it doesn't make a
>> difference whether the input data is 64-bit aligned or not.
>
> Right; I just meant the reason why it originally went one-past-the-end.

Oh, I think that was just an oversight.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/5] test-hash: Don't check bit 2048.

[Ben Pfaff]

On Thu, May 25, 2017 at 01:08:21PM -0700, Joe Stringer wrote:
> On 25 May 2017 at 10:36, Ben Pfaff  wrote:
> > On Tue, May 23, 2017 at 04:02:14PM -0700, Joe Stringer wrote:
> >> When running 256B hash check, we currently iterate from 0 up to and
> >> including bit 2048, which is beyond the range of bits that 256B holds.
> >> For bit 2048, set_bit128() doesn't set a bit due to the range check.
> >> Simplify the code by dropping the handling of bit 2048.
> >>
> >> Signed-off-by: Joe Stringer 
> >
> > Hmm, weird code.
> >
> > Looking at check_word_hash(), I think the goal here is to test that the
> > hash of all-bits-0 is different from the hash for any single bit being
> > set.  That does seem like a valuable check.  Do you think that there is
> > a better way to still accomplish that goal for the larger cases?
> 
> I think that the above is part of it, but it's also validating that
> for murmurhash operating on 64-bit chunks at a time, it doesn't make a
> difference whether the input data is 64-bit aligned or not.

Right; I just meant the reason why it originally went one-past-the-end.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/5] test-hash: Don't check bit 2048.

[Joe Stringer]

On 25 May 2017 at 10:36, Ben Pfaff  wrote:
> On Tue, May 23, 2017 at 04:02:14PM -0700, Joe Stringer wrote:
>> When running 256B hash check, we currently iterate from 0 up to and
>> including bit 2048, which is beyond the range of bits that 256B holds.
>> For bit 2048, set_bit128() doesn't set a bit due to the range check.
>> Simplify the code by dropping the handling of bit 2048.
>>
>> Signed-off-by: Joe Stringer 
>
> Hmm, weird code.
>
> Looking at check_word_hash(), I think the goal here is to test that the
> hash of all-bits-0 is different from the hash for any single bit being
> set.  That does seem like a valuable check.  Do you think that there is
> a better way to still accomplish that goal for the larger cases?

I think that the above is part of it, but it's also validating that
for murmurhash operating on 64-bit chunks at a time, it doesn't make a
difference whether the input data is 64-bit aligned or not.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/5] test-hash: Don't check bit 2048.

[Ben Pfaff]

On Tue, May 23, 2017 at 04:02:14PM -0700, Joe Stringer wrote:
> When running 256B hash check, we currently iterate from 0 up to and
> including bit 2048, which is beyond the range of bits that 256B holds.
> For bit 2048, set_bit128() doesn't set a bit due to the range check.
> Simplify the code by dropping the handling of bit 2048.
> 
> Signed-off-by: Joe Stringer 

Hmm, weird code.

Looking at check_word_hash(), I think the goal here is to test that the
hash of all-bits-0 is different from the hash for any single bit being
set.  That does seem like a valuable check.  Do you think that there is
a better way to still accomplish that goal for the larger cases?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 3/5] test-hash: Don't check bit 2048.

[Joe Stringer]

When running 256B hash check, we currently iterate from 0 up to and
including bit 2048, which is beyond the range of bits that 256B holds.
For bit 2048, set_bit128() doesn't set a bit due to the range check.
Simplify the code by dropping the handling of bit 2048.

Signed-off-by: Joe Stringer 
---
 tests/test-hash.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tests/test-hash.c b/tests/test-hash.c
index d1beead36ed5..a20c87fad6a0 100644
--- a/tests/test-hash.c
+++ b/tests/test-hash.c
@@ -39,16 +39,15 @@ set_bit(uint32_t array[3], int bit)
 static void
 set_bit128(ovs_u128 *values, int bit, int n_bits)
 {
-assert(bit >= 0 && bit <= 2048);
+int b = bit % 128;
+
+assert(bit >= 0 && bit < 2048);
 memset(values, 0, n_bits/8);
-if (bit < n_bits) {
-int b = bit % 128;
 
-if (b < 64) {
-values[bit / 128].u64.lo = UINT64_C(1) << (b % 64);
-} else {
-values[bit / 128].u64.hi = UINT64_C(1) << (b % 64);
-}
+if (b < 64) {
+values[bit / 128].u64.lo = UINT64_C(1) << (b % 64);
+} else {
+values[bit / 128].u64.hi = UINT64_C(1) << (b % 64);
 }
 }
 
@@ -149,7 +148,7 @@ check_hash_bytes128(void (*hash)(const void *, size_t, 
uint32_t, ovs_u128 *),
 const int n_bits = sizeof(ovs_u128) * 8;
 int i, j;
 
-for (i = 0; i <= n_bits; i++) {
+for (i = 0; i < n_bits; i++) {
 OVS_PACKED(struct offset_ovs_u128 {
 uint32_t a;
 ovs_u128 b;
@@ -168,7 +167,7 @@ check_hash_bytes128(void (*hash)(const void *, size_t, 
uint32_t, ovs_u128 *),
name, out0.u64.lo, out0.u64.hi, out1.u64.lo, out1.u64.hi);
 }
 
-for (j = i + 1; j <= n_bits; j++) {
+for (j = i + 1; j < n_bits; j++) {
 ovs_u128 in2;
 ovs_u128 out2;
 int ofs;
@@ -201,7 +200,7 @@ check_256byte_hash(void (*hash)(const void *, size_t, 
uint32_t, ovs_u128 *),
 const int n_bits = sizeof(ovs_u128) * 8 * 16;
 int i, j;
 
-for (i = 0; i <= n_bits; i++) {
+for (i = 0; i < n_bits; i++) {
 OVS_PACKED(struct offset_ovs_u128 {
 uint32_t a;
 ovs_u128 b[16];
@@ -220,7 +219,7 @@ check_256byte_hash(void (*hash)(const void *, size_t, 
uint32_t, ovs_u128 *),
name, out0.u64.lo, out0.u64.hi, out1.u64.lo, out1.u64.hi);
 }
 
-for (j = i + 1; j <= n_bits; j++) {
+for (j = i + 1; j < n_bits; j++) {
 ovs_u128 in2[16];
 ovs_u128 out2;
 
-- 
2.12.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev