[dpdk-dev] [PATCH v4 0/6] update jhash function
On Tue, May 12, 2015 at 12:02:32PM +0100, Pablo de Lara wrote:
> Jenkins hash function was developed originally in 1996,
> and was integrated in first versions of DPDK.
> The function has been improved in 2006,
> achieving up to 60% better performance, compared to the original one.
>
> This patchset updates the current jhash in DPDK,
> including two new functions that generate two hashes from a single key.
>
> It also separates the existing hash function performance tests to
> another file, to make it quicker to run.
>
> changes in v4:
> - Simplify key alignment checks
> - Include missing x86 arch check
>
> changes in v3:
>
> - Update rte_jhash_1word, rte_jhash_2words and rte_jhash_3words
> functions
>
> changes in v2:
>
> - Split single commit in three commits, one that updates the existing
> functions
> and another that adds two new functions and use one of those functions
> as a base to be called by the other ones.
> - Remove some unnecessary ifdefs in the code.
> - Add new macros to help on the reutilization of constants
> - Separate hash function performance tests to another file
> and improve cycle measurements.
> - Rename existing function rte_jhash2 to rte_jhash_32b
> (something more meaninful) and mark rte_jhash2 as
> deprecated
>
Hi Pablo,
Patchset looks good to me, and unit tests all pass across the set. Some general
comments or suggestions though - particularly about testing.
1. The set of lengths used when testing the functions looks strange and rather
arbitrary. Perhaps we could have a set of key lengths which are documented. E.g.
lengths[] = {
4, 8, 16, 48, 64, /* standard key sizes */
9,/* IPv4 SRC + DST + protocol, unpadded */
13, /* IPv4 5-tuple, unpadded */
37, /* IPv6 5-tuple, unpadded */
40, /* IPv6 5-tuple, padded to 8-byte boundary */
}
2. When testing multiple algorithms, it might be nice to change the order of the
loops so that we test all algorithms with the same key lengths first, and then
change length, rather than running the same algorithm with multiple lengths and
then changing algorithm. The output would be clearer and easier to see which
algorithm performs best for a given key-length.
3. For sanity checking across the patches making changes to the jhash functions,
I think it would be nice to have an initial sanity test with a set of known
keys and hash results in it. That way we can verify that the actual calculation
result never changes as the functions are modified. This would also be a big
help for future work changing the code. [As far as I can see, we don't ever
check
in the algorithm checks that we are ever getting the right answer :-)]
All the above suggestions could perhaps go in a patch (or 2/3 patches) after the
first two, which splits out the algorithm tests, and before the actual changes
to the jhash implementation.
Regards,
/Bruce
[dpdk-dev] [PATCH v4 0/6] update jhash function
Hi Neil, > -Original Message- > From: Neil Horman [mailto:nhorman at tuxdriver.com] > Sent: Tuesday, May 12, 2015 4:33 PM > To: De Lara Guarch, Pablo > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 0/6] update jhash function > > On Tue, May 12, 2015 at 12:02:32PM +0100, Pablo de Lara wrote: > > Jenkins hash function was developed originally in 1996, > > and was integrated in first versions of DPDK. > > The function has been improved in 2006, > > achieving up to 60% better performance, compared to the original one. > > > > This patchset updates the current jhash in DPDK, > > including two new functions that generate two hashes from a single key. > > > > It also separates the existing hash function performance tests to > > another file, to make it quicker to run. > > > > changes in v4: > > - Simplify key alignment checks > > - Include missing x86 arch check > > > > changes in v3: > > > > - Update rte_jhash_1word, rte_jhash_2words and rte_jhash_3words > > functions > > > > changes in v2: > > > > - Split single commit in three commits, one that updates the existing > functions > > and another that adds two new functions and use one of those functions > > as a base to be called by the other ones. > > - Remove some unnecessary ifdefs in the code. > > - Add new macros to help on the reutilization of constants > > - Separate hash function performance tests to another file > > and improve cycle measurements. > > - Rename existing function rte_jhash2 to rte_jhash_32b > > (something more meaninful) and mark rte_jhash2 as > > deprecated > > > > Pablo de Lara (6): > > test/hash: move hash function perf tests to separate file > > test/hash: improve accuracy on cycle measurements > > hash: update jhash function with the latest available > > hash: add two new functions to jhash library > > hash: remove duplicated code > > hash: rename rte_jhash2 to rte_jhash_32b > > > > app/test/Makefile |1 + > > app/test/test_func_reentrancy.c |2 +- > > app/test/test_hash.c|4 +- > > app/test/test_hash_func_perf.c | 145 + > > app/test/test_hash_perf.c | 71 + > > lib/librte_hash/rte_jhash.h | 338 +- > - > > 6 files changed, 402 insertions(+), 159 deletions(-) > > create mode 100644 app/test/test_hash_func_perf.c > > > > -- > > 1.7.4.1 > > > > > did you run this through the ABI checker? I see you're removing several > symbols > that will likely need to go through the ABI deprecation process. > > Neil I had not run it, but I just did. I see no problems on librte_hash (but I see some on rte_ethdev.h, due to another commit). Anyway, I renamed two functions to be more meaningful, but those functions are "static inline", so I am not sure exactly what the deprecation process is for those. What I did was leaving the original function that calls the same function as the new renamed one, but adds a line warning that the functions is deprecated. Is that OK or should I do it differently? Thanks! Pablo
[dpdk-dev] [PATCH v4 0/6] update jhash function
On Wed, May 13, 2015 at 01:52:33PM +, De Lara Guarch, Pablo wrote: > Hi Neil, > > > -Original Message- > > From: Neil Horman [mailto:nhorman at tuxdriver.com] > > Sent: Tuesday, May 12, 2015 4:33 PM > > To: De Lara Guarch, Pablo > > Cc: dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v4 0/6] update jhash function > > > > On Tue, May 12, 2015 at 12:02:32PM +0100, Pablo de Lara wrote: > > > Jenkins hash function was developed originally in 1996, > > > and was integrated in first versions of DPDK. > > > The function has been improved in 2006, > > > achieving up to 60% better performance, compared to the original one. > > > > > > This patchset updates the current jhash in DPDK, > > > including two new functions that generate two hashes from a single key. > > > > > > It also separates the existing hash function performance tests to > > > another file, to make it quicker to run. > > > > > > changes in v4: > > > - Simplify key alignment checks > > > - Include missing x86 arch check > > > > > > changes in v3: > > > > > > - Update rte_jhash_1word, rte_jhash_2words and rte_jhash_3words > > > functions > > > > > > changes in v2: > > > > > > - Split single commit in three commits, one that updates the existing > > functions > > > and another that adds two new functions and use one of those functions > > > as a base to be called by the other ones. > > > - Remove some unnecessary ifdefs in the code. > > > - Add new macros to help on the reutilization of constants > > > - Separate hash function performance tests to another file > > > and improve cycle measurements. > > > - Rename existing function rte_jhash2 to rte_jhash_32b > > > (something more meaninful) and mark rte_jhash2 as > > > deprecated > > > > > > Pablo de Lara (6): > > > test/hash: move hash function perf tests to separate file > > > test/hash: improve accuracy on cycle measurements > > > hash: update jhash function with the latest available > > > hash: add two new functions to jhash library > > > hash: remove duplicated code > > > hash: rename rte_jhash2 to rte_jhash_32b > > > > > > app/test/Makefile |1 + > > > app/test/test_func_reentrancy.c |2 +- > > > app/test/test_hash.c|4 +- > > > app/test/test_hash_func_perf.c | 145 + > > > app/test/test_hash_perf.c | 71 + > > > lib/librte_hash/rte_jhash.h | 338 +- > > - > > > 6 files changed, 402 insertions(+), 159 deletions(-) > > > create mode 100644 app/test/test_hash_func_perf.c > > > > > > -- > > > 1.7.4.1 > > > > > > > > did you run this through the ABI checker? I see you're removing several > > symbols > > that will likely need to go through the ABI deprecation process. > > > > Neil > > I had not run it, but I just did. I see no problems on librte_hash > (but I see some on rte_ethdev.h, due to another commit). > > Anyway, I renamed two functions to be more meaningful, but those functions > are "static inline", > so I am not sure exactly what the deprecation process is for those. > What I did was leaving the original function that calls the same function as > the new renamed one, > but adds a line warning that the functions is deprecated. > > Is that OK or should I do it differently? > As long as their all static inline and binaries that are already compiled can continue to access the data structures they reference at the member offsets encoded to them at compile time, you should be ok. Thanks! Neil > Thanks! > Pablo >
[dpdk-dev] [PATCH v4 0/6] update jhash function
Jenkins hash function was developed originally in 1996, and was integrated in first versions of DPDK. The function has been improved in 2006, achieving up to 60% better performance, compared to the original one. This patchset updates the current jhash in DPDK, including two new functions that generate two hashes from a single key. It also separates the existing hash function performance tests to another file, to make it quicker to run. changes in v4: - Simplify key alignment checks - Include missing x86 arch check changes in v3: - Update rte_jhash_1word, rte_jhash_2words and rte_jhash_3words functions changes in v2: - Split single commit in three commits, one that updates the existing functions and another that adds two new functions and use one of those functions as a base to be called by the other ones. - Remove some unnecessary ifdefs in the code. - Add new macros to help on the reutilization of constants - Separate hash function performance tests to another file and improve cycle measurements. - Rename existing function rte_jhash2 to rte_jhash_32b (something more meaninful) and mark rte_jhash2 as deprecated Pablo de Lara (6): test/hash: move hash function perf tests to separate file test/hash: improve accuracy on cycle measurements hash: update jhash function with the latest available hash: add two new functions to jhash library hash: remove duplicated code hash: rename rte_jhash2 to rte_jhash_32b app/test/Makefile |1 + app/test/test_func_reentrancy.c |2 +- app/test/test_hash.c|4 +- app/test/test_hash_func_perf.c | 145 + app/test/test_hash_perf.c | 71 + lib/librte_hash/rte_jhash.h | 338 +-- 6 files changed, 402 insertions(+), 159 deletions(-) create mode 100644 app/test/test_hash_func_perf.c -- 1.7.4.1
[dpdk-dev] [PATCH v4 0/6] update jhash function
On Tue, May 12, 2015 at 12:02:32PM +0100, Pablo de Lara wrote: > Jenkins hash function was developed originally in 1996, > and was integrated in first versions of DPDK. > The function has been improved in 2006, > achieving up to 60% better performance, compared to the original one. > > This patchset updates the current jhash in DPDK, > including two new functions that generate two hashes from a single key. > > It also separates the existing hash function performance tests to > another file, to make it quicker to run. > > changes in v4: > - Simplify key alignment checks > - Include missing x86 arch check > > changes in v3: > > - Update rte_jhash_1word, rte_jhash_2words and rte_jhash_3words > functions > > changes in v2: > > - Split single commit in three commits, one that updates the existing > functions > and another that adds two new functions and use one of those functions > as a base to be called by the other ones. > - Remove some unnecessary ifdefs in the code. > - Add new macros to help on the reutilization of constants > - Separate hash function performance tests to another file > and improve cycle measurements. > - Rename existing function rte_jhash2 to rte_jhash_32b > (something more meaninful) and mark rte_jhash2 as > deprecated > > Pablo de Lara (6): > test/hash: move hash function perf tests to separate file > test/hash: improve accuracy on cycle measurements > hash: update jhash function with the latest available > hash: add two new functions to jhash library > hash: remove duplicated code > hash: rename rte_jhash2 to rte_jhash_32b > > app/test/Makefile |1 + > app/test/test_func_reentrancy.c |2 +- > app/test/test_hash.c|4 +- > app/test/test_hash_func_perf.c | 145 + > app/test/test_hash_perf.c | 71 + > lib/librte_hash/rte_jhash.h | 338 > +-- > 6 files changed, 402 insertions(+), 159 deletions(-) > create mode 100644 app/test/test_hash_func_perf.c > > -- > 1.7.4.1 > > did you run this through the ABI checker? I see you're removing several symbols that will likely need to go through the ABI deprecation process. Neil
