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 97248A0096 for ; Thu, 6 Jun 2019 15:18:33 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4A70B1B95A; Thu, 6 Jun 2019 15:18:33 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id B0E091B958 for ; Thu, 6 Jun 2019 15:18:30 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jun 2019 06:18:29 -0700 X-ExtLoop1: 1 Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 06 Jun 2019 06:18:29 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 6 Jun 2019 06:18:29 -0700 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.141]) by fmsmsx111.amr.corp.intel.com ([169.254.12.164]) with mapi id 14.03.0415.000; Thu, 6 Jun 2019 06:18:29 -0700 From: "Wiles, Keith" To: Jerin Jacob Kollanukkaran CC: Neil Horman , "Richardson, Bruce" , "dev@dpdk.org" , Thomas Monjalon Thread-Topic: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag Thread-Index: AQHVHGAeTHy0x+iqgUew6BEqMONpAqaPEUAA Date: Thu, 6 Jun 2019 13:18:29 +0000 Message-ID: <015190E6-5703-40E2-9CBE-DAB3DA74669B@intel.com> 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> In-Reply-To: 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: 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 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 wrot= e: >>>> -----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 function= s >> 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 corre= late >> the code implementing those functions to the documentation. Its valuabl= e >> 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 cognisc= ent >> of the convention and make the appropriate naming choices. It also impl= icitly >> restricts the abliity for future developers to make naming changes in co= nflict >> 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 on= ly are >> internal functions excused from ABI constraints, but forces developers t= o >> intentionally mark their internal functions as being internal in the cod= e, which >> is beneficial to clarlity of understanding during the development proces= s. >=20 > No issues in adding __rte_internal. But, I am against current implementai= on,=20 > Ie. adding objdump dependency > to checkpatch i.e developer has to build the library first so that check= patch 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 t= o 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. I feel like we are creating a lot of extra work for the developer and addin= g a number of constraints to getting code patches submitted as the tools al= l have to be working together. The versioning file and __rte_experimental s= tuff today has always being handle wrong or not done by the developer. Alte= ring the tools to detect these changes works and it seemed to take a while = to iron out. To me we should be doing the minimum steps to reasonably isola= te internal API and data from the user. If someone wants to access the thos= e APIs that is their choice and enforcing with new macros and tools is over= kill IMHO. 1) Adding @internal to documentation is a great start along with more docs = to explain what internal functions/data should be handled. 2) Hiding/moving internal function prototypes in private headers. 3) Adding setters/getters for internal data. 4) Make sure we review and reject direct use of internal functions and data= . The goal here is to handle the 80% rule and make it very obvious to the dev= eloper these are internal functions and data. Using these or similar minimu= m 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 Regards, Keith