DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Richardson, Bruce" <bruce.richardson@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Aaron Conole <aconole@redhat.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Jerin Jacob <jerinj@marvell.com>,
	Gavin Hu <gavin.hu@arm.com>,
	Michael Santana <msantana@redhat.com>
Subject: Re: [dpdk-dev] [PATCH 3/3] acl: adjust the tests
Date: Tue, 9 Apr 2019 17:05:21 +0000	[thread overview]
Message-ID: <59AF69C657FD0841A61C55336867B5B072787B4F@IRSMSX103.ger.corp.intel.com> (raw)
Message-ID: <20190409170521.nIEtksmKYApu167AegB6wHdaxMcPOf_sRr6L75Yfz7k@z> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB9772580148A94D87@irsmsx105.ger.corp.intel.com>



> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, April 9, 2019 5:03 PM
> To: Aaron Conole <aconole@redhat.com>
> Cc: dev@dpdk.org; Jerin Jacob <jerinj@marvell.com>; Gavin Hu
> <gavin.hu@arm.com>; Richardson, Bruce <bruce.richardson@intel.com>;
> Michael Santana <msantana@redhat.com>
> Subject: RE: [PATCH 3/3] acl: adjust the tests
> 
> 
> > > 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 <aconole@redhat.com>
> > >> ---
> > >>  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, but for SSE specifically why do we even compile up two copies of the function for x86 platforms, since SSE will always be supported?

/Bruce

  parent reply	other threads:[~2019-04-09 17:05 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-08 18:24 [dpdk-dev] [PATCH 0/3] librte_acl: fixes related to testing with the meson build Aaron Conole
2019-04-08 18:24 ` Aaron Conole
2019-04-08 18:24 ` [dpdk-dev] [PATCH 1/3] acl: fix arm argument types Aaron Conole
2019-04-08 18:24   ` Aaron Conole
2019-04-10 14:39   ` [dpdk-dev] [EXT] " Jerin Jacob Kollanukkaran
2019-04-10 14:39     ` Jerin Jacob Kollanukkaran
2019-04-10 15:52     ` Aaron Conole
2019-04-10 15:52       ` Aaron Conole
2019-04-10 16:07       ` Jerin Jacob Kollanukkaran
2019-04-10 16:07         ` Jerin Jacob Kollanukkaran
2019-04-10 17:20         ` Aaron Conole
2019-04-10 17:20           ` Aaron Conole
2019-04-30 12:57           ` Aaron Conole
2019-04-30 12:57             ` Aaron Conole
2019-06-05 15:16     ` Jerin Jacob Kollanukkaran
2019-06-05 17:09       ` Aaron Conole
2019-04-08 18:24 ` [dpdk-dev] [PATCH 2/3] acl: update the build for multi-arch Aaron Conole
2019-04-08 18:24   ` Aaron Conole
2019-04-08 18:24 ` [dpdk-dev] [PATCH 3/3] acl: adjust the tests Aaron Conole
2019-04-08 18:24   ` Aaron Conole
2019-04-09  8:41   ` Ananyev, Konstantin
2019-04-09  8:41     ` Ananyev, Konstantin
2019-04-09 13:01     ` Aaron Conole
2019-04-09 13:01       ` Aaron Conole
2019-04-09 16:03       ` Ananyev, Konstantin
2019-04-09 16:03         ` Ananyev, Konstantin
2019-04-09 17:04         ` Ananyev, Konstantin
2019-04-09 17:04           ` Ananyev, Konstantin
2019-04-10  8:13           ` Richardson, Bruce
2019-04-10  8:13             ` Richardson, Bruce
2019-04-10 13:10           ` Aaron Conole
2019-04-10 13:10             ` Aaron Conole
2019-04-10 13:24             ` Bruce Richardson
2019-04-10 13:24               ` Bruce Richardson
2019-04-10 13:46               ` Bruce Richardson
2019-04-10 13:46                 ` Bruce Richardson
2019-04-09 17:05         ` Richardson, Bruce [this message]
2019-04-09 17:05           ` Richardson, Bruce
2019-04-09 18:29           ` Ananyev, Konstantin
2019-04-09 18:29             ` Ananyev, Konstantin
2019-04-10  9:06             ` Bruce Richardson
2019-04-10  9:06               ` Bruce Richardson
2019-04-08 20:40 ` [dpdk-dev] [PATCH 0/3] librte_acl: fixes related to testing with the meson build Aaron Conole
2019-04-08 20:40   ` Aaron Conole

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=59AF69C657FD0841A61C55336867B5B072787B4F@IRSMSX103.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=aconole@redhat.com \
    --cc=dev@dpdk.org \
    --cc=gavin.hu@arm.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=msantana@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).