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 E66FBA0096 for ; Fri, 7 Jun 2019 17:42:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BB63A1BB32; Fri, 7 Jun 2019 17:42:18 +0200 (CEST) Received: from qrelay184.mxroute.com (qrelay184.mxroute.com [172.82.139.184]) by dpdk.org (Postfix) with ESMTP id 728DA1BAA7 for ; Fri, 7 Jun 2019 17:42:17 +0200 (CEST) Received: from filter002.mxroute.com (unknown [94.130.183.33]) by qrelay184.mxroute.com (Postfix) with ESMTP id 90373160507; Fri, 7 Jun 2019 11:42:16 -0400 (EDT) Received: from galaxy.mxroute.com (unknown [23.92.70.113]) by filter002.mxroute.com (Postfix) with ESMTPS id B43A53F0D4; Fri, 7 Jun 2019 15:42:10 +0000 (UTC) Received: from jfdmzpr04-ext.jf.intel.com ([134.134.139.73]) by galaxy.mxroute.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.91) (envelope-from ) id 1hZH0L-0001CF-DP; Fri, 07 Jun 2019 11:42:10 -0400 From: Ray Kinsella To: dev@dpdk.org, Neil Horman , Jerin Jacob Kollanukkaran , bruce.richardson@intel.com, Thomas Monjalon , Keith 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> Openpgp: preference=signencrypt Autocrypt: addr=mdr@ashroe.eu; keydata= mQINBFv8B3wBEAC+5ImcgbIvadt3axrTnt7Sxch3FsmWTTomXfB8YiuHT8KL8L/bFRQSL1f6 ASCHu3M89EjYazlY+vJUWLr0BhK5t/YI7bQzrOuYrl9K94vlLwzD19s/zB/g5YGGR5plJr0s JtJsFGEvF9LL3e+FKMRXveQxBB8A51nAHfwG0WSyx53d61DYz7lp4/Y4RagxaJoHp9lakn8j HV2N6rrnF+qt5ukj5SbbKWSzGg5HQF2t0QQ5tzWhCAKTfcPlnP0GymTBfNMGOReWivi3Qqzr S51Xo7hoGujUgNAM41sxpxmhx8xSwcQ5WzmxgAhJ/StNV9cb3HWIoE5StCwQ4uXOLplZNGnS uxNdegvKB95NHZjRVRChg/uMTGpg9PqYbTIFoPXjuk27sxZLRJRrueg4tLbb3HM39CJwSB++ YICcqf2N+GVD48STfcIlpp12/HI+EcDSThzfWFhaHDC0hyirHxJyHXjnZ8bUexI/5zATn/ux TpMbc/vicJxeN+qfaVqPkCbkS71cHKuPluM3jE8aNCIBNQY1/j87k5ELzg3qaesLo2n1krBH bKvFfAmQuUuJT84/IqfdVtrSCTabvDuNBDpYBV0dGbTwaRfE7i+LiJJclUr8lOvHUpJ4Y6a5 0cxEPxm498G12Z3NoY/mP5soItPIPtLR0rA0fage44zSPwp6cQARAQABtBxSYXkgS2luc2Vs bGEgPG1kckBhc2hyb2UuZXU+iQJUBBMBCAA+FiEEcDUDlKDJaDuJlfZfdJdaH/sCCpsFAlv8 B3wCGyMFCQlmAYAFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdJdaH/sCCptdtRAAl0oE msa+djBVYLIsax+0f8acidtWg2l9f7kc2hEjp9h9aZCpPchQvhhemtew/nKavik3RSnLTAyn B3C/0GNlmvI1l5PFROOgPZwz4xhJKGN7jOsRrbkJa23a8ly5UXwF3Vqnlny7D3z+7cu1qq/f VRK8qFyWkAb+xgqeZ/hTcbJUWtW+l5Zb+68WGEp8hB7TuJLEWb4+VKgHTpQ4vElYj8H3Z94a 04s2PJMbLIZSgmKDASnyrKY0CzTpPXx5rSJ1q+B1FCsfepHLqt3vKSALa3ld6bJ8fSJtDUJ7 JLiU8dFZrywgDIVme01jPbjJtUScW6jONLvhI8Z2sheR71UoKqGomMHNQpZ03ViVWBEALzEt TcjWgJFn8yAmxqM4nBnZ+hE3LbMo34KCHJD4eg18ojDt3s9VrDLa+V9fNxUHPSib9FD9UX/1 +nGfU/ZABmiTuUDM7WZdXri7HaMpzDRJUKI6b+/uunF8xH/h/MHW16VuMzgI5dkOKKv1LejD dT5mA4R+2zBS+GsM0oa2hUeX9E5WwjaDzXtVDg6kYq8YvEd+m0z3M4e6diFeLS77/sAOgaYL 92UcoKD+Beym/fVuC6/55a0e12ksTmgk5/ZoEdoNQLlVgd2INtvnO+0k5BJcn66ZjKn3GbEC VqFbrnv1GnA58nEInRCTzR1k26h9nmS5Ag0EW/wHfAEQAMth1vHr3fOZkVOPfod3M6DkQir5 xJvUW5EHgYUjYCPIa2qzgIVVuLDqZgSCCinyooG5dUJONVHj3nCbITCpJp4eB3PI84RPfDcC hf/V34N/Gx5mTeoymSZDBmXT8YtvV/uJvn+LvHLO4ZJdvq5ZxmDyxfXFmkm3/lLw0+rrNdK5 pt6OnVlCqEU9tcDBezjUwDtOahyV20XqxtUttN4kQWbDRkhT+HrA9WN9l2HX91yEYC+zmF1S OhBqRoTPLrR6g4sCWgFywqztpvZWhyIicJipnjac7qL/wRS+wrWfsYy6qWLIV80beN7yoa6v ccnuy4pu2uiuhk9/edtlmFE4dNdoRf7843CV9k1yRASTlmPkU59n0TJbw+okTa9fbbQgbIb1 pWsAuicRHyLUIUz4f6kPgdgty2FgTKuPuIzJd1s8s6p2aC1qo+Obm2gnBTduB+/n1Jw+vKpt 07d+CKEKu4CWwvZZ8ktJJLeofi4hMupTYiq+oMzqH+V1k6QgNm0Da489gXllU+3EFC6W1qKj tkvQzg2rYoWeYD1Qn8iXcO4Fpk6wzylclvatBMddVlQ6qrYeTmSbCsk+m2KVrz5vIyja0o5Y yfeN29s9emXnikmNfv/dA5fpi8XCANNnz3zOfA93DOB9DBf0TQ2/OrSPGjB3op7RCfoPBZ7u AjJ9dM7VABEBAAGJAjwEGAEIACYWIQRwNQOUoMloO4mV9l90l1of+wIKmwUCW/wHfAIbDAUJ CWYBgAAKCRB0l1of+wIKm3KlD/9w/LOG5rtgtCUWPl4B3pZvGpNym6XdK8cop9saOnE85zWf u+sKWCrxNgYkYP7aZrYMPwqDvilxhbTsIJl5HhPgpTO1b0i+c0n1Tij3EElj5UCg3q8mEc17 c+5jRrY3oz77g7E3oPftAjaq1ybbXjY4K32o3JHFR6I8wX3m9wJZJe1+Y+UVrrjY65gZFxcA thNVnWKErarVQGjeNgHV4N1uF3pIx3kT1N4GSnxhoz4Bki91kvkbBhUgYfNflGURfZT3wIKK +d50jd7kqRouXUCzTdzmDh7jnYrcEFM4nvyaYu0JjSS5R672d9SK5LVIfWmoUGzqD4AVmUW8 pcv461+PXchuS8+zpltR9zajl72Q3ymlT4BTAQOlCWkD0snBoKNUB5d2EXPNV13nA0qlm4U2 GpROfJMQXjV6fyYRvttKYfM5xYKgRgtP0z5lTAbsjg9WFKq0Fndh7kUlmHjuAIwKIV4Tzo75 QO2zC0/NTaTjmrtiXhP+vkC4pcrOGNsbHuaqvsc/ZZ0siXyYsqbctj/sCd8ka2r94u+c7o4l BGaAm+FtwAfEAkXHu4y5Phuv2IRR+x1wTey1U1RaEPgN8xq0LQ1OitX4t2mQwjdPihZQBCnZ wzOrkbzlJMNrMKJpEgulmxAHmYJKgvZHXZXtLJSejFjR0GdHJcL5rwVOMWB8cg== Message-ID: <3ab2fa96-e07b-b6c8-3a03-1b6ad2d5bb04@ashroe.eu> Date: Fri, 7 Jun 2019 16:42:03 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190606150354.GF29521@hmswarspite.think-freely.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-AuthUser: mdr@ashroe.eu 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 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. > > >>> >>>>> >>>>> 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 understanding >>> during the development process. >>>> >>>> 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 >> >> Sorry, I could not see the dependency. >> >> [master][dpdk.org] $ grep -ri "objdump" devtools/ >> [master][dpdk.org] $ grep -ri "objdump" usertools/ >> [master][dpdk.org] $ grep -ri "__rte_external" * >> >>> >>>> 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? >>>> >>> What developer is running checkpatch/posting patches without first building >>> their changes? >> >> # it is not developer, The CI/CD tools can quicky check the sanity of patches >> 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. >> >> >>> >>> >>>>> >>>>> 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 >>>>> >>>>> 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. >>>>> >>>>> >>>>> [1] https://mails.dpdk.org/archives/dev/2017-December/083828.html >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> Best >>>>>>> Neil >>>>>>> >>>>>>>> /Bruce >>>>>>>> >>>>>> >>>> >> >