From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH 0/2] rewritten rte_hash_crc() call Date: Fri, 14 Nov 2014 08:53:08 -0500 Message-ID: <20141114135308.GD19147@hmsreliant.think-freely.org> References: <1409724351-23786-1-git-send-email-e_zhumabekov@sts.kz> <2916837.QJI9btJm0N@xps13> <20141114005211.GC14230@localhost.localdomain> <5465AC00.1070602@sts.kz> <20141114113327.GA19147@hmsreliant.think-freely.org> <5465EE3F.2010404@sts.kz> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: dev-VfR2kkLFssw@public.gmane.org To: Yerden Zhumabekov Return-path: Content-Disposition: inline In-Reply-To: <5465EE3F.2010404-8EHiFRVJVgQ@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Fri, Nov 14, 2014 at 05:57:51PM +0600, Yerden Zhumabekov wrote: >=20 > 14.11.2014 17:33, Neil Horman =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > 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 > >> #define DEFAULT_HASH_FUNC rte_hash_crc > >> #else > >> #include > >> #define DEFAULT_HASH_FUNC rte_jhash > >> #endif > >> > >> In rte_fbk_hash.h > >> #ifdef RTE_MACHINE_CPUFLAG_SSE4_2 > >> #include > >> /** Default four-byte key hash function if none is specified. */ > >> #define RTE_FBK_HASH_FUNC_DEFAULT=C2=B7=C2=B7=C2=B7=C2=B7=C2=B7=C2=B7= =C2=B7rte_hash_crc_4byte > >> #else > >> #include > >> #define RTE_FBK_HASH_FUNC_DEFAULT=C2=B7=C2=B7=C2=B7=C2=B7=C2=B7=C2=B7= =C2=B7rte_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 applicatio= ns using > > the function directly. Test_hash_perf is an example of this, and os= tensibly > > because of the behavior without SSE4.2 it defines these huge test tab= les 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 use= s at run > > time configurable. >=20 > I see, then we have a problem here :) >=20 > 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 no= t > to contradict this approach. >=20 Thats a reasonable approach, but I really hate the idea of continuing thi= s 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 introduc= e > 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. >=20 Well, its possible you'll get lucky. crc is such a common operation, its entirely possible that the gcc intrinsic emits software based crc computa= tion if the SSE4.2 instructions aren't enabled. I recommend modifying the test_h= ash_crc function to use rte_hash_crc with SSE4.2 disabled, and see if you get a c= rash. If you don't examine the disassembly of your new function and confirm tha= t something reasonable that doesn't use SSE4.2 is emitted. If thats the ca= se, your patch is fine, and we can focus on how to change the ifdefs in the e= xisting code, as use of the rte_hash_crc functions should be safe. Best Neil > --=20 > Sincerely, >=20 > Yerden Zhumabekov > State Technical Service > Astana, KZ >=20 >=20