From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A7F25569A for ; Tue, 6 Jan 2015 10:57:43 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 06 Jan 2015 01:57:42 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,706,1413270000"; d="scan'208";a="646922179" Received: from irsmsx155.ger.corp.intel.com ([163.33.192.3]) by fmsmga001.fm.intel.com with ESMTP; 06 Jan 2015 01:57:41 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.195]) by IRSMSX155.ger.corp.intel.com ([169.254.14.228]) with mapi id 14.03.0195.001; Tue, 6 Jan 2015 09:57:40 +0000 From: "Ananyev, Konstantin" To: Neil Horman Thread-Topic: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as new rte_acl_classify() method Thread-Index: AQHQGIBe6R2bzzgwb0yDNvzVCW5u5ZyQ0+9ggABERICAAUFi0IABksMAgAAvUECAACMpgIABNPIAgB2JDpA= Date: Tue, 6 Jan 2015 09:57:40 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258213D1E29@irsmsx105.ger.corp.intel.com> References: <1418580659-12595-1-git-send-email-konstantin.ananyev@intel.com> <1418580659-12595-11-git-send-email-konstantin.ananyev@intel.com> <20141215160009.GC3803@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB977258213C0D9C@IRSMSX105.ger.corp.intel.com> <20141215202043.GD3803@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB977258213C12AD@IRSMSX105.ger.corp.intel.com> <20141217153232.GA6618@localhost.localdomain> <2601191342CEEE43887BDE71AB977258213C1AB4@IRSMSX105.ger.corp.intel.com> <20141217202743.GA10240@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB977258213C1DDD@IRSMSX105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB977258213C1DDD@IRSMSX105.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as new rte_acl_classify() method X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Jan 2015 09:57:46 -0000 Hi Neil, Any further comments on that one? Konstantin > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin > Sent: Thursday, December 18, 2014 3:02 PM > To: Neil Horman > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as new rte_acl= _classify() method >=20 >=20 >=20 > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > Sent: Wednesday, December 17, 2014 8:28 PM > > To: Ananyev, Konstantin > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as new rte_a= cl_classify() method > > > > On Wed, Dec 17, 2014 at 07:22:06PM +0000, Ananyev, Konstantin wrote: > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > Sent: Wednesday, December 17, 2014 3:33 PM > > > > To: Ananyev, Konstantin > > > > Cc: dev@dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as new r= te_acl_classify() method > > > > > > > > On Tue, Dec 16, 2014 at 04:16:48PM +0000, Ananyev, Konstantin wrote= : > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > > > Sent: Monday, December 15, 2014 8:21 PM > > > > > > To: Ananyev, Konstantin > > > > > > Cc: dev@dpdk.org > > > > > > Subject: Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 as n= ew rte_acl_classify() method > > > > > > > > > > > > On Mon, Dec 15, 2014 at 04:33:47PM +0000, Ananyev, Konstantin w= rote: > > > > > > > Hi Neil, > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > > > > > Sent: Monday, December 15, 2014 4:00 PM > > > > > > > > To: Ananyev, Konstantin > > > > > > > > Cc: dev@dpdk.org > > > > > > > > Subject: Re: [dpdk-dev] [PATCH 10/17] librte_acl: add AVX2 = as new rte_acl_classify() method > > > > > > > > > > > > > > > > On Sun, Dec 14, 2014 at 06:10:52PM +0000, Konstantin Ananye= v wrote: > > > > > > > > > Introduce new classify() method that uses AVX2 instructio= ns. > > > > > > > > > From my measurements: > > > > > > > > > On HSW boards when processing >=3D 16 packets per call, > > > > > > > > > AVX2 method outperforms it's SSE counterpart by 10-25%, > > > > > > > > > (depending on the ruleset). > > > > > > > > > At runtime, this method is selected as default one on HW = that supports AVX2. > > > > > > > > > > > > > > > > > > Signed-off-by: Konstantin Ananyev > > > > > > > > > --- > > > > > > > > > lib/librte_acl/Makefile | 9 + > > > > > > > > > lib/librte_acl/acl.h | 4 + > > > > > > > > > lib/librte_acl/acl_run.h | 2 +- > > > > > > > > > lib/librte_acl/acl_run_avx2.c | 58 +++++ > > > > > > > > > lib/librte_acl/acl_run_avx2.h | 305 ++++++++++++++++++++= ++++ > > > > > > > > > lib/librte_acl/acl_run_sse.c | 537 +-------------------= ---------------------- > > > > > > > > > lib/librte_acl/acl_run_sse.h | 533 ++++++++++++++++++++= +++++++++++++++++++++ > > > > > > > > > lib/librte_acl/rte_acl.c | 5 +- > > > > > > > > > lib/librte_acl/rte_acl.h | 2 + > > > > > > > > > 9 files changed, 917 insertions(+), 538 deletions(-) > > > > > > > > > create mode 100644 lib/librte_acl/acl_run_avx2.c > > > > > > > > > create mode 100644 lib/librte_acl/acl_run_avx2.h > > > > > > > > > create mode 100644 lib/librte_acl/acl_run_sse.h > > > > > > > > > > > > > > > > > > diff --git a/lib/librte_acl/Makefile b/lib/librte_acl/Mak= efile > > > > > > > > > index 65e566d..223ec31 100644 > > > > > > > > > --- a/lib/librte_acl/Makefile > > > > > > > > > +++ b/lib/librte_acl/Makefile > > > > > > > > > @@ -45,8 +45,17 @@ SRCS-$(CONFIG_RTE_LIBRTE_ACL) +=3D acl= _bld.c > > > > > > > > > SRCS-$(CONFIG_RTE_LIBRTE_ACL) +=3D acl_gen.c > > > > > > > > > SRCS-$(CONFIG_RTE_LIBRTE_ACL) +=3D acl_run_scalar.c > > > > > > > > > SRCS-$(CONFIG_RTE_LIBRTE_ACL) +=3D acl_run_sse.c > > > > > > > > > +SRCS-$(CONFIG_RTE_LIBRTE_ACL) +=3D acl_run_avx2.c > > > > > > > > > > > > > > > > > > CFLAGS_acl_run_sse.o +=3D -msse4.1 > > > > > > > > > +ifeq ($(CC), icc) > > > > > > > > > +CFLAGS_acl_run_avx2.o +=3D -march=3Dcore-avx2 > > > > > > > > > +else ifneq ($(shell \ > > > > > > > > > +test $(GCC_MAJOR_VERSION) -le 4 -a $(GCC_MINOR_VERSION) = -le 6 && echo 1), 1) > > > > > > > > > +CFLAGS_acl_run_avx2.o +=3D -mavx2 > > > > > > > > > +else > > > > > > > > > +CFLAGS_acl_run_avx2.o +=3D -msse4.1 > > > > > > > > > +endif > > > > > > > > > > > > > > > > > This seems broken. You've unilaterally included acl_run_av= x2.c in the build > > > > > > > > list above, but only enable -mavx2 if the compiler is at le= ast gcc 4.6. > > > > > > > > > > > > > > Actually 4.7 (before that version, as I know, gcc doesn't su= pport avx2) > > > > > > > > > > > > > > > Unless > > > > > > > > you want to make gcc 4.6 a requirement for building, > > > > > > > > > > > > > > I believe DPDK is required to be buildable by gcc 4.6 > > > > > > > As I remember, we have to support it all way down to gcc 4.3. > > > > > > > > > > > > > > > you need to also exclude > > > > > > > > the file above from the build list. > > > > > > > > > > > > > > That means that for gcc 4.6 and below rte_acl_classify_avx2(= ) would not be defined. > > > > > > > And then at runtime, I have to check for that somehow and (re= )populate classify_fns[]. > > > > > > > Doesn't seems like a good way to me. > > > > > > There are plenty of ways around that. > > > > > > > > > > > > At a minimum you could make the classify_fns array the one plac= e that you need > > > > > > to add an ifdef __AVX__ call. > > > > > > > > > > > > You could also create a secondary definition of rte_acl_classif= y_avx2, and mark > > > > > > it as a weak symbol, which only returns -EOPNOTSUPP. That woul= d be good, since > > > > > > the right thing will just automatically happen then if you don'= t build the > > > > > > actual avx2 classification code > > > > > > > > > > > > > Instead, I prefer to always build acl_run_avx2.c, > > > > > > > > > > > > > > > > But you can't do that. You just said above that you need to su= pport down to gcc > > > > > > 4.3. I see you've worked around that with some additional ifde= f __AVX__ > > > > > > instructions, but in so doing you ignore the possibiity that ss= e isn't > > > > > > supported, so you need to add __SSE__ checks now as well. ifde= ffing that much > > > > > > just isn't scalable. > > > > > > > > > > We don't need to worry about compiler without SSE4.1 support. > > > > > I believe that all compilers that DDPDK has to build with, do sup= port SSE4.1. > > > > > So for SSE4.1 we only has to worry about situation when target CP= U doesn't support it > > > > > We manage it by runtime selection. > > > > > For AVX2 - situation is a bit different: it could be both compile= r and target CPU that don't support it. > > > > > > > > > > > And for your effort, you get an AVX2 classification path > > > > > > that potentially doesn't actually do vectorized classification. > > > > > > > > > > > > It really seems better to me to not build the code if the compi= ler doesn't > > > > > > support the instruction set it was meant to enable, and change = the > > > > > > classification function pointer to something that informs the u= ser of the lack > > > > > > of support at run time. > > > > > > > > > > > > > but for old compilers that don't support AVX2 - > > > > > > > rte_acl_classify_avx2() would simply be identical to rte_acl_= classify_sse(). > > > > > > > > > > > > > That doesn't make sense to me, for two reasons: > > > > > > > > > > > > 1) What if the machine being targeted doesn't support sse eithe= r? > > > > > > > > > > > > > > > > Exactly the same what is happening now on the machine with now SS= E4.1 support. > > > > > There is absolutely no difference here. > > > > > > > > > > > 2) If an application selects an AVX2 classifier, I as a develop= er expect to > > > > > > either get AVX2 based classification, or an error indicating th= at I can't do > > > > > > AVX2 classification, not a silent performance degradation down = to scalar > > > > > > classification. > > > > > > > > > > In fact I was considering both variants for compilers not support= ing AVX2: > > > > > 1. silently degrade to SSE method. > > > > > 2. create a dummy function rte_acl_classify_error() and put it = into classify_fns[RTE_ACL_CLASSIFY_AVX2]. > > > > > > > > > > I choose #1 because it seems like a less distraction for the user= - > > > > > all would keep working as before, user just wouldn't see any impr= ovement comparing to SSE method. > > > > > Again didn't want to spread "ifdef __AVX2__" into rte_acl.c > > > > > Though I don't have any strong opinion here. > > > > > So if you can provide some good reason why #2 is preferable, I am= ok to switch to #2. > > > > > > > > > Because 2 doesn't require any ifdeffing. As you note above the pro= blem here is > > > > that AVX2 support is both compiler and machine dependent. If you m= ake a weak > > > > symbol version of rte_acl_classify_avx2 that always gets built, the= n you've > > > > reduced the problem to just being compiler support, which you can c= heck in the > > > > makefile. > > > > > > I don't think we'll get rid of ifdefing with #2. > > > We'll remove 2 ifdefs in acl_run_avx2.h, but then we have to introdu= ce 2 new in rte_acl.c instead. > > > From my understanding, we we'll need something like that: > > > > > > static const rte_acl_classify_t classify_fns[] =3D { > > > [RTE_ACL_CLASSIFY_DEFAULT] =3D rte_acl_classify_scalar, > > > [RTE_ACL_CLASSIFY_SCALAR] =3D rte_acl_classify_scalar, > > > [RTE_ACL_CLASSIFY_SSE] =3D rte_acl_classify_sse, > > > +#if (defined __GNUC__ && __GNUC__ <=3D 4 && __GNUC_MINOR__ < 7) > > > + [RTE_ACL_CLASSIFY_AVX2] =3D rte_acl_classify_error, > > > +#else > > > [RTE_ACL_CLASSIFY_AVX2] =3D rte_acl_classify_avx2, > > > +#endif > > > > > > You don't need to do this, you need to use a weak symbol: > > static int rte_acl_classify_avx2(...) __attributes__(weak) > > { > > return -EOPNOTSUP > > } > > > > > > Then in the rte_acl_avx2.c file define it again without the weak symbol > > > > That way, you do conditional compilation, and when you do the "real" sy= mbol > > overrides the weak one. >=20 > Ah yes, you right - not need for ifdef here, thought I still think we nee= d one below, in rte_acl_init(). >=20 > > > > > }; > > > > > > static void __attribute__((constructor)) > > > rte_acl_init(void) > > > { > > > enum rte_acl_classify_alg alg =3D RTE_ACL_CLASSIFY_DEFAULT; > > > > > > +#if (defined __GNUC__ && __GNUC__ <=3D 4 && __GNUC_MINOR__ < 7) > > > if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)) > > > alg =3D RTE_ACL_CLASSIFY_AVX2; > > > else if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1)) > > > +#else > > > + if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1)) > > > alg =3D RTE_ACL_CLASSIFY_SSE; > > > +#endif > > > rte_acl_set_default_classify(alg); > > > } > > > > > Why would you do this, this cpu feature flag definitions aren't matched= to > > compiler support, it should always be defined. >=20 > Because if we don't do this, then on machine that does support AVX2, > we'll always set CLASSIFY_AVX2 as default method, no matter was compiler > able to produce a proper code for it or not. > We should set CLASSIFY_AVX2 as default method only if both conditions ar= e met: > at build time compiler supports AVX2 and target cpu supports AVX2. >=20 > Konstantin >=20 > > You should still be able to > > check for AVX2 support in code that doesn't support emitting the instru= ction. > > > > Neil > > > > > Correct? > > > Konstantin > > > > > > > > > > > > > > > > > > > > > That in turn I think allows you to remove a > > > > > > > > bunch of the ifdeffing that you've done in some of the avx2= specific files. > > > > > > > > > > > > > > Actually there are not many of them. > > > > > > > One in acl_run_avx2.h and another in acl_run_avx2.c. > > > > > > > > > > > > > 2 in acl_run_avx2.h and 1 in rte_acl_osdep_alone.h, which is re= ally 3 more than > > > > > > you need if you just do an intellegent weak classifier function= defintion. > > > > > > > > > > grep -n __AVX2__ lib/librte_acl/*.[c,h] | grep -v endif > > > > > lib/librte_acl/acl_run_avx2.c:45:#ifdef __AVX2__ > > > > > lib/librte_acl/acl_run_avx2.h:36:#ifdef __AVX2__ > > > > > > > > > > rte_acl_osdep_alone.h - is a different story. > > > > > It needs to be there anyway, as in rte_common_vect.h. > > > > > In fact rte_acl_osdep_alone.h is only needed for cases when RTE_= LIBRTE_ACL_STANDALONE=3Dy. > > > > > That comes from the old days, when we had to to support building = librte_acl library without the rest of DPDK. > > > > > I think we don't need it anymore and plan to remove it. > > > > > Just thought it should be in a separate patch. > > > > > Konstantin > > > > > > > > > > > > > > > > > Neil > > > > > > > >