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 3254EA0096 for ; Tue, 9 Apr 2019 18:03:09 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C463E2BCE; Tue, 9 Apr 2019 18:03:07 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 1E7B02BCE for ; Tue, 9 Apr 2019 18:03:05 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Apr 2019 09:03:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,329,1549958400"; d="scan'208";a="138842100" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga008.fm.intel.com with ESMTP; 09 Apr 2019 09:03:03 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.31]) by IRSMSX102.ger.corp.intel.com ([169.254.2.21]) with mapi id 14.03.0415.000; Tue, 9 Apr 2019 17:03:02 +0100 From: "Ananyev, Konstantin" To: Aaron Conole CC: "dev@dpdk.org" , Jerin Jacob , Gavin Hu , "Richardson, Bruce" , "Michael Santana" Thread-Topic: [PATCH 3/3] acl: adjust the tests Thread-Index: AQHU7tRnViMjaDjlwE61eucKpeigIKYzz1TA Date: Tue, 9 Apr 2019 16:03:01 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580148A94D87@irsmsx105.ger.corp.intel.com> References: <20190408182420.4398-1-aconole@redhat.com> <20190408182420.4398-4-aconole@redhat.com> <2601191342CEEE43887BDE71AB9772580148A94A44@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTVlMDgyNjUtZmRlZC00NTcwLWIyYWQtZjQxYzc0NjI4OTczIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiXC9uVXlYZldcL2EzK1dVS28rZjdhZ2ZIWW5FOFJGREN0clVGbk5vZ3BBNGtZY1UyUEpmS1ZWbitIdlNOam05azRcLyJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: <20190409160301.FCbIkZPHOQ3hV0SxoF4nuDNe-AZyKh_lXn-NI_MFY9g@z> > > Hi Aaron, > > > >> > >> This makes the tests pass, and also ensures that on platforms where th= e > >> 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 suppo= rt > >> AVX2, but where the compiler can generate such instructions. That cou= ld > >> 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 =3D 0; > >> for (i =3D 0; i !=3D TEST_CLASSIFY_ITER; i++) { > >> > >> @@ -547,6 +550,7 @@ test_build_ports_range(void) > >> for (i =3D 0; i !=3D RTE_DIM(test_data); i++) > >> data[i] =3D (uint8_t *)&test_data[i]; > >> > >> + rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR); > >> for (i =3D 0; i !=3D RTE_DIM(test_rules); i++) { > >> rte_acl_reset(acx); > >> ret =3D 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. >=20 > 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? >=20 > 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?=20 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: =20 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=3Dshared' acl_autotest passes without the problem.=20 Though for static lib we have both: nm x86_64-native-linuxapp-gcc-meson/lib/librte_acl.a | grep acl_classify_ss= e 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=20 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. > and on the systems I tested with (including tests under travis). >=20 > 1. Any meson build that I observed didn't correctly fill anything but > the scalar variable. I had to remove the -ENOTSUP definitions in the > rte_acl.c file (forgot to git add it), and make the second version. >=20 > 2. The tests never selected scalar, or nor sse implementations.=20 As I can see test_classify_run() *always* run both default method (sse/avx2= on x86) and then scalar one.=20 > Rather, > they selected only what the currently running platform provided. > This meant that I was always seeing the AVX2 code executed, but never > SSE nor scalar (but for one case) - at least as far as I could see. >=20 > There were others - I iterated on these for a few days. >=20 > This is why I changed a block to run through each implementation in one > of the versions. >=20 > HOWEVER, it's still deficient. >=20 > We need to fully cover all the cases. BUT it's better than the failure > that currently happens on almost every system I've tried - including > shipping the build to travis to run. So, I figured running > failing wit= h > almost no reason why. And looking into the failure revealed that the > meson build didn't even include the platform specific builds. >=20 > During my rework, I can change the test cases to iterate as in other > test cases. It will extend the time. And I don't know how to resolve > the case where we run on a system that doesn't support AVX2 but we have > a compiler that supports AVX2 (since that case will fail - but we > shouldn't even attempt it). I don't see why that should happen. At rte_acl_init() we do check does that machine supports AVX2(SSE, NEON)=20 instructions or not. Are you saying under some circumstances rte_acl_init() are not working prop= erly, or not get invoked? Konstantin