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