On Dec 29, 2014, at 2:12 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Wed, Dec 17, 2014 at 10:30:41AM -0800, Jarno Rajahalme wrote:
>> Add support for adding 64-bit words to hashes. This will be used by
>> subsequent patches.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> I noticed that hash_words64() takes a 64-bit basis (which is not new).
> That seems odd because it returns a 32-bit hash value. Usually the
> basis is used for chaining together the hashes of several values, so
> usually I expect the basis to be the same size as the hash (both
> either 32 or 64 bits, for example).
>
> Assuming that a 64-bit basis does make sense, would you mind adding
> parentheses here in hash_words64_inline():
>> + hash = basis ^ basis >> 32;
> so that it becomes:
> hash = basis ^ (basis >> 32);
>
> Also, again assuming that a 64-bit basis does makes sense, if the
> provided basis is really a hash, then a plain XOR as above should be
> OK, but if it's some other value provided by the caller, without
> necessarily high or evenly distributed entropy, as we occasionally
> use, then a better hash function than XOR might be useful.
>
I recall that I planned to use the 64-bit miniflow map as a basis value, hence
the uint64_t. But you are right in that it is weird to have a 64-bit initial
hash value when the range of the hash function is 32 bits, so I changed it to
32-bits in a separate patch.
>> + for (i = 0; i < n_words; i++) {
>> + hash = hash_add64(hash, p[i]);
>> + }
>> + return hash_finish(hash, n_words * 8);
>> }
>
> Other than that philosophical issue:
> Acked-by: Ben Pfaff <b...@nicira.com>
Thanks for the review!
I also fixed the 32-bit compile error.
Merged with master,
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
