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 91142A0096 for ; Thu, 6 Jun 2019 15:53:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 32B331B95A; Thu, 6 Jun 2019 15:53:51 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id A8B4E1B959 for ; Thu, 6 Jun 2019 15:53:49 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jun 2019 06:53:48 -0700 X-ExtLoop1: 1 Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 06 Jun 2019 06:53:48 -0700 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 6 Jun 2019 06:53:48 -0700 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.141]) by FMSMSX126.amr.corp.intel.com ([169.254.1.74]) with mapi id 14.03.0415.000; Thu, 6 Jun 2019 06:53:48 -0700 From: "Wiles, Keith" To: Neil Horman CC: Jerin Jacob Kollanukkaran , "Richardson, Bruce" , "dev@dpdk.org" , Thomas Monjalon Thread-Topic: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag Thread-Index: AQHVHGAeTHy0x+iqgUew6BEqMONpAqaPEUAAgAAG2wCAAAMCAA== Date: Thu, 6 Jun 2019 13:53:47 +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> <015190E6-5703-40E2-9CBE-DAB3DA74669B@intel.com> <20190606134300.GC29521@hmswarspite.think-freely.org> In-Reply-To: <20190606134300.GC29521@hmswarspite.think-freely.org> 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: <9C29CE9A38D9F94D9659F5706838A77C@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 6, 2019, at 8:43 AM, Neil Horman wrote: >=20 > On Thu, Jun 06, 2019 at 01:18:29PM +0000, Wiles, Keith wrote: >>=20 >>=20 >>> On Jun 6, 2019, at 7:04 AM, Jerin Jacob Kollanukkaran wrote: >>>=20 >>>> -----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 wr= ote: >>>>>> -----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 sto= p >>>> 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 functi= ons >>>> 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 cor= relate >>>> the code implementing those functions to the documentation. Its valua= ble >>>> to have a tag on a function identifying it as internal only. >>>>=20 >>>> 3) Using naming conventions to separate internal only from non-interna= l >>>> functions is a vague approach, requiring future developers to be cogni= scent >>>> of the convention and make the appropriate naming choices. It also im= plicitly >>>> 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 well= . >>>=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 c= ode, which >>>> is beneficial to clarlity of understanding during the development proc= ess. >>>=20 >>> No issues in adding __rte_internal. But, I am against current implement= aion,=20 >>> Ie. adding objdump dependency >>> to checkpatch i.e developer has to build the library first so that che= ckpatch can >>> can know, Is it belongs to internal section or not? >>>=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 constitute= s >>>> an internal symbol rather than relying on naming conventions and hopin= g >>>> that developers read the documentation before exporting a symbol >>>> publically. >>=20 >> I feel like we are creating a lot of extra work for the developer and ad= ding a number of constraints to getting code patches submitted as the tools= all have to be working together. The versioning file and __rte_experimenta= l stuff today has always being handle wrong or not done by the developer. A= ltering the tools to detect these changes works and it seemed to take a whi= le to iron out. To me we should be doing the minimum steps to reasonably is= olate internal API and data from the user. If someone wants to access the t= hose APIs that is their choice and enforcing with new macros and tools is o= ver kill IMHO. >>=20 >> 1) Adding @internal to documentation is a great start along with more do= cs to explain what internal functions/data should be handled. > I've got no issue with this >> 2) Hiding/moving internal function prototypes in private headers. > Nor this, though if you want to do it, ensuring that every library has a = private > header that doesn't get exported has to be part of the review process, an= d I'm > not confident that that will be a focus of anyones review >=20 >> 3) Adding setters/getters for internal data. > Sure, no issue here >> 4) Make sure we review and reject direct use of internal functions and d= ata. > How exactly do you intend to enforce this? By definition, the use of int= ernal > functions is restricted only to dpdk core libraries, and those are the on= ly > patches we ever see on the list. This change is meant to enforce usage > prevention for users writing applications whos patches will never be seen= for > review (save for our example programs) >=20 > We already have this mechanism available for experimental abi, and from m= y > perspective it works quite well, and is fairly well understood. I don't = see > whats wrong with expanding that mechanism to internal functions. On this case I was talking more about accessing internal data directly inst= ead of using getters/setters. I understand your other points, but if we use the items above it should sol= ve most of the issues. Adding another tag and then adding tools to make it = work is just going a bit too far. The reason that experimental works is nor= mally the developer only has a few APIs to mark and it works along with ver= sioning you added. The versioning and experimental parts are kind of a requ= irement with shared libs and identifying experimental APIs. In the case of = marking all of the APIs as internal could be a huge list for all of the lib= s, we should move them to private headers instead. >=20 > Neil >=20 >>=20 >> The goal here is to handle the 80% rule and make it very obvious to the = developer these are internal functions and data. Using these or similar min= imum steps should be reasonable IMHO. >>>>=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 here. >>>>>>=20 >>>>>> Best >>>>>> Neil >>>>>>=20 >>>>>>> /Bruce >>=20 >> Regards, >> Keith Regards, Keith