Re: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag

["Wiles, Keith" <keith.wiles@intel.com>]

> On Jun 6, 2019, at 8:43 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> On Thu, Jun 06, 2019 at 01:18:29PM +0000, Wiles, Keith wrote:
>> 
>> 
>>> On Jun 6, 2019, at 7:04 AM, Jerin Jacob Kollanukkaran <jerinj@marvell.com> wrote:
>>> 
>>>> -----Original Message-----
>>>> From: Neil Horman <nhorman@tuxdriver.com>
>>>> Sent: Thursday, June 6, 2019 5:04 PM
>>>> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>>>> Cc: Bruce Richardson <bruce.richardson@intel.com>; dev@dpdk.org;
>>>> Thomas Monjalon <thomas@monjalon.net>
>>>> 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 <nhorman@tuxdriver.com>
>>>>>> Sent: Wednesday, June 5, 2019 11:41 PM
>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>
>>>>>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org;
>>>>>> Thomas Monjalon <thomas@monjalon.net>
>>>>>> 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 <nhorman@tuxdriver.com>
>>>>>>>>> Sent: Sunday, May 26, 2019 12:14 AM
>>>>>>>>> To: dev@dpdk.org
>>>>>>>>> Cc: Neil Horman <nhorman@tuxdriver.com>; Jerin Jacob
>>>>>>>>> Kollanukkaran <jerinj@marvell.com>; Bruce Richardson
>>>>>>>>> <bruce.richardson@intel.com>; Thomas Monjalon
>>>>>>>>> <thomas@monjalon.net>
>>>>>>>>> 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.
>>> 
>>>> 
>>>> 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
>>> 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?
>>> 
>>>> 
>>>> 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.
>> 
>> I feel like we are creating a lot of extra work for the developer and adding a number of constraints to getting code patches submitted as the tools all have to be working together. The versioning file and __rte_experimental stuff today has always being handle wrong or not done by the developer. Altering 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 isolate internal API and data from the user. If someone wants to access the those 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.
> 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, and I'm
> not confident that that will be a focus of anyones review
> 
>> 3) Adding setters/getters for internal data.
> Sure, no issue here
>> 4) Make sure we review and reject direct use of internal functions and data.
> How exactly do you intend to enforce this?  By definition, the use of internal
> functions is restricted only to dpdk core libraries, and those are the only
> 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)
> 
> We already have this mechanism available for experimental abi, and from my
> 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 instead of using getters/setters.

I understand your other points, but if we use the items above it should solve 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 normally the developer only has a few APIs to mark and it works along with versioning you added. The versioning and experimental parts are kind of a requirement 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 libs, we should move them to private headers instead.
> 
> Neil
> 
>> 
>> 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 minimum steps should be reasonable IMHO.
>>>> 
>>>> 
>>>> [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
>> 
>> Regards,
>> Keith

Regards,
Keith