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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2014-11-14 08:53, Neil Horman:
> 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:
> > >> 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.

Yes, it's better to make it working on all architectures.

> > 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.

It's the ideal case.

I remind the consensus we had about having different code paths:
	http://dpdk.org/ml/archives/dev/2014-August/004670.html
The idea is to make the code working on all architectures thanks to a generic
code which leverage build time optimizations.
In the meantime, we want to be able to build DPDK for a default platform with
different code paths. In case a really interesting optimization requires more
CPU features, we can check CPU flags at runtime and select the best code path.
In ACL code, it is the responsibility of the app to choose the code path:
	http://dpdk.org/browse/dpdk/tree/lib/librte_acl/rte_acl.h#n346

-- 
Thomas