From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 68EB04F9A for ; Tue, 9 Apr 2019 15:02:18 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AC41C80467; Tue, 9 Apr 2019 13:02:17 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-123-89.rdu2.redhat.com [10.10.123.89]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27F6D69196; Tue, 9 Apr 2019 13:02:15 +0000 (UTC) From: Aaron Conole To: "Ananyev\, Konstantin" Cc: "dev\@dpdk.org" , Jerin Jacob , Gavin Hu , Bruce Richardson , Michael Santana In-Reply-To: <2601191342CEEE43887BDE71AB9772580148A94A44@irsmsx105.ger.corp.intel.com> (Konstantin Ananyev's message of "Tue, 9 Apr 2019 08:41:41 +0000") References: <20190408182420.4398-1-aconole@redhat.com> <20190408182420.4398-4-aconole@redhat.com> <2601191342CEEE43887BDE71AB9772580148A94A44@irsmsx105.ger.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) Date: Tue, 09 Apr 2019 09:01:53 -0400 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 09 Apr 2019 13:02:17 +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: , X-List-Received-Date: Tue, 09 Apr 2019 13:02:18 -0000 "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. 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). The tests were failing for a number of reasons when built with meson, 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. 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). WDYT? > Konstantin 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 970F6A0096 for ; Tue, 9 Apr 2019 15:02:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8BDEC568A; Tue, 9 Apr 2019 15:02:19 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 68EB04F9A for ; Tue, 9 Apr 2019 15:02:18 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AC41C80467; Tue, 9 Apr 2019 13:02:17 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-123-89.rdu2.redhat.com [10.10.123.89]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27F6D69196; Tue, 9 Apr 2019 13:02:15 +0000 (UTC) From: Aaron Conole To: "Ananyev\, Konstantin" Cc: "dev\@dpdk.org" , Jerin Jacob , Gavin Hu , Bruce Richardson , Michael Santana In-Reply-To: <2601191342CEEE43887BDE71AB9772580148A94A44@irsmsx105.ger.corp.intel.com> (Konstantin Ananyev's message of "Tue, 9 Apr 2019 08:41:41 +0000") References: <20190408182420.4398-1-aconole@redhat.com> <20190408182420.4398-4-aconole@redhat.com> <2601191342CEEE43887BDE71AB9772580148A94A44@irsmsx105.ger.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) Date: Tue, 09 Apr 2019 09:01:53 -0400 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 09 Apr 2019 13:02:17 +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: <20190409130153.SvpjD_RgbJ9ZEWlytG9R6_rDa3Ar-tM57ucfjPU_S-Y@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. 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). The tests were failing for a number of reasons when built with meson, 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. 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). WDYT? > Konstantin