Re: [dpdk-dev] [PATCH 0/2] rewritten rte_hash_crc() call

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Nov 14, 2014 at 05:57:51PM +0600, Yerden Zhumabekov wrote:
> 
> 14.11.2014 17:33, Neil Horman пишет:
> > On Fri, Nov 14, 2014 at 01:15:12PM +0600, Yerden Zhumabekov wrote:
> >>
> >> Hello,
> >>
> >> A quick grep on dpdk source shows that rte_hash_crc() is used in
> >> librte_hash in following context:
> >>
> >> In rte_hash.c:
> >> /* Hash function used if none is specified */
> >> #ifdef RTE_MACHINE_CPUFLAG_SSE4_2
> >> #include <rte_hash_crc.h>
> >> #define DEFAULT_HASH_FUNC       rte_hash_crc
> >> #else
> >> #include <rte_jhash.h>
> >> #define DEFAULT_HASH_FUNC       rte_jhash
> >> #endif
> >>
> >> In rte_fbk_hash.h
> >> #ifdef RTE_MACHINE_CPUFLAG_SSE4_2
> >> #include <rte_hash_crc.h>
> >> /** Default four-byte key hash function if none is specified. */
> >> #define RTE_FBK_HASH_FUNC_DEFAULT·······rte_hash_crc_4byte
> >> #else
> >> #include <rte_jhash.h>
> >> #define RTE_FBK_HASH_FUNC_DEFAULT·······rte_jhash_1word
> >> #endif
> >> #endif
> >>
> >>
> >> I guess it covers the cpu flags check you're talking about.
> >>
> > Not really.  That covers the case of applications selecting the hash function
> > using the DEFUALT_HASH_FUNC macro, but doesn't nothing for applications using
> > the function directly.  Test_hash_perf is an example  of this, and ostensibly
> > because of the behavior without SSE4.2 it defines these huge test tables twice
> > based on the availability of SSE4.2.  It would be better if we could allow
> > applications to use rte_hash_crc regardless, and make the code it uses at run
> > time configurable.
> 
> I see, then we have a problem here :)
> 
> Actually, that was one of my concerns when developing these patches. I
> looked through the source code of libs and examples and I saw the
> '#ifdef..#include..#endif'-like appoach while selecting hash function
> was common. So I organized patches to minimize the impact on API and not
> to contradict this approach.
> 
Thats a reasonable approach, but I really hate the idea of continuing this need
to select cpu features at compile time if its not nececcesary.

> If we prefer to change this approach then, I guess, we need to introduce
> broader changes to rte_hash library and change other code which uses it.
> If that's what's needed, then it'll take some time for me to rework
> these patches.
> 
Well, its possible you'll get lucky.  crc is such a common operation, its
entirely possible that the gcc intrinsic emits software based crc computation if
the SSE4.2 instructions aren't enabled.  I recommend modifying the test_hash_crc
function to use rte_hash_crc with SSE4.2 disabled, and see if you get a crash.
If you don't examine the disassembly of your new function and confirm that
something reasonable that doesn't use SSE4.2 is emitted.  If thats the case,
your patch is fine, and we can focus on how to change the ifdefs in the existing
code, as use of the rte_hash_crc functions should be safe.

Best
Neil

> -- 
> Sincerely,
> 
> Yerden Zhumabekov
> State Technical Service
> Astana, KZ
> 
>