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 205F0A0096 for ; Fri, 7 Jun 2019 20:21:30 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 13B8A1BB92; Fri, 7 Jun 2019 20:21:29 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 931CF1BB91 for ; Fri, 7 Jun 2019 20:21:26 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Jun 2019 11:21:25 -0700 X-ExtLoop1: 1 Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga006.jf.intel.com with ESMTP; 07 Jun 2019 11:21:24 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 7 Jun 2019 11:21:23 -0700 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.248]) by fmsmsx122.amr.corp.intel.com ([169.254.5.41]) with mapi id 14.03.0415.000; Fri, 7 Jun 2019 11:21:23 -0700 From: "Wiles, Keith" To: Ray Kinsella CC: dpdk-dev , Neil Horman , "Jerin Jacob Kollanukkaran" , "Richardson, Bruce" , Thomas Monjalon Thread-Topic: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag Thread-Index: AQHVHHB97aIDBKYr+Ump4lVIDa17J6aPLpUAgAGc/YCAACyCgA== Date: Fri, 7 Jun 2019 18:21:21 +0000 Message-ID: References: <20190525184346.27932-1-nhorman@tuxdriver.com> <20190605164541.GH1550@bricha3-MOBL.ger.corp.intel.com> <20190605181108.GC554@hmswarspite.think-freely.org> <20190606113422.GA29521@hmswarspite.think-freely.org> <20190606133503.GB29521@hmswarspite.think-freely.org> <20190606150354.GF29521@hmswarspite.think-freely.org> <3ab2fa96-e07b-b6c8-3a03-1b6ad2d5bb04@ashroe.eu> In-Reply-To: <3ab2fa96-e07b-b6c8-3a03-1b6ad2d5bb04@ashroe.eu> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.255.75.4] Content-Type: text/plain; charset="us-ascii" Content-ID: <68DA62054433A943A898D659F163D3BF@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag 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" > On Jun 7, 2019, at 10:42 AM, Ray Kinsella wrote: >=20 >=20 >=20 > On 06/06/2019 16:03, Neil Horman wrote: >> On Thu, Jun 06, 2019 at 02:02:03PM +0000, Jerin Jacob Kollanukkaran wrot= e: >>>> -----Original Message----- >>>> From: Neil Horman >>>> Sent: Thursday, June 6, 2019 7:05 PM >>>> To: Jerin Jacob Kollanukkaran >>>> Cc: Bruce Richardson ; dev@dpdk.org; >>>> Thomas Monjalon >>>> Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag >>>>=20 >>>> On Thu, Jun 06, 2019 at 12:04:57PM +0000, Jerin Jacob Kollanukkaran wr= ote: >>>>>> -----Original Message----- >>>>>> From: Neil Horman >>>>>> Sent: Thursday, June 6, 2019 5:04 PM >>>>>> To: Jerin Jacob Kollanukkaran >>>>>> Cc: Bruce Richardson ; dev@dpdk.org; >>>>>> Thomas Monjalon >>>>>> Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag >>>>>>=20 >>>>>> On Thu, Jun 06, 2019 at 09:44:52AM +0000, Jerin Jacob Kollanukkaran >>>> wrote: >>>>>>>> -----Original Message----- >>>>>>>> From: Neil Horman >>>>>>>> Sent: Wednesday, June 5, 2019 11:41 PM >>>>>>>> To: Bruce Richardson >>>>>>>> Cc: Jerin Jacob Kollanukkaran ; >>>>>>>> dev@dpdk.org; Thomas Monjalon >>>>>>>> Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag >>>>>>>>=20 >>>>>>>> On Wed, Jun 05, 2019 at 05:45:41PM +0100, Bruce Richardson wrote: >>>>>>>>> On Wed, Jun 05, 2019 at 04:24:09PM +0000, Jerin Jacob >>>>>>>>> Kollanukkaran >>>>>>>> wrote: >>>>>>>>>>> -----Original Message----- >>>>>>>>>>> From: Neil Horman >>>>>>>>>>> Sent: Sunday, May 26, 2019 12:14 AM >>>>>>>>>>> To: dev@dpdk.org >>>>>>>>>>> Cc: Neil Horman ; Jerin Jacob >>>>>>>>>>> Kollanukkaran ; Bruce Richardson >>>>>>>>>>> ; Thomas Monjalon >>>>>>>>>>> >>>>>>>>>>> Subject: [EXT] [RFC PATCH 0/2] introduce __rte_internal >>>>>>>>>>> tag >>>>>>>>>>>=20 >>>>>>>>>>> Hey- >>>>>>>>>>> Based on our recent conversations regarding the use of >>>>>>>>>>> symbols only meant for internal dpdk consumption (between >>>>>>>>>>> dpdk libraries), this is an idea that I've come up with >>>>>>>>>>> that I'd like to get some feedback on >>>>>>>>>>>=20 >>>>>>>>>>> Summary: >>>>>>>>>>> 1) We have symbols in the DPDK that are meant to be used >>>>>>>>>>> between DPDK libraries, but not by applications linking to >>>>>>>>>>> them >>>>>>>>>>> 2) We would like to document those symbols in the code, so >>>>>>>>>>> as to note them clearly as for being meant for internal >>>>>>>>>>> use only >>>>>>>>>>> 3) Linker symbol visibility is a very coarse grained tool, >>>>>>>>>>> and so there is no good way in a single library to mark >>>>>>>>>>> items as being meant for use only by other DPDK libraries, >>>>>>>>>>> at least not without some extensive runtime checking >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>> Proposal: >>>>>>>>>>> I'm proposing that we introduce the __rte_internal tag. >>>>>>>>>>> From a coding standpoint it works a great deal like the >>>>>>>>>>> __rte_experimental tag in that it expempts the tagged >>>>>>>>>>> symbol from ABI constraints (as the only users should be >>>>>>>>>>> represented in the DPDK build environment). Additionally, >>>>>>>>>>> the __rte_internal macro resolves differently based on the >>>>>>>>>>> definition of the BUILDING_RTE_SDK flag (working under the >>>>>>>>>>> assumption that said flag should only ever be set if we >>>>>>>>>>> are actually building DPDK libraries which will make use >>>>>>>>>>> of internal calls). If the BUILDING_RTE_SDK flag is set >>>>>>>>>>> __rte_internal resolves to __attribute__((section >>>>>>>>>>> "text.internal)), placing it in a special text section >>>>>>>>>>> which is then used to validate that the the symbol appears >>>>>>>>>>> in the INTERNAL section of the corresponding library version >>>> map). >>>>>>>>>>> If BUILDING_RTE_SDK is not set, then __rte_internal >>>>>>>>>>> resolves to >>>>>>>> __attribute__((error("..."))), which causes any caller of the >>>>>>>> tagged function to throw an error at compile time, indicating >>>>>>>> that the symbol is not available for external use. >>>>>>>>>>>=20 >>>>>>>>>>> This isn't a perfect solution, as applications can still >>>>>>>>>>> hack around it of course, >>>>>>>>>>=20 >>>>>>>>>> I think, one way to, avoid, hack around could be to, >>>>>>>>>>=20 >>>>>>>>>> 1) at config stage, create a random number for the build >>>>>>>>>> 2) introduce RTE_CALL_INTERNAL macro for calling internal >>>>>>>>>> function, compare the generated random number for allowing >>>>>>>>>> the calls to make within the library. i.e leverage the fact >>>>>>>>>> that external library would never know the random number >>>>>>>>>> generated for the DPDK build >>>>>>>> and internal driver code does. >>>>>>>>>>=20 >>>>>>>>> Do we really need to care about this. If have some determined >>>>>>>>> enough to hack around our limitations, then they surely know >>>>>>>>> that they have an unsupported configuration. We just need to >>>>>>>>> protect against inadvertent use of internals, IMHO. >>>>>>>>>=20 >>>>>>>> I agree, I too had thought about doing some sort of internal >>>>>>>> runtime checking to match internal only symbols, such that they >>>>>>>> were only accessable by internally approved users, but it >>>>>>>> started to feel like a great >>>>>> deal of overhead. >>>>>>>> Its a good idea for a general mechanism I think, but I believe >>>>>>>> the value here is more to internally document which apis we want >>>>>>>> to mark as being for internal use only, and create a lightweight >>>>>>>> roadblock at build time to catch users inadvertently using them. >>>>>>>> Determined users will get around anything, and theres not much >>>>>>>> we can do to stop >>>>>> them. >>>>>>>=20 >>>>>>> I agree too. IMHO, Simply having following items would be enough >>>>>>>=20 >>>>>>> 1) Avoid exposing the internal function prototype through public >>>>>>> header files >>>>>>> 2) Add @internal to API documentation >>>>>>> 3) Just decide the name space for internal API for tooling(i.e not >>>>>>> start with rte_ or so) Using objdump scheme to detect internal >>>>>>> functions >>>>>> requires the the library to build prior to run the checkpatch. >>>>>>>=20 >>>>>>=20 >>>>>> No, I'm not comfortable with that approach, and I've stated why: >>>>>> 1) Not exposing the functions via header files is a fine start >>>>>>=20 >>>>>> 2) Adding internal documentation is also fine, but does nothing to >>>>>> correlate the code implementing those functions to the >>>>>> documentation. Its valuable to have a tag on a function identifying= it as >>>> internal only. >>>>>>=20 >>>>>> 3) Using naming conventions to separate internal only from >>>>>> non-internal functions is a vague approach, requiring future >>>>>> developers to be cogniscent of the convention and make the >>>>>> appropriate naming choices. It also implicitly restricts the >>>>>> abliity for future developers to make naming changes in conflict >>>>>> with that convention >>>>>=20 >>>>> Enforcing the naming convention can be achieved through tooling as we= ll. >>>>>=20 >>>> Sure, but why enforce any function naming at all, when you don't have = to. >>>=20 >>> May I ask, why to enforce __rte_internal, when you don't have to >>>=20 >>=20 >> Because its more clear. Implicitly deciding that any function not prefi= xed with >> rte_ is internal only does nothing to prevent a developer from accidenta= lly >> naming a function incorrectly, exporting it, and allowing a user to call= it. We >> can move headers all you want, but we provide an ABI guarantee to end us= ers, and >> developers should have a way to clearly record that without having to ch= eck the >> documentation for each function that an application developer wants to u= se. >>=20 >> The long and the short of it for me is that I want a way for developers = to opt >> their code into an internal only condition, not to just document it as s= uch >> and hope its up to date. If they tag a function as __rte_internal then = its >> clearly marked as internal only, they have checks to ensure that its in = the >> INTERNAL section of the version map, and should that header somehow get >> externally exported (see rte_mempool_check_cookies for an example of how= thats >> happened), users are prevented from using them at build time, rather tha= n having >> to ask questions on the list, or read documentation after an error to fi= nd out >> "oops, shouldn't have done that". >>=20 >> I think you'll find that going through all the header files, and bifurca= ting >> them to public and private headers is a much larger undertaking than jus= t >> tagging those functions accordingly. a quick scan of all our header fil= e for >> the @internal tag shows about 260 instances of such functions, almost al= l of >> which are published to applications. All of those functions would have = to be >> moved to private headers, and their requisite C files would need to be u= pdated >> to include the new header. with the use of __rte_internal, we just have= tag the >> functions as such, which can be handled with a cocinelle or awk script. >>=20 >> Neil >=20 > This is good, I like alot about this, especially the build system > complaining loudly when the developer does something they shouldn't - I > think anything that we can add that promotes good behaviors is to be > 100% welcomed. >=20 > I also agree with the points made elsewhere that this is essentially > trying to solve a header problem, the mixing of public and private > symbols in what are public headers, with __rte_internal. Adding > __rte_internal would essentially ratify that behavior, whereas I would > argue that something inherently private, should never see the light of > day in a public header. >=20 > I completely get that it may be more work, however for me it is better > way to fix this problem. It would also add completely clarity, all the > extra hassle around does it have the rte_ prefix goes away - if it is in > a "public header" it is part of the ABI/API, end of discussion. >=20 > Finally, not opposed to also asking folks putting symbols in the private > header to mark those symbols with __rte_internal. +1 I think we need to do both split headers and __rte_internal for extra me= asure. I am still concerned we are adding more work for the developer, if n= ot then at least we split the headers. >=20 >>=20 >>=20 >>>>=20 >>>>>>=20 >>>>>> 4) Adding a tag like __rte_internal creates an interlock whereby, >>>>>> not only are internal functions excused from ABI constraints, but >>>>>> forces developers to intentionally mark their internal functions as >>>>>> being internal in the code, which is beneficial to clarlity of under= standing >>>> during the development process. >>>>>=20 >>>>> No issues in adding __rte_internal. But, I am against current >>>>> implementaion, Ie. adding objdump dependency >>>> That dependency already exists for the __rte_external flag >>>=20 >>> Sorry, I could not see the dependency. >>>=20 >>> [master][dpdk.org] $ grep -ri "objdump" devtools/ >>> [master][dpdk.org] $ grep -ri "objdump" usertools/ >>> [master][dpdk.org] $ grep -ri "__rte_external" * >>>=20 >>>>=20 >>>>> to checkpatch i.e developer has to build the library first so that >>>>> checkpatch can can know, Is it belongs to internal section or not? >>>>>=20 >>>> What developer is running checkpatch/posting patches without first bui= lding >>>> their changes? >>>=20 >>> # it is not developer, The CI/CD tools can quicky check the sanity of p= atches >>> before the build itself. Why to add unnecessary dependency? >>> # If some PMD is not building if the requirements are not meet(say AES = NI PMD for crypto) >>> then how do take care of the dependency. >>>=20 >>>=20 >>>>=20 >>>>=20 >>>>>>=20 >>>>>> 5) Adding a tag like __rte_internal is explicit, and allows >>>>>> developers to use a single header file instead of multiple header >>>>>> files if they so choose >>>>>>=20 >>>>>> We went through this with experimental symbols as well[1], and it >>>>>> just makes more sense to me to clearly document in the code what >>>>>> constitutes an internal symbol rather than relying on naming >>>>>> conventions and hoping that developers read the documentation before >>>>>> exporting a symbol publically. >>>>>>=20 >>>>>>=20 >>>>>> [1] https://mails.dpdk.org/archives/dev/2017-December/083828.html >>>>>>>>=20 >>>>>>>> If we really wanted to go down that road, we could use a >>>>>>>> mechainsm simmilar to the EXPORT_SYMBOL / EXPORT_SYMBOL_GPL >>>>>>>> infrastructure that the kernel uses, but that would required >>>>>>>> building our own custom linker script, which seems like overkill h= ere. >>>>>>>>=20 >>>>>>>> Best >>>>>>>> Neil >>>>>>>>=20 >>>>>>>>> /Bruce Regards, Keith