From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f45.google.com (mail-wg0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id 60AA27F00 for ; Fri, 14 Nov 2014 15:23:24 +0100 (CET) Received: by mail-wg0-f45.google.com with SMTP id x12so19764167wgg.18 for ; Fri, 14 Nov 2014 06:33:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=O6Sby8LIGIujdVtwAZqOJPZH8x9j17LsEqDma56P6Tg=; b=D5ewn7AmJjsCGFRx/sVTW5EbYiDI4xvmC4uyWKsi9lnqCe7UbyZaLE0JnpfVe8aaHW CZw0Wv2NQkLOdMMurfPm26XLcFIPvBpVYPmqgCpN76aTPUBFjafu9HmlWpmDMWOL0uUI 5r/k0O2Ea60N507uQIjIbLIii7sc4vo0h5MXRGWqscxbU4pFNrPDP64FjyePCAHoBDCX xveE+GUm8dA6KWGCSWSqGJ6ywziFwURvDZk92IU5DnWQY4ONCNYQ9rd76H73Ngp0t6iX EHKsEasnWYXiesLAbDdaSQ2fbSqJiElql3BG5yvWfMXbT7AQKODNbLdyz0W2LpJkBuZF 4ITg== X-Gm-Message-State: ALoCoQlrY06BsldLaSwWx1Q3ltFS5p3FvuLF+VHM8s81A0h1yPUeNMJpohpE21C49P5iygsyYdCV X-Received: by 10.194.95.200 with SMTP id dm8mr5822326wjb.133.1415975607712; Fri, 14 Nov 2014 06:33:27 -0800 (PST) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id a8sm3627046wiz.21.2014.11.14.06.33.26 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 14 Nov 2014 06:33:26 -0800 (PST) From: Thomas Monjalon To: Neil Horman Date: Fri, 14 Nov 2014 15:33:06 +0100 Message-ID: <1509048.bxfDRSE0F2@xps13> Organization: 6WIND User-Agent: KMail/4.14.2 (Linux/3.17.2-1-ARCH; KDE/4.14.2; x86_64; ; ) In-Reply-To: <20141114135308.GD19147@hmsreliant.think-freely.org> References: <1409724351-23786-1-git-send-email-e_zhumabekov@sts.kz> <5465EE3F.2010404@sts.kz> <20141114135308.GD19147@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 0/2] rewritten rte_hash_crc() call 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: Fri, 14 Nov 2014 14:23:24 -0000 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 =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > > 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 > > >> #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 h= ash function > > > using the DEFUALT_HASH_FUNC macro, but doesn't nothing for applic= ations using > > > the function directly. Test_hash_perf is an example of this, an= d 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 co= uld allow > > > applications to use rte_hash_crc regardless, and make the code it= uses 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 functi= on > > was common. So I organized patches to minimize the impact on API an= d not > > to contradict this approach. > >=20 > 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 intr= oduce > > broader changes to rte_hash library and change other code which use= s 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 com= putation if > the SSE4.2 instructions aren't enabled. I recommend modifying the te= st_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 th= e case, > your patch is fine, and we can focus on how to change the ifdefs in t= he 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: =09http://dpdk.org/ml/archives/dev/2014-August/004670.html The idea is to make the code working on all architectures thanks to a g= eneric code which leverage build time optimizations. In the meantime, we want to be able to build DPDK for a default platfor= m with different code paths. In case a really interesting optimization require= s more CPU features, we can check CPU flags at runtime and select the best cod= e path. In ACL code, it is the responsibility of the app to choose the code pat= h: =09http://dpdk.org/browse/dpdk/tree/lib/librte_acl/rte_acl.h#n346 --=20 Thomas