From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6490DA046B; Thu, 9 Jan 2020 16:49:41 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 278031DBA7; Thu, 9 Jan 2020 16:49:41 +0100 (CET) Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id E4D4C1DB96 for ; Thu, 9 Jan 2020 16:49:39 +0100 (CET) Received: from 2606-a000-111b-43ee-0000-0000-0000-115f.inf6.spectrum.com ([2606:a000:111b:43ee::115f] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1ipa3m-0003Qu-U8; Thu, 09 Jan 2020 10:49:31 -0500 Date: Thu, 9 Jan 2020 10:49:22 -0500 From: Neil Horman To: "Wiles, Keith" Cc: Ray Kinsella , dpdk-dev , Jerin Jacob Kollanukkaran , "Richardson, Bruce" , Thomas Monjalon Message-ID: <20200109154922.GB11396@hmswarspite.think-freely.org> References: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Score: -2.9 (--) X-Spam-Status: No 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 Fri, Jun 07, 2019 at 06:21:21PM +0000, Wiles, Keith wrote: > > > > On Jun 7, 2019, at 10:42 AM, Ray Kinsella wrote: > > > > > > > > On 06/06/2019 16:03, Neil Horman wrote: > >> On Thu, Jun 06, 2019 at 02:02:03PM +0000, Jerin Jacob Kollanukkaran wrote: > >>>> -----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 > >>>> > >>>> On Thu, Jun 06, 2019 at 12:04:57PM +0000, Jerin Jacob Kollanukkaran wrote: > >>>>>> -----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 > >>>>>> > >>>>>> 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 > >>>>>>>> > >>>>>>>> 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 > >>>>>>>>>>> > >>>>>>>>>>> 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 > >>>>>>>>>>> > >>>>>>>>>>> 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 > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> 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. > >>>>>>>>>>> > >>>>>>>>>>> This isn't a perfect solution, as applications can still > >>>>>>>>>>> hack around it of course, > >>>>>>>>>> > >>>>>>>>>> I think, one way to, avoid, hack around could be to, > >>>>>>>>>> > >>>>>>>>>> 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. > >>>>>>>>>> > >>>>>>>>> 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. > >>>>>>>>> > >>>>>>>> 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. > >>>>>>> > >>>>>>> I agree too. IMHO, Simply having following items would be enough > >>>>>>> > >>>>>>> 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. > >>>>>>> > >>>>>> > >>>>>> 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 > >>>>>> > >>>>>> 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. > >>>>>> > >>>>>> 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 > >>>>> > >>>>> Enforcing the naming convention can be achieved through tooling as well. > >>>>> > >>>> Sure, but why enforce any function naming at all, when you don't have to. > >>> > >>> May I ask, why to enforce __rte_internal, when you don't have to > >>> > >> > >> Because its more clear. Implicitly deciding that any function not prefixed with > >> rte_ is internal only does nothing to prevent a developer from accidentally > >> 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 users, and > >> developers should have a way to clearly record that without having to check the > >> documentation for each function that an application developer wants to use. > >> > >> 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 such > >> 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 than having > >> to ask questions on the list, or read documentation after an error to find out > >> "oops, shouldn't have done that". > >> > >> I think you'll find that going through all the header files, and bifurcating > >> them to public and private headers is a much larger undertaking than just > >> tagging those functions accordingly. a quick scan of all our header file for > >> the @internal tag shows about 260 instances of such functions, almost all 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 updated > >> 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. > >> > >> Neil > > > > 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. > > > > 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. > > > > 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. > > > > 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 measure. I am still concerned we are adding more work for the developer, if not then at least we split the headers. I think this makes sense. Perhaps we could add a check in checkpatch to warn a user if the __rte_internal tag is present in a header that has been copied to the builds include directory (i.e. was specified as SYMLINK-$(VAR) in the makefile). Would that help? Neil