From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 9D4F0A0096 for ; Wed, 10 Apr 2019 11:06:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1183C5942; Wed, 10 Apr 2019 11:06:32 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 22E9C58C4 for ; Wed, 10 Apr 2019 11:06:29 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Apr 2019 02:06:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,332,1549958400"; d="scan'208";a="130121490" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.35]) by orsmga007.jf.intel.com with SMTP; 10 Apr 2019 02:06:23 -0700 Received: by (sSMTP sendmail emulation); Wed, 10 Apr 2019 10:06:22 +0100 Date: Wed, 10 Apr 2019 10:06:22 +0100 From: Bruce Richardson To: "Ananyev, Konstantin" Cc: Aaron Conole , "dev@dpdk.org" , Jerin Jacob , Gavin Hu , Michael Santana Message-ID: <20190410090622.GB700@bricha3-MOBL.ger.corp.intel.com> References: <20190408182420.4398-1-aconole@redhat.com> <20190408182420.4398-4-aconole@redhat.com> <2601191342CEEE43887BDE71AB9772580148A94A44@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580148A94D87@irsmsx105.ger.corp.intel.com> <59AF69C657FD0841A61C55336867B5B072787B4F@IRSMSX103.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580148A94DF5@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB9772580148A94DF5@irsmsx105.ger.corp.intel.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH 3/3] acl: adjust the tests X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190410090622.8I5f0bA8q-WeCCH00WznbBfE8xPAxRZifgab9X5nxuY@z> On Tue, Apr 09, 2019 at 07:29:09PM +0100, Ananyev, Konstantin wrote: > > > > > > > > > Hi Aaron, > > > > > > > > > >> > > > > >> This makes the tests pass, and also ensures that on platforms where > > > > >> the testing is supported, we can properly test the implementation > > > > >> specific code. One edge case is when we run on x86_64 systems that > > > > >> don't support AVX2, but where the compiler can generate such > > > > >> instructions. That could be an enhancement in the future, but for > > > > >> now at least the tests will pass. > > > > >> > > > > >> Signed-off-by: Aaron Conole > > > > >> --- > > > > >> app/test/test_acl.c | 62 +++++++++++++------------------ > > > -- > > > > >> lib/librte_acl/Makefile | 1 + > > > > >> lib/librte_acl/acl_run_notsup.c | 46 ++++++++++++++++++++++++ > > > > >> lib/librte_acl/meson.build | 4 +-- > > > > >> 4 files changed, 73 insertions(+), 40 deletions(-) create mode > > > > >> 100644 lib/librte_acl/acl_run_notsup.c > > > > >> > > > > >> diff --git a/app/test/test_acl.c b/app/test/test_acl.c index > > > > >> b1f75d1bc..c44faa251 100644 > > > > >> --- a/app/test/test_acl.c > > > > >> +++ b/app/test/test_acl.c > > > > >> @@ -408,6 +408,9 @@ test_classify(void) > > > > >> return -1; > > > > >> } > > > > >> > > > > >> + /* Always use the scalar testing for now. */ > > > > >> + rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR); > > > > >> + > > > > >> ret = 0; > > > > >> for (i = 0; i != TEST_CLASSIFY_ITER; i++) { > > > > >> > > > > >> @@ -547,6 +550,7 @@ test_build_ports_range(void) > > > > >> for (i = 0; i != RTE_DIM(test_data); i++) > > > > >> data[i] = (uint8_t *)&test_data[i]; > > > > >> > > > > >> + rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR); > > > > >> for (i = 0; i != RTE_DIM(test_rules); i++) { > > > > >> rte_acl_reset(acx); > > > > >> ret = test_classify_buid(acx, test_rules, i + 1); @@ - > > > 911,6 > > > > >> +915,8 @@ test_convert_rules(const char *desc, > > > > >> return -1; > > > > >> } > > > > >> > > > > >> + rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR); > > > > >> + > > > > > > > > > > As I understand here and above, on x86 you replaced default algo > > > > > (SSE, AVX2) with scalar one, right? > > > > > That looks like reduction of test coverage for x86. > > > > > > > > In one way, you're right. However, the tests weren't testing what > > > > they purported anyway. > > > > > > Could you explain a bit more here? > > > What I am seeing: tests were running bot sse(or avx2) and scalar > > > classify() method. > > > Now they always running scalar only. > > > To me it definitely looks like reduction in coverage. > > > > > > > Actually, it's just a shift I think (previously, it would have tested > > > > the AVX2 but I don't see AVX2 having a fallback into the SSE code - > > > > unlike the SSE code falling back into scalar). > > > > > > Not sure I understand you here. > > > What fallback for AVX2 you expect that you think is missing? > > > > > > > > > > > The tests were failing for a number of reasons when built with meson, > > > > > > Ok, but with legacy build system (make) on x86 all tests passes, right? > > > So the problem is in new build system, not in the test itself. > > > Why we should compromise our test coverage to make it work with new tools? > > > That just hides the problem without fixing it. > > > Instead I think the build system needs to be fixed. > > > Looking at it a bit closely, for .so meson+ninja generates code with > > > correct version of the function: > > > > > > nm x86_64-native-linuxapp-gcc-meson/lib/librte_acl.so.2 | grep > > > acl_classify_sse > > > 000000000000fa50 t rte_acl_classify_sse > > > > > > So for 'meson -Ddefault_library=shared' > > > acl_autotest passes without the problem. > > > > > > Though for static lib we have both: > > > nm x86_64-native-linuxapp-gcc-meson/lib/librte_acl.a | grep > > > acl_classify_sse > > > 0000000000000000 W rte_acl_classify_sse > > > 0000000000004880 T rte_acl_classify_sse > > > > > > And then linker pickups the wrong one: > > > nm x86_64-native-linuxapp-gcc-meson/app/test/dpdk-test | grep > > > acl_classify_sse > > > 00000000005f6100 W rte_acl_classify_sse > > > > > > While for make: > > > $ nm x86_64-native-linuxapp-gcc-aesmb/lib/librte_acl.a | grep > > > acl_classify_sse > > > 0000000000000000 W rte_acl_classify_sse > > > 0000000000004880 T rte_acl_classify_sse > > > $ nm x86_64-native-linuxapp-gcc-aesmb/app/test | grep acl_classify_sse > > > 0000000000240440 T rte_acl_classify_sse > > > > > > Linker pickups the right one. > > > > > > > I assume the same issues occurs for AVX2, > > Yes, I just used sse because it is always available on x86. > > but for SSE specifically why do we even compile up two copies of the function for x86 platforms, > > since SSE will always be supported? > > for non IA platforms. Yes, I realise that, but there is no point in compiling the weak version for IA platforms, since the normal version will be guaranteed available. In any case, it doesn't matter much, since the issue needs fixing for AVX2 anyway. /Bruce