From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 332DD7E18 for ; Wed, 19 Nov 2014 11:06:17 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 19 Nov 2014 02:14:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,416,1413270000"; d="scan'208";a="610342769" Received: from bricha3-mobl3.ger.corp.intel.com ([10.243.20.23]) by orsmga001.jf.intel.com with SMTP; 19 Nov 2014 02:16:15 -0800 Received: by (sSMTP sendmail emulation); Wed, 19 Nov 2014 10:16:14 +0025 Date: Wed, 19 Nov 2014 10:16:14 +0000 From: Bruce Richardson To: Neil Horman Message-ID: <20141119101614.GA6532@bricha3-MOBL3> References: <1409724351-23786-1-git-send-email-e_zhumabekov@sts.kz> <20141118144138.GB32375@hmsreliant.think-freely.org> <546B607B.9030808@sts.kz> <20141118160005.GC32375@hmsreliant.think-freely.org> <546B7E2D.7050705@sts.kz> <20141118174619.GE32375@hmsreliant.think-freely.org> <20141118175226.GC5840@bricha3-MOBL3> <20141118213624.GF32375@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20141118213624.GF32375@hmsreliant.think-freely.org> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v4 3/5] hash: add fallback to software CRC32 implementation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Nov 2014 10:06:20 -0000 On Tue, Nov 18, 2014 at 04:36:24PM -0500, Neil Horman wrote: > On Tue, Nov 18, 2014 at 05:52:27PM +0000, Bruce Richardson wrote: > > On Tue, Nov 18, 2014 at 12:46:19PM -0500, Neil Horman wrote: > > > On Tue, Nov 18, 2014 at 11:13:17PM +0600, Yerden Zhumabekov wrote: > > > > > > > > 18.11.2014 22:00, Neil Horman пишет: > > > > > On Tue, Nov 18, 2014 at 09:06:35PM +0600, Yerden Zhumabekov wrote: > > > > >> 18.11.2014 20:41, Neil Horman пишет: > > > > >>> On Tue, Nov 18, 2014 at 08:03:40PM +0600, Yerden Zhumabekov wrote: > > > > >>>> /** > > > > >>>> * Use single crc32 instruction to perform a hash on a 4 byte value. > > > > >>>> + * Fall back to software crc32 implementation in case SSE4.2 is > > > > >>>> + * not supported > > > > >>>> * > > > > >>>> * @param data > > > > >>>> * Data to perform hash on. > > > > >>>> @@ -376,11 +413,18 @@ crc32c_2words(uint64_t data, uint32_t init_val) > > > > >>>> static inline uint32_t > > > > >>>> rte_hash_crc_4byte(uint32_t data, uint32_t init_val) > > > > >>>> { > > > > >>>> - return _mm_crc32_u32(init_val, data); > > > > >>>> +#ifdef RTE_MACHINE_CPUFLAG_SSE4_2 > > > > >>>> + if (likely(crc32_alg == CRC32_SSE42)) > > > > >>>> + return _mm_crc32_u32(init_val, data); > > > > >>>> +#endif > > > > >>> you don't really need these ifdefs here anymore given that you have a > > > > >>> constructor to do the algorithm selection. In fact you need to remove them, in > > > > >>> the event you build on a system that doesn't support SSE42, but run on a system > > > > >>> that does. > > > > >> Originally, I thought so as well. I wrote the code without these ifdefs, > > > > >> but it didn't compile on my machine which doesn't support SSE4.2. Error > > > > >> was triggered by nmmintrin.h which has a check for respective GCC > > > > >> extension. So I think these ifdefs are indeed required. > > > > >> > > > > > You need to edit the makefile so that the compiler gets passed the option > > > > > -msse42. That way it will know to emit sse42 instructions. It will also allow > > > > > you to remove the ifdef from the include file > > > > > > > > In this case, I guess there are two options: > > > > 1) modify all makefiles which use librte_hash > > > > 2) move all function bodies from rte_hash_crc.h to separate module, > > > > leaving prototype definitions there only. > > > > > > > > Everybody's up for the second option? :) > > > > > > > Crud, you're right, I didn't think about the header inclusion issue. Is it > > > worth adding the jump to enable the dynamic hash selection? > > > Neil > > > > Maybe for cases where SSE4.2 is not currently available, i.e. for generic builds. > > For builds where we have hardware support confirmed at compile time, just use > > the function from the header file. > > Does that make sense? > > > I'm not certain of that, as I don't think anything can be 'confirmed' at compile > time. I.e. just because you have sse42 at compile time doesn't guarantee you > have it at run time with a DSO. If you have these as macros, you need to enable > sse42 whereever you include the file so that the intrinsic works properly. Well, if you compile with sse42 at compile time, the compiler is free to insert sse4 instructions at any place it feels like, irrespective of whether or not you use SSE4 intrinsics, so I would never expect such a DSO to work on a system without SSE42 support. > > an alternate option would be to not use the intrinsic, and craft some explicit > __asm__ statement that executes the right sse42 instructions. That way the asm > is directly emitted, without requiring the -msse42 flag at all, and it will just > work in all the files that call it. > I really don't like that approach. I think using intrinsics is much more maintainable. /Bruce