lkml.org 
[lkml]   [2017]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 1/2] xxHash: create arch dependent 32/64-bit xxhash()
2017-09-25 17:59 GMT+03:00 Matthew Wilcox <willy@infradead.org>:
> On Fri, Sep 22, 2017 at 02:18:17AM +0300, Timofey Titovets wrote:
>> diff --git a/include/linux/xxhash.h b/include/linux/xxhash.h
>> index 9e1f42cb57e9..195a0ae10e9b 100644
>> --- a/include/linux/xxhash.h
>> +++ b/include/linux/xxhash.h
>> @@ -76,6 +76,7 @@
>> #define XXHASH_H
>>
>> #include <linux/types.h>
>> +#include <linux/bitops.h> /* BITS_PER_LONG */
>>
>> /*-****************************
>> * Simple Hash Functions
>
> Huh? linux/types.h already brings in BITS_PER_LONG. Look:
>
> linux/types.h
> uapi/linux/types.h
> uapi/asm/types.h
> uapi/asm-generic/types.h
> uapi/asm-generic/int-ll64.h
> asm/bitsperlong.h

Will fix that, thanks.

>> @@ -107,6 +108,29 @@ uint32_t xxh32(const void *input, size_t length, uint32_t seed);
>> */
>> uint64_t xxh64(const void *input, size_t length, uint64_t seed);
>>
>> +#if BITS_PER_LONG == 64
>> +typedef u64 xxhash_t;
>> +#else
>> +typedef u32 xxhash_t;
>> +#endif
>
> This is a funny way to spell 'unsigned long' ...

i'm just want some strict and obvious types for in memory hashing.
And that just looks pretty for my eye (IMHO),
I will replace that with 'unsigned long' of course and drop xxhash_t completely,
as you find that unacceptable.

>> +/**
>> + * xxhash() - calculate 32/64-bit hash based on cpu word size
>> + *
>> + * @input: The data to hash.
>> + * @length: The length of the data to hash.
>> + * @seed: The seed can be used to alter the result predictably.
>> + *
>> + * This function always work as xxh32() for 32-bit systems
>> + * and as xxh64() for 64-bit systems.
>> + * Because result depends on cpu work size,
>> + * the main proporse of that function is for in memory hashing.
>> + *
>> + * Return: 32/64-bit hash of the data.
>> + */
>> +
>
>> +xxhash_t xxhash(const void *input, size_t length, uint64_t seed)
>> +{
>> +#if BITS_PER_LONG == 64
>> + return xxh64(input, length, seed);
>> +#else
>> + return xxh32(input, length, seed);
>> +#endif
>> +}
>
> Let's move that to the header file and make it a static inline. That way
> it doesn't need to be an EXPORT_SYMBOL.

Agreed, thanks.

> Also, I think the kerneldoc could do with a bit of work. Try this:
>
> /**
> * xxhash() - calculate wordsize hash of the input with a given seed
> * @input: The data to hash.
> * @length: The length of the data to hash.
> * @seed: The seed can be used to alter the result predictably.
> *
> * If the hash does not need to be comparable between machines with
> * different word sizes, this function will call whichever of xxh32()
> * or xxh64() is faster.
> *
> * Return: wordsize hash of the data.
> */

Replace with your version, thanks.

> static inline
> unsigned long xxhash(const void *input, size_t length, unsigned long seed)
> {
> #if BITS_PER_LONG == 64
> return xxh64(input, length, seed);
> #else
> return xxh32(input, length, seed);
> #endif
> }


--
Have a nice day,
Timofey.

\
 
 \ /
  Last update: 2017-09-25 18:19    [W:0.056 / U:15.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site