[RFC,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, but I think it hits some of the high points, restricting symbol access
for any library that prototypes its public and private symbols in the same
header file, excluding the internal symbols from ABI constraints, and clearly
documenting those symbols which we wish to limit to internal usage.

I've included a patch to the dpaa library to demonstrate its usage.  If there is
consensus on this approach, I'll expand and repost the patch, pulling in the
other libraries which have internal-only symbol usage.

Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
CC: Bruce Richardson <bruce.richardson@intel.com>
CC: Thomas Monjalon <thomas@monjalon.net>
  

> -----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.


> but I think it hits some of the high points, restricting symbol access for any
> library that prototypes its public and private symbols in the same header file,
> excluding the internal symbols from ABI constraints, and clearly documenting
> those symbols which we wish to limit to internal usage.
> 
> I've included a patch to the dpaa library to demonstrate its usage.  If there is
> consensus on this approach, I'll expand and repost the patch, pulling in the
> other libraries which have internal-only symbol usage.
> 
> Regards
> Neil
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> CC: Bruce Richardson <bruce.richardson@intel.com>
> CC: Thomas Monjalon <thomas@monjalon.net>
>
  

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.

/Bruce
  

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.

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
>
  

> -----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.

> 
> 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
> >
  

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

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.

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
> > >
>
  

> -----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.
> 
> 
> [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
> > > >
> >
  

> 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.
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 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
  

On Thu, Jun 06, 2019 at 12:04:57PM +0000, Jerin Jacob Kollanukkaran 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.
> 
Sure, but why enforce any function naming at all, when you don't have to.

> > 
> > 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

> 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?


> > 
> > 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
> > > > >
> > >
>
  

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.

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
> 
>
  

> 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
  

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Thursday, June 6, 2019 7:05 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 12:04:57PM +0000, Jerin Jacob Kollanukkaran 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.
> >
> 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

> 
> > >
> > > 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
> > > > > >
> > > >
> >
  

On Thu, Jun 06, 2019 at 01:53:47PM +0000, Wiles, Keith wrote:
> 
> 
> > 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.
> 
Sure, we want to prevent that, but the linker version map should prevent it by
not exporting those globals already, meaning we're required to implement getters
and setters.  You can get around that of course by adding a global variable to
the linker map file, but thats strongly discouraged.

> 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.
We're not adding tools, we're modifying existing tools to make this work.  

If you're concerned about the added onus of tagging internal only functions, I
wouldn't be opposed to adding tooling to automatically document functions marked
with __rte_internal with @internal in the documentation tree, so that the
overhead is no larger than what we expect now (documenting @internal in the docs
directly)

As for the effort to tag functions as internal only, I think we might disagree
on what the size of the internal-only api is.  If we start with Jerins
assumption that all internal functions do not start with rte_, then a quick scan
of the version maps in the dpdk suggests there are currently 266 functions that
we would consider to be internal only.  Of those 266 a little less than half are
already addressed by the RFC patch I sent.  Getting the rest under tagged as
__rte_internal is about a days worth of work.  From there its just standard
maintenence.

Neil

> > 
> > 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
> 
>
  

On Thu, Jun 06, 2019 at 02:02:03PM +0000, Jerin Jacob Kollanukkaran wrote:
> > -----Original Message-----
> > From: Neil Horman <nhorman@tuxdriver.com>
> > Sent: Thursday, June 6, 2019 7:05 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 12:04:57PM +0000, Jerin Jacob Kollanukkaran 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.
> > >
> > 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
 

> > 
> > > >
> > > > 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
> > > > > > >
> > > > >
> > >
>
  

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Thursday, June 6, 2019 8:34 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 02:02:03PM +0000, Jerin Jacob Kollanukkaran wrote:
> > > -----Original Message-----
> > > From: Neil Horman <nhorman@tuxdriver.com>
> > > Sent: Thursday, June 6, 2019 7:05 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 12:04:57PM +0000, Jerin Jacob Kollanukkaran
> 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.
> > > >
> > > 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.

I don't have any strong opinion on name prefix vs marking as __rte_internal.
Or combination of both. I am fine any approach.

I have only strong option on not to  induce objdump dependency for checkpatch. 
For the reason mentioned in http://mails.dpdk.org/archives/dev/2019-June/134160.html.


> 
> Neil
> 
> 
> > >
> > > > >
> > > > > 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
> > > > > > > >
> > > > > >
> > > >
> >
  

On Thu, Jun 06, 2019 at 03:14:42PM +0000, Jerin Jacob Kollanukkaran wrote:
><snip as this is getting long>
> 
> I don't have any strong opinion on name prefix vs marking as __rte_internal.
> Or combination of both. I am fine any approach.
> 
> I have only strong option on not to  induce objdump dependency for checkpatch. 
> For the reason mentioned in http://mails.dpdk.org/archives/dev/2019-June/134160.html.
> 

Sorry, in my haste I didn't fully adress this in your previous email

I'm really uncertain what you mean by introducing a checkpatch dependency on
objdump here.  Theres nothing preventing you from running checkpatch before you
build the library.  The only thing checkpatch does in dpdk is scan the patches
for sytle violations, and for changes in the map file for movement to and from
the EXPERIMENTAL section (i.e. no use of objdump).

My patch modifies check-experimental-syms.sh (adding an objdump scan for
INTERNAL symbols, and renaming the script to check-special-syms.sh to be more
meaningful).  That script however, is not run by checkpatch, its run during
compilation of the library to ensure that any symbol in a map file is also
tagged with __rte_internal in the corresponding object).  Theres no path from
checkpatches to check-experimental-syms.sh

What I meant in my last comment was that any dependency on objdump in
check-[experimental|special]-syms.sh already existed prior to this patch.

So I'm unsure why you think checkpatches has a dependency.

Neil
  

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Thursday, June 6, 2019 8:57 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 03:14:42PM +0000, Jerin Jacob Kollanukkaran wrote:
> ><snip as this is getting long>
> >
> > I don't have any strong opinion on name prefix vs marking as
> __rte_internal.
> > Or combination of both. I am fine any approach.
> >
> > I have only strong option on not to  induce objdump dependency for
> checkpatch.
> > For the reason mentioned in http://mails.dpdk.org/archives/dev/2019-
> June/134160.html.
> >
> 
> Sorry, in my haste I didn't fully adress this in your previous email
> 
> I'm really uncertain what you mean by introducing a checkpatch dependency
> on objdump here.  Theres nothing preventing you from running checkpatch
> before you build the library.  The only thing checkpatch does in dpdk is scan
> the patches for sytle violations, and for changes in the map file for
> movement to and from the EXPERIMENTAL section (i.e. no use of objdump).
> 
> My patch modifies check-experimental-syms.sh (adding an objdump scan for
> INTERNAL symbols, and renaming the script to check-special-syms.sh to be
> more meaningful).  That script however, is not run by checkpatch, its run
> during compilation of the library to ensure that any symbol in a map file is
> also tagged with __rte_internal in the corresponding object).  Theres no path
> from checkpatches to check-experimental-syms.sh
> 
> What I meant in my last comment was that any dependency on objdump in
> check-[experimental|special]-syms.sh already existed prior to this patch.

