DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag
Date: Thu, 6 Jun 2019 12:04:57 +0000
Message-ID: <BYAPR18MB242407B0A2A21E187843917FC8170@BYAPR18MB2424.namprd18.prod.outlook.com> (raw)
In-Reply-To: <20190606113422.GA29521@hmswarspite.think-freely.org>

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

  reply index

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-25 18:43 [dpdk-dev] " Neil Horman
2019-05-25 18:43 ` [dpdk-dev] [RFC PATCH 1/2] Add __rte_internal tag for functions and version target Neil Horman
2019-06-05 16:14   ` [dpdk-dev] [EXT] " Jerin Jacob Kollanukkaran
2019-05-25 18:43 ` [dpdk-dev] [RFC PATCH 2/2] Convert dpaa driver to tag internal-only symbols appropriately Neil Horman
2019-06-05 16:24 ` [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag Jerin Jacob Kollanukkaran
2019-06-05 16:45   ` Bruce Richardson
2019-06-05 18:11     ` Neil Horman
2019-06-06  9:44       ` Jerin Jacob Kollanukkaran
2019-06-06 11:34         ` Neil Horman
2019-06-06 12:04           ` Jerin Jacob Kollanukkaran [this message]
2019-06-06 13:18             ` Wiles, Keith
2019-06-06 13:43               ` Neil Horman
2019-06-06 13:53                 ` Wiles, Keith
2019-06-06 14:46                   ` Neil Horman
2019-06-06 13:35             ` Neil Horman
2019-06-06 14:02               ` Jerin Jacob Kollanukkaran
2019-06-06 15:03                 ` Neil Horman
2019-06-06 15:14                   ` Jerin Jacob Kollanukkaran
2019-06-06 15:26                     ` Neil Horman
2019-06-06 16:23                       ` Jerin Jacob Kollanukkaran
2019-06-06 16:55                         ` Neil Horman
2019-06-07  9:41                           ` Jerin Jacob Kollanukkaran
2019-06-07 10:35                             ` Neil Horman
2019-06-07 15:42                   ` Ray Kinsella
2019-06-07 18:21                     ` Wiles, Keith
2019-06-12 20:38 ` [dpdk-dev] [PATCH v1 0/9] dpdk: " Neil Horman
2019-06-12 20:38   ` [dpdk-dev] [PATCH v1 1/9] Add __rte_internal tag for functions and version target Neil Horman
2019-06-12 20:38   ` [dpdk-dev] [PATCH v1 2/9] Exempt INTERNAL symbols from checking Neil Horman
2019-06-12 20:38   ` [dpdk-dev] [PATCH v1 3/9] mark dpaa driver internal-only symbols with __rte_internal Neil Horman
2019-06-12 20:38   ` [dpdk-dev] [PATCH v1 4/9] fslmc: identify internal only functions and tag them as __rte_internal Neil Horman
2019-06-12 20:38   ` [dpdk-dev] [PATCH v1 5/9] dpaa2: Adjust dpaa2 driver to mark internal symbols with __rte_internal Neil Horman
2019-06-12 20:39   ` [dpdk-dev] [PATCH v1 6/9] dpaax: mark internal functions " Neil Horman
2019-06-12 20:39   ` [dpdk-dev] [PATCH v1 7/9] cpt: " Neil Horman
2019-06-17  5:27     ` [dpdk-dev] [EXT] " Anoob Joseph
2019-06-12 20:39   ` [dpdk-dev] [PATCH v1 8/9] octeonx: " Neil Horman
2019-06-12 20:39   ` [dpdk-dev] [PATCH v1 9/9] dpaa2: " Neil Horman
2019-06-12 21:14     ` Aaron Conole
2019-06-13 10:24       ` Neil Horman
2019-06-13  7:53   ` [dpdk-dev] [PATCH v1 0/9] dpdk: introduce __rte_internal tag David Marchand
2019-06-13 10:30     ` Neil Horman
2019-06-13 14:23 ` [dpdk-dev] [PATCH v2 0/10] " Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 01/10] Add __rte_internal tag for functions and version target Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 02/10] meson: add BUILDING_RTE_SDK Neil Horman
2019-06-13 14:44     ` Bruce Richardson
2019-06-19 10:39       ` Neil Horman
2019-06-19 10:46         ` Bruce Richardson
2019-06-19 18:34           ` Neil Horman
2019-06-20 10:20             ` Bruce Richardson
2019-06-20 10:21     ` Bruce Richardson
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 03/10] Exempt INTERNAL symbols from checking Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 04/10] mark dpaa driver internal-only symbols with __rte_internal Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 05/10] fslmc: identify internal only functions and tag them as __rte_internal Neil Horman
2019-06-17  7:30     ` Hemant Agrawal
2019-06-19 10:45       ` Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 06/10] dpaa2: Adjust dpaa2 driver to mark internal symbols with __rte_internal Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 07/10] dpaax: mark internal functions " Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 08/10] cpt: " Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 09/10] octeonx: " Neil Horman
2019-06-13 14:23   ` [dpdk-dev] [PATCH v2 10/10] dpaa2: " Neil Horman
2019-08-06 10:03   ` [dpdk-dev] [PATCH v2 0/10] dpdk: introduce __rte_internal tag Thomas Monjalon
2019-08-06 12:21     ` Neil Horman

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR18MB242407B0A2A21E187843917FC8170@BYAPR18MB2424.namprd18.prod.outlook.com \
    --to=jerinj@marvell.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=nhorman@tuxdriver.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox