| Message ID | 20170523230216.29696-4-joe@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
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 <joe@ovn.org> 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?
On 25 May 2017 at 10:36, Ben Pfaff <blp@ovn.org> 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 <joe@ovn.org> > > 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.
On Thu, May 25, 2017 at 01:08:21PM -0700, Joe Stringer wrote: > On 25 May 2017 at 10:36, Ben Pfaff <blp@ovn.org> 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 <joe@ovn.org> > > > > 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.
On 25 May 2017 at 13:55, Ben Pfaff <blp@ovn.org> wrote: > On Thu, May 25, 2017 at 01:08:21PM -0700, Joe Stringer wrote: >> On 25 May 2017 at 10:36, Ben Pfaff <blp@ovn.org> 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 <joe@ovn.org> >> > >> > 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.
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;
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 <joe@ovn.org> --- tests/test-hash.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)