[ovs-dev] [PATCH 1/2] hash: Add hash_add64().

[Jarno Rajahalme]

On Dec 29, 2014, at 2:12 PM, Ben Pfaff <blp at 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 <jrajahalme at 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 <blp at nicira.com>

Thanks for the review!

I also fixed the 32-bit compile error.

Merged with master,

  Jarno