From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id E4046B373 for ; Thu, 28 Aug 2014 11:00:40 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 28 Aug 2014 02:04:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,416,1406617200"; d="scan'208";a="591032908" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga002.fm.intel.com with ESMTP; 28 Aug 2014 02:04:34 -0700 Received: from irsmsx154.ger.corp.intel.com (163.33.192.96) by IRSMSX101.ger.corp.intel.com (163.33.3.153) with Microsoft SMTP Server (TLS) id 14.3.195.1; Thu, 28 Aug 2014 10:02:20 +0100 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.112]) by IRSMSX154.ger.corp.intel.com ([169.254.12.233]) with mapi id 14.03.0195.001; Thu, 28 Aug 2014 10:02:20 +0100 From: "Richardson, Bruce" To: "Ananyev, Konstantin" , Neil Horman Thread-Topic: [dpdk-dev] [PATCHv3] librte_acl make it build/work for 'default' target Thread-Index: AQHPwIInS14YAZ2RiU++wyqji/CLfZvjGQ6AgAEoQgCAAH4+AIAABhkAgADx6wA= Date: Thu, 28 Aug 2014 09:02:19 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B0343ECB59@IRSMSX103.ger.corp.intel.com> References: <1407436263-9360-1-git-send-email-konstantin.ananyev@intel.com> <1408652100-29217-1-git-send-email-nhorman@tuxdriver.com> <2601191342CEEE43887BDE71AB9772582135D369@IRSMSX105.ger.corp.intel.com> <20140826174443.GB797@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB9772582135DC13@IRSMSX105.ger.corp.intel.com> <20140827185653.GA31916@localhost.localdomain> <2601191342CEEE43887BDE71AB9772582135DD43@IRSMSX105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772582135DD43@IRSMSX105.ger.corp.intel.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCHv3] librte_acl make it build/work for 'default' target 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: Thu, 28 Aug 2014 09:00:41 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin > Sent: Wednesday, August 27, 2014 8:19 PM > To: Neil Horman > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCHv3] librte_acl make it build/work for 'defa= ult' > target >=20 >=20 >=20 > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > Sent: Wednesday, August 27, 2014 7:57 PM > > To: Ananyev, Konstantin > > Cc: dev@dpdk.org; thomas.monjalon@6wind.com > > Subject: Re: [PATCHv3] librte_acl make it build/work for 'default' targ= et > > > > On Wed, Aug 27, 2014 at 11:25:04AM +0000, Ananyev, Konstantin wrote: > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > Sent: Tuesday, August 26, 2014 6:45 PM > > > > To: Ananyev, Konstantin > > > > Cc: dev@dpdk.org; thomas.monjalon@6wind.com > > > > Subject: Re: [PATCHv3] librte_acl make it build/work for 'default' = target > > > > > > > > On Mon, Aug 25, 2014 at 04:30:05PM +0000, Ananyev, Konstantin wrote= : > > > > > Hi Neil, > > > > > > > > > > > -----Original Message----- > > > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > > > Sent: Thursday, August 21, 2014 9:15 PM > > > > > > To: dev@dpdk.org > > > > > > Cc: Ananyev, Konstantin; thomas.monjalon@6wind.com; Neil Horman > > > > > > Subject: [PATCHv3] librte_acl make it build/work for 'default' = target > > > > > > > > > > > > Make ACL library to build/work on 'default' architecture: > > > > > > - make rte_acl_classify_scalar really scalar > > > > > > (make sure it wouldn't use sse4 instrincts through resolve_pri= ority()). > > > > > > - Provide two versions of rte_acl_classify code path: > > > > > > rte_acl_classify_sse() - could be build and used only on syst= ems with > sse4.2 > > > > > > and upper, return -ENOTSUP on lower arch. > > > > > > rte_acl_classify_scalar() - a slower version, but could be bu= ild and used > > > > > > on all systems. > > > > > > - keep common code shared between these two codepaths. > > > > > > > > > > > > v2 chages: > > > > > > run-time selection of most appropriate code-path for given ISA= . > > > > > > By default the highest supprted one is selected. > > > > > > User can still override that selection by manually assigning n= ew value > to > > > > > > the global function pointer rte_acl_default_classify. > > > > > > rte_acl_classify() becomes a macro calling whatever > rte_acl_default_classify > > > > > > points to. > > > > > > > > > > > > > > > > I see you decided not to wait for me and fix everything by yourse= lf :) > > > > > > > > > Yeah, sorry, I'm getting pinged about enabling these features in Fe= dora, > and it > > > > had been about 2 weeks, so I figured I'd just take care of it. > > > > > > No worries. I admit that it was a long delay from my side. > > > > > > > > > > > > > V3 Changes > > > > > > Updated classify pointer to be a function so as to better pres= erve ABI > > > > > > > > > > As I said in my previous mail it generates extra jump... > > > > > Though from numbers I got the performance impact is negligible: <= 1%. > > > > > So I suppose, I don't have a good enough reason to object :) > > > > > > > > > Yeah, I just don't see a way around it. I was hoping that the comp= iler > would > > > > have been smart enough to see that the rte_acl_classify function wa= s small > and > > > > in-linable, but apparently it won't do that. As you note however t= he > > > > performance change is minor (I'm guessing within a standard deviati= on of > your > > > > results). > > > > > > > > > Though I still think we better keep rte_acl_classify_scalar() pu= blically > available (same as we do for rte acl_classify_sse()): > > > > > First of all keep rte_acl_classify_scalar() is already part of o= ur public API. > > > > > Also, as I remember, one of the customers explicitly asked for sc= alar > version and they planned to call it directly. > > > > > Plus using rte_acl_select_classify() to always switch between > implementations is not always handy: > > > > > > > > I'm not exactly opposed to this, though it seems odd to me that a u= ser > might > > > > want to call a particular version of the classifier directly. But = I certainly > > > > can't predict everything a consumer wants to do. If we really need= to keep > it > > > > public then, it begs the question, is providing a generic entry poi= nt even > > > > worthwhile? Is it just as easy to expose the scalar/sse and any fu= ture > versions > > > > directly so the application can just embody the intellegence to sel= ect the > best > > > > path? That saves us having to maintain another API point. I can g= o with > > > > consensus on that. > > > > > > > > > - it is global, which means that we can't simultaneously use > classify_scalar() and classify_sse() for 2 different ACL contexts. > > > > > - to properly support such switching we then will need to support > something like (see app/test/test_acl.c below): > > > > > old_alg =3D rte_acl_get_classify(); > > > > > rte_acl_select_classify(new_alg); > > > > > ... > > > > > rte_acl_select_classify(old_alg); > > > > > > > > > We could attach the classification method to the acl context, so ea= ch > > > > rte_acl_ctx can point to whatever classifier funtion it wants to. = That would > > > > remove the global issues you point out above. > > > > > > I thought about that approach too. > > > But there is one implication with DPDK MP model: > > > Same ACL context can be shared by different DPDK processes, > > > while acl_classify() could be loaded to the different addresses. > > > Of course we can overcome it by creating a global table of function p= ointers > indexed by calssify_alg and > > > store inside ACL ctx alg instead of actual function pointer. > > > But that means extra overhead of at least two loads per classify() ca= ll. > > > > > Hmm, how is the context shared around between processes? Is it just sh= ared > as a > > common cow data page resulting from a fork? If so, then we should be g= ood > > because the DSO text will be at the same address (i.e. the pointer will= still be > > valid). If you do some sort of message passing, then, yes, thats a pro= blem. > > >=20 > No, it is not parent-child relationship. > There could be a group of independently spawned processes. > One of them should be 'primary' (starts first), other 'secondary's'. > All hugepage memory pages mapped by the primary process, supposed to be > mapped to the same VAs by each secondary. > So all stuff that is allocated from hugepage memory is shared between all > processes in the group. > More detailed description: http://dpdk.org/doc/intel/dpdk-prog-guide- > 1.7.0.pdf, section 23. >=20 Function pointers just don't work easily with multiprocess. Again some his= tory, since today seems to be my Intel DPDK history day... For the PMDs, originally we allowed NIC access only by the primary process,= but later removed that limitation by having the secondary processes do a d= river load and pci scan on startup, and having the ethdev structure split b= etween the function pointer part which is not shared and configured indepen= dently in the secondary process as part of the pci scan, and the data part = which is in hugepage memory and is shared across all processes. For the has= h library, we needed a different approach and we looked at having tables of= functions, but discarded the idea as largely unworkable when we took user-= specified functions into account. What we ended up doing was provide separa= te api's to call the add/delete/lookup function with a pre-computed hash, s= o that multi-process apps could explicitly call the hash function without u= sing a fn pointer and then pass in the computed value to the rest of the AP= I calls. Apologies for the digression from the immediate topic at hand, but I think = it's something that is good to make people generally aware of when working = with DPDK libs. Regards, /Bruce