DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Aaron Conole <aconole@redhat.com>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"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: Wed, 10 Apr 2019 14:24:56 +0100	[thread overview]
Message-ID: <20190410132456.GB718@bricha3-MOBL.ger.corp.intel.com> (raw)
Message-ID: <20190410132456.STjBIdiR8U2ik1RuzrqQuXsdDsOoRI5ywodInBtW-eo@z> (raw)
In-Reply-To: <f7ta7gygg5a.fsf@dhcp-25.97.bos.redhat.com>

On Wed, Apr 10, 2019 at 09:10:25AM -0400, Aaron Conole wrote:
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> 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 <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.
> >
> > 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!
> 
I've dug into this a bit, and I'm doing up a patch to remove the use of
weak symbols in our libraries (note, just libs, not drivers) entirely.
That's fairly easy to do, and not a big change, but should make this
problem go away.

/Bruce

  parent reply	other threads:[~2019-04-10 13:25 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 [this message]
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
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=20190410132456.GB718@bricha3-MOBL.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).