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 173C9A0096 for ; Wed, 10 Apr 2019 15:10:31 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 85C4F7CCA; Wed, 10 Apr 2019 15:10:29 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 796C15F32 for ; Wed, 10 Apr 2019 15:10:27 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9D6233099FC4; Wed, 10 Apr 2019 13:10:26 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C7ED5600CC; Wed, 10 Apr 2019 13:10:25 +0000 (UTC) From: Aaron Conole To: "Ananyev\, Konstantin" Cc: "dev\@dpdk.org" , Jerin Jacob , Gavin Hu , "Richardson\, Bruce" , "Michael Santana" 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> <2601191342CEEE43887BDE71AB9772580148A94DA8@irsmsx105.ger.corp.intel.com> Date: Wed, 10 Apr 2019 09:10:25 -0400 In-Reply-To: <2601191342CEEE43887BDE71AB9772580148A94DA8@irsmsx105.ger.corp.intel.com> (Konstantin Ananyev's message of "Tue, 9 Apr 2019 17:04:35 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Wed, 10 Apr 2019 13:10:26 +0000 (UTC) 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: <20190410131025.nSlWzzZD5_PRaMKv2FXd6apeKYnbqnyUxZpdskFqGr0@z> "Ananyev, Konstantin" writes: >> >> > > 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. > > And the changes below make linker to pick-up the proper version of the function > and make acl_autotest to pass for static build too. > > diff --git a/app/test/meson.build b/app/test/meson.build > index 867cc5863..4364be932 100644 > --- a/app/test/meson.build > +++ b/app/test/meson.build > @@ -328,6 +328,7 @@ test_dep_objs += cc.find_library('execinfo', required: false) > link_libs = [] > if get_option('default_library') == 'static' > link_libs = dpdk_drivers > + link_libs += dpdk_static_libraries > endif > > if get_option('tests') > diff --git a/meson.build b/meson.build > index a96486597..df1e1c41c 100644 > --- a/meson.build > +++ b/meson.build > @@ -62,6 +62,7 @@ configure_file(output: build_cfg, > # for static builds, include the drivers as libs and we need to "whole-archive" > # them. > dpdk_drivers = ['-Wl,--whole-archive'] + dpdk_drivers + ['-Wl,--no-whole-archive'] > +dpdk_static_libraries = ['-Wl,--whole-archive'] + dpdk_static_libraries + ['-Wl,--no-whole-archive'] > > Not saying that's the proper patch, but just to prove that linking librte_acl.a > with '--whole-archive' does fix the problem. > Bruce, could you point is the best way to fix things here > (my meson knowledge is very limited)? > Do we need extra container here as 'whole_archive_static_libraries[]' or so? > Thanks > Konstantin Okay - I'll look at this part more. I think I went down the path of explicitly setting these because the comments didn't match with what was occuring (for example, in the section that I changed that loops through all versions, only the AVX2 and Scalar were being tested on my system, while the comment implied SSE). I also believe that I split out the functions because of the linking issue (I guess the way the linker resolves the functions works properly when the weak versions are in a different translation unit)? I'll spend some time trying to get it working in a different way. Regardless, this wasn't ready for posting as 'PATCH' - I meant it as RFC. I don't intend to change the first two patches, though. And thank you for the all the feedback! > >> >> >> > and on the systems I tested with (including tests under travis). >> > >> > 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. >> > >> > 2. The tests never selected scalar, or nor sse implementations. >> >> As I can see test_classify_run() *always* run both default method (sse/avx2 on x86) >> and then scalar one. >> >> > 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. >> > >> > There were others - I iterated on these for a few days. >> > >> > This is why I changed a block to run through each implementation in one >> > of the versions. >> > >> > HOWEVER, it's still deficient. >> > >> > 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 with >> > almost no reason why. And looking into the failure revealed that the >> > meson build didn't even include the platform specific builds. >> > >> > 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) >> instructions or not. >> Are you saying under some circumstances rte_acl_init() are not working properly, >> or not get invoked? >> >> Konstantin