I see. I thought your patches addressing issue related to 
http://patches.dpdk.org/patch/53590/

Where checkpatch.sh complaints when we add new internal driver API with
out rte_experimental? That's where all the discussion started.
The reason why I was saying the API name prefix to mark as internal API is
that checkpatch can detect that case.

Example:
ERROR: symbol otx2_mbox_alloc_msg_rsp is added in the DPDK_19.05 section, but is expected to be added in the EXPERIMENTAL section of the version map


> 
> So I'm unsure why you think checkpatches has a dependency.
> 
> Neil
  

On Thu, Jun 06, 2019 at 04:23:26PM +0000, Jerin Jacob Kollanukkaran wrote:
> > -----Original Message-----
> > From: Neil Horman <nhorman@tuxdriver.com>
> > Sent: Thursday, June 6, 2019 8:57 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 03:14:42PM +0000, Jerin Jacob Kollanukkaran wrote:
> > ><snip as this is getting long>
> > >
> > > I don't have any strong opinion on name prefix vs marking as
> > __rte_internal.
> > > Or combination of both. I am fine any approach.
> > >
> > > I have only strong option on not to  induce objdump dependency for
> > checkpatch.
> > > For the reason mentioned in http://mails.dpdk.org/archives/dev/2019-
> > June/134160.html.
> > >
> > 
> > Sorry, in my haste I didn't fully adress this in your previous email
> > 
> > I'm really uncertain what you mean by introducing a checkpatch dependency
> > on objdump here.  Theres nothing preventing you from running checkpatch
> > before you build the library.  The only thing checkpatch does in dpdk is scan
> > the patches for sytle violations, and for changes in the map file for
> > movement to and from the EXPERIMENTAL section (i.e. no use of objdump).
> > 
> > My patch modifies check-experimental-syms.sh (adding an objdump scan for
> > INTERNAL symbols, and renaming the script to check-special-syms.sh to be
> > more meaningful).  That script however, is not run by checkpatch, its run
> > during compilation of the library to ensure that any symbol in a map file is
> > also tagged with __rte_internal in the corresponding object).  Theres no path
> > from checkpatches to check-experimental-syms.sh
> > 
> > What I meant in my last comment was that any dependency on objdump in
> > check-[experimental|special]-syms.sh already existed prior to this patch.
> 
> I see. I thought your patches addressing issue related to 
> http://patches.dpdk.org/patch/53590/
> 
It does, it just does it in a different way than you do it.

> Where checkpatch.sh complaints when we add new internal driver API with
> out rte_experimental? That's where all the discussion started.
> The reason why I was saying the API name prefix to mark as internal API is
> that checkpatch can detect that case.
> 
Ah, thats a fair point, with my approach we need to add some code to
check-symbol-change.sh such that any symbols listed in an INTERNAL section were
ignored.  That would provide behavioral compatibility to what you were doing in
your patch.

But regardless, there is no dependency on objdump that wasn't there before, are
we in agreement on that?

Neil

> Example:
> ERROR: symbol otx2_mbox_alloc_msg_rsp is added in the DPDK_19.05 section, but is expected to be added in the EXPERIMENTAL section of the version map
> 
> 
> > 
> > So I'm unsure why you think checkpatches has a dependency.
> > 
> > Neil
> 
>
  

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Thursday, June 6, 2019 10:26 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 04:23:26PM +0000, Jerin Jacob Kollanukkaran wrote:
> > > -----Original Message-----
> > > From: Neil Horman <nhorman@tuxdriver.com>
> > > Sent: Thursday, June 6, 2019 8:57 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 03:14:42PM +0000, Jerin Jacob Kollanukkaran
> wrote:
> > > ><snip as this is getting long>
> > > >
> > > > I don't have any strong opinion on name prefix vs marking as
> > > __rte_internal.
> > > > Or combination of both. I am fine any approach.
> > > >
> > > > I have only strong option on not to  induce objdump dependency for
> > > checkpatch.
> > > > For the reason mentioned in
> > > > http://mails.dpdk.org/archives/dev/2019-
> > > June/134160.html.
> > > >
> > >
> > > Sorry, in my haste I didn't fully adress this in your previous email
> > >
> > > I'm really uncertain what you mean by introducing a checkpatch
> > > dependency on objdump here.  Theres nothing preventing you from
> > > running checkpatch before you build the library.  The only thing
> > > checkpatch does in dpdk is scan the patches for sytle violations,
> > > and for changes in the map file for movement to and from the
> EXPERIMENTAL section (i.e. no use of objdump).
> > >
> > > My patch modifies check-experimental-syms.sh (adding an objdump scan
> > > for INTERNAL symbols, and renaming the script to
> > > check-special-syms.sh to be more meaningful).  That script however,
> > > is not run by checkpatch, its run during compilation of the library
> > > to ensure that any symbol in a map file is also tagged with
> > > __rte_internal in the corresponding object).  Theres no path from
> > > checkpatches to check-experimental-syms.sh
> > >
> > > What I meant in my last comment was that any dependency on objdump
> > > in check-[experimental|special]-syms.sh already existed prior to this
> patch.
> >
> > I see. I thought your patches addressing issue related to
> > http://patches.dpdk.org/patch/53590/
> >
> It does, it just does it in a different way than you do it.
> 
> > Where checkpatch.sh complaints when we add new internal driver API
> > with out rte_experimental? That's where all the discussion started.
> > The reason why I was saying the API name prefix to mark as internal
> > API is that checkpatch can detect that case.
> >
> Ah, thats a fair point, with my approach we need to add some code to check-
> symbol-change.sh such that any symbols listed in an INTERNAL section were
> ignored.  That would provide behavioral compatibility to what you were
> doing in your patch.

OK.

> But regardless, there is no dependency on objdump that wasn't there
> before, are we in agreement on that?

Yes.

> 
> Neil
> 
> > Example:
> > ERROR: symbol otx2_mbox_alloc_msg_rsp is added in the DPDK_19.05
> > section, but is expected to be added in the EXPERIMENTAL section of
> > the version map
> >
> >
> > >
> > > So I'm unsure why you think checkpatches has a dependency.
> > >
> > > Neil
> >
> >
  

On Fri, Jun 07, 2019 at 09:41:07AM +0000, Jerin Jacob Kollanukkaran wrote:
> > -----Original Message-----
> > From: Neil Horman <nhorman@tuxdriver.com>
> > Sent: Thursday, June 6, 2019 10:26 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 04:23:26PM +0000, Jerin Jacob Kollanukkaran wrote:
> > > > -----Original Message-----
> > > > From: Neil Horman <nhorman@tuxdriver.com>
> > > > Sent: Thursday, June 6, 2019 8:57 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 03:14:42PM +0000, Jerin Jacob Kollanukkaran
> > wrote:
> > > > ><snip as this is getting long>
> > > > >
> > > > > I don't have any strong opinion on name prefix vs marking as
> > > > __rte_internal.
> > > > > Or combination of both. I am fine any approach.
> > > > >
> > > > > I have only strong option on not to  induce objdump dependency for
> > > > checkpatch.
> > > > > For the reason mentioned in
> > > > > http://mails.dpdk.org/archives/dev/2019-
> > > > June/134160.html.
> > > > >
> > > >
> > > > Sorry, in my haste I didn't fully adress this in your previous email
> > > >
> > > > I'm really uncertain what you mean by introducing a checkpatch
> > > > dependency on objdump here.  Theres nothing preventing you from
> > > > running checkpatch before you build the library.  The only thing
> > > > checkpatch does in dpdk is scan the patches for sytle violations,
> > > > and for changes in the map file for movement to and from the
> > EXPERIMENTAL section (i.e. no use of objdump).
> > > >
> > > > My patch modifies check-experimental-syms.sh (adding an objdump scan
> > > > for INTERNAL symbols, and renaming the script to
> > > > check-special-syms.sh to be more meaningful).  That script however,
> > > > is not run by checkpatch, its run during compilation of the library
> > > > to ensure that any symbol in a map file is also tagged with
> > > > __rte_internal in the corresponding object).  Theres no path from
> > > > checkpatches to check-experimental-syms.sh
> > > >
> > > > What I meant in my last comment was that any dependency on objdump
> > > > in check-[experimental|special]-syms.sh already existed prior to this
> > patch.
> > >
> > > I see. I thought your patches addressing issue related to
> > > http://patches.dpdk.org/patch/53590/
> > >
> > It does, it just does it in a different way than you do it.
> > 
> > > Where checkpatch.sh complaints when we add new internal driver API
> > > with out rte_experimental? That's where all the discussion started.
> > > The reason why I was saying the API name prefix to mark as internal
> > > API is that checkpatch can detect that case.
> > >
> > Ah, thats a fair point, with my approach we need to add some code to check-
> > symbol-change.sh such that any symbols listed in an INTERNAL section were
> > ignored.  That would provide behavioral compatibility to what you were
> > doing in your patch.
> 
> OK.
> 
> > But regardless, there is no dependency on objdump that wasn't there
> > before, are we in agreement on that?
> 
> Yes.
> 
ok, let me revise this patch with that change in place.  I'll also complete
tagging of the affected internal functions so we have a better idea of what this
change entails

Neil
  

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 <nhorman@tuxdriver.com>
>>> Sent: Thursday, June 6, 2019 7:05 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 12:04:57PM +0000, Jerin Jacob Kollanukkaran 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.
>>>>
>>> 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
>>>>>>>>
>>>>>>
>>>>
>>
>
  

> On Jun 7, 2019, at 10:42 AM, Ray Kinsella <mdr@ashroe.eu> 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 <nhorman@tuxdriver.com>
>>>> Sent: Thursday, June 6, 2019 7:05 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 12:04:57PM +0000, Jerin Jacob Kollanukkaran 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.
>>>>> 
>>>> 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.
> 
>> 
>> 
>>>> 
>>>>>> 
>>>>>> 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

Regards,
Keith
  

On Fri, Jun 07, 2019 at 06:21:21PM +0000, Wiles, Keith wrote:
> 
> 
> > On Jun 7, 2019, at 10:42 AM, Ray Kinsella <mdr@ashroe.eu> 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 <nhorman@tuxdriver.com>
> >>>> Sent: Thursday, June 6, 2019 7:05 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 12:04:57PM +0000, Jerin Jacob Kollanukkaran 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.
> >>>>> 
> >>>> 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