Re: [dpdk-dev] [PATCH v2 09/10] octeonx: mark internal functions with __rte_internal

Re: [dpdk-dev] [PATCH v2 09/10] octeonx: mark internal functions with __rte_internal

[Jerin Jacob Kollanukkaran]

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Thursday, June 13, 2019 7:54 PM
> 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] [PATCH v2 09/10] octeonx: mark internal functions with
> __rte_internal
>
> +
> +DPDK_18.05 {
> +	global:
> +	octeontx_logtype_mbox;

It should move to INTERNAL. Right?

> +
> +	local: *;
> +};
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v2 09/10] octeonx: mark internal functions with __rte_internal

[Neil Horman]

On Mon, Jun 17, 2019 at 04:09:26PM +0000, Jerin Jacob Kollanukkaran wrote:
> > -----Original Message-----
> > From: Neil Horman <nhorman@tuxdriver.com>
> > Sent: Thursday, June 13, 2019 7:54 PM
> > 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] [PATCH v2 09/10] octeonx: mark internal functions with
> > __rte_internal
> >
> > +
> > +DPDK_18.05 {
> > +	global:
> > +	octeontx_logtype_mbox;
> 
> It should move to INTERNAL. Right?
> 

So, thats an interesting symbol that we should probably discuss more.
octeontx_logtype_mbox is actually a global int variable, not a function, and
__rte_internal only works on the latter type of symbol (i.e. the
__attribute__(error(...)) tag only applies to functions.  I could create an
__rte_internal_data data, that can do something simmilar for global variables,
but it occured to me that perhaps global variables should not be a method of
communication between internal libraries like this (opting instead for getter
and setter methods to protect it, and then exempting those functions with
__rte_internal).  I believe David mentioned something along these lines as well
previously, but I didn't want to go making changes like that without a more
focused discussion, so I opted to leave global variables in place for now.

Thoughts on how to address this case?

Neil

> > +
> > +	local: *;
> > +};
> > --
> > 2.20.1
> 
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 09/10] octeonx: mark internal functions with __rte_internal

[Jerin Jacob Kollanukkaran]

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Tuesday, June 18, 2019 12:44 AM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Subject: [EXT] Re: [PATCH v2 09/10] octeonx: mark internal functions with
> __rte_internal
> 
> On Mon, Jun 17, 2019 at 04:09:26PM +0000, Jerin Jacob Kollanukkaran wrote:
> > > -----Original Message-----
> > > From: Neil Horman <nhorman@tuxdriver.com>
> > > Sent: Thursday, June 13, 2019 7:54 PM
> > > 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] [PATCH v2 09/10] octeonx: mark internal functions
> > > with __rte_internal
> > >
> > > +
> > > +DPDK_18.05 {
> > > +	global:
> > > +	octeontx_logtype_mbox;
> >
> > It should move to INTERNAL. Right?
> >
> 
> So, thats an interesting symbol that we should probably discuss more.
> octeontx_logtype_mbox is actually a global int variable, not a function, and
> __rte_internal only works on the latter type of symbol (i.e. the
> __attribute__(error(...)) tag only applies to functions.  I could create an
> __rte_internal_data data, that can do something simmilar for global
> variables, but it occured to me that perhaps global variables should not be a
> method of communication between internal libraries like this (opting instead
> for getter and setter methods to protect it, and then exempting those
> functions with __rte_internal).  I believe David mentioned something along
> these lines as well previously, but I didn't want to go making changes like that
> without a more focused discussion, so I opted to leave global variables in
> place for now.
> 
> Thoughts on how to address this case?

The runtime log infrastructure currently depends on global variables in DPDK.
Either way is fine with me(Introducing getter/setter vs current scheme). But it has
to be addressed for completeness.

> 
> Neil
> 
> > > +
> > > +	local: *;
> > > +};
> > > --
> > > 2.20.1
> >
> >

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

[Neil Horman]

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>

[dpdk-dev] [PATCH v2 0/10] dpdk: introduce __rte_internal tag

[Neil Horman]

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.

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>

---
Change notes

v1->v2
Moved check-experimental-syms.sh rename to earlier in the series
Updated meson build environment to support BUILDING_RTE_SDK

[dpdk-dev] [PATCH v2 09/10] octeonx: mark internal functions with __rte_internal

[Neil Horman]

Identify functions in the octeon driver which are internal (based on
their not having an rte_ prefix) and tag them with __rte_internal

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>
---
 drivers/common/octeontx/octeontx_mbox.c                 | 6 +++---
 drivers/common/octeontx/octeontx_mbox.h                 | 6 +++---
 drivers/common/octeontx/rte_common_octeontx_version.map | 9 ++++++++-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/common/octeontx/octeontx_mbox.c b/drivers/common/octeontx/octeontx_mbox.c
index 880f8a40f..02bb593b8 100644
--- a/drivers/common/octeontx/octeontx_mbox.c
+++ b/drivers/common/octeontx/octeontx_mbox.c
@@ -190,7 +190,7 @@ mbox_send(struct mbox *m, struct octeontx_mbox_hdr *hdr, const void *txmsg,
 }
 
 int
-octeontx_mbox_set_ram_mbox_base(uint8_t *ram_mbox_base)
+__rte_internal octeontx_mbox_set_ram_mbox_base(uint8_t *ram_mbox_base)
 {
 	struct mbox *m = &octeontx_mbox;
 
@@ -213,7 +213,7 @@ octeontx_mbox_set_ram_mbox_base(uint8_t *ram_mbox_base)
 }
 
 int
-octeontx_mbox_set_reg(uint8_t *reg)
+__rte_internal octeontx_mbox_set_reg(uint8_t *reg)
 {
 	struct mbox *m = &octeontx_mbox;
 
@@ -236,7 +236,7 @@ octeontx_mbox_set_reg(uint8_t *reg)
 }
 
 int
-octeontx_mbox_send(struct octeontx_mbox_hdr *hdr, void *txdata,
+__rte_internal octeontx_mbox_send(struct octeontx_mbox_hdr *hdr, void *txdata,
 				 uint16_t txlen, void *rxdata, uint16_t rxlen)
 {
 	struct mbox *m = &octeontx_mbox;
diff --git a/drivers/common/octeontx/octeontx_mbox.h b/drivers/common/octeontx/octeontx_mbox.h
index 43fbda282..1055d30b0 100644
--- a/drivers/common/octeontx/octeontx_mbox.h
+++ b/drivers/common/octeontx/octeontx_mbox.h
@@ -29,9 +29,9 @@ struct octeontx_mbox_hdr {
 	uint8_t res_code; /* Functional layer response code */
 };
 
-int octeontx_mbox_set_ram_mbox_base(uint8_t *ram_mbox_base);
-int octeontx_mbox_set_reg(uint8_t *reg);
-int octeontx_mbox_send(struct octeontx_mbox_hdr *hdr,
+int __rte_internal octeontx_mbox_set_ram_mbox_base(uint8_t *ram_mbox_base);
+int __rte_internal octeontx_mbox_set_reg(uint8_t *reg);
+int __rte_internal octeontx_mbox_send(struct octeontx_mbox_hdr *hdr,
 		void *txdata, uint16_t txlen, void *rxdata, uint16_t rxlen);
 
 #endif /* __OCTEONTX_MBOX_H__ */
diff --git a/drivers/common/octeontx/rte_common_octeontx_version.map b/drivers/common/octeontx/rte_common_octeontx_version.map
index f04b3b7f8..523444a75 100644
--- a/drivers/common/octeontx/rte_common_octeontx_version.map
+++ b/drivers/common/octeontx/rte_common_octeontx_version.map
@@ -1,7 +1,14 @@
-DPDK_18.05 {
+INTERNAL {
 	global:
 
 	octeontx_mbox_set_ram_mbox_base;
 	octeontx_mbox_set_reg;
 	octeontx_mbox_send;
 };
+
+DPDK_18.05 {
+	global:
+	octeontx_logtype_mbox;
+
+	local: *;
+};
-- 
2.20.1