| Message ID | 20180419185159.11266-1-pbhagavatula@caviumnetworks.com |
|---|---|
| State | New |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
On 4/19/2018 7:51 PM, Pavan Nikhilesh wrote: > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
On Friday 20 April 2018 12:21 AM, Pavan Nikhilesh wrote: > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> > --- > drivers/bus/dpaa/base/fman/netcfg_layer.c | 5 ----- > drivers/bus/dpaa/base/qbman/bman_driver.c | 4 ++-- > drivers/bus/dpaa/base/qbman/qman.c | 2 +- > drivers/bus/dpaa/base/qbman/qman_driver.c | 4 ++-- > drivers/bus/dpaa/base/qbman/qman_priv.h | 1 - > drivers/bus/dpaa/dpaa_bus.c | 2 +- > drivers/bus/fslmc/qbman/qbman_portal.c | 3 +-- > drivers/bus/fslmc/qbman/qbman_portal.h | 1 - > drivers/net/i40e/i40e_flow.c | 2 +- > drivers/net/qede/base/bcm_osal.c | 2 +- > drivers/raw/skeleton_rawdev/skeleton_rawdev.c | 2 +- > lib/librte_net/net_crc_neon.h | 4 ++-- > 12 files changed, 12 insertions(+), 20 deletions(-) > For the DPAA, DPAA2 (FSLMC) and Skeleton_Rawdev part: Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
On Fri, Apr 20, 2018 at 12:21:59AM +0530, Pavan Nikhilesh wrote: > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> > --- > drivers/bus/dpaa/base/fman/netcfg_layer.c | 5 ----- > drivers/bus/dpaa/base/qbman/bman_driver.c | 4 ++-- > drivers/bus/dpaa/base/qbman/qman.c | 2 +- > drivers/bus/dpaa/base/qbman/qman_driver.c | 4 ++-- > drivers/bus/dpaa/base/qbman/qman_priv.h | 1 - > drivers/bus/dpaa/dpaa_bus.c | 2 +- > drivers/bus/fslmc/qbman/qbman_portal.c | 3 +-- > drivers/bus/fslmc/qbman/qbman_portal.h | 1 - > drivers/net/i40e/i40e_flow.c | 2 +- > drivers/net/qede/base/bcm_osal.c | 2 +- > drivers/raw/skeleton_rawdev/skeleton_rawdev.c | 2 +- > lib/librte_net/net_crc_neon.h | 4 ++-- > 12 files changed, 12 insertions(+), 20 deletions(-) [...] > diff --git a/lib/librte_net/net_crc_neon.h b/lib/librte_net/net_crc_neon.h > index 63fa1d4a1..cb3da72ed 100644 > --- a/lib/librte_net/net_crc_neon.h > +++ b/lib/librte_net/net_crc_neon.h > @@ -21,8 +21,8 @@ struct crc_pmull_ctx { > uint64x2_t rk7_rk8; > }; > > -struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); > -struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); > +static struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); > +static struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); > > /** Not sure it will still work after that. From what I see, these global variables are initialized once in rte_net_crc_neon_init, and used as a const parameter in crc32_eth_calc_pmull(). Changing them to static will create an instance of these variables for each included file, which is not what we want. I think that the proper way to solve it would be to add the definition in a new .c file, and only have a declaration in the .h. An even better way would be to make variable const and initialize it with its content. It could even enhance performance. Something like: net_crc_neon.h: static const struct crc_pmull_ctx crc32_eth_pmull = { <values...> } __rte_aligned(16); static const struct crc_pmull_ctx crc16_ccitt_pmull = { <values...> } __rte_aligned(16);
On Mon, Apr 23, 2018 at 11:00:09AM +0200, Olivier Matz wrote: > On Fri, Apr 20, 2018 at 12:21:59AM +0530, Pavan Nikhilesh wrote: > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> > > --- > > drivers/bus/dpaa/base/fman/netcfg_layer.c | 5 ----- > > drivers/bus/dpaa/base/qbman/bman_driver.c | 4 ++-- > > drivers/bus/dpaa/base/qbman/qman.c | 2 +- > > drivers/bus/dpaa/base/qbman/qman_driver.c | 4 ++-- > > drivers/bus/dpaa/base/qbman/qman_priv.h | 1 - > > drivers/bus/dpaa/dpaa_bus.c | 2 +- > > drivers/bus/fslmc/qbman/qbman_portal.c | 3 +-- > > drivers/bus/fslmc/qbman/qbman_portal.h | 1 - > > drivers/net/i40e/i40e_flow.c | 2 +- > > drivers/net/qede/base/bcm_osal.c | 2 +- > > drivers/raw/skeleton_rawdev/skeleton_rawdev.c | 2 +- > > lib/librte_net/net_crc_neon.h | 4 ++-- > > 12 files changed, 12 insertions(+), 20 deletions(-) > > [...] > > > diff --git a/lib/librte_net/net_crc_neon.h b/lib/librte_net/net_crc_neon.h > > index 63fa1d4a1..cb3da72ed 100644 > > --- a/lib/librte_net/net_crc_neon.h > > +++ b/lib/librte_net/net_crc_neon.h > > @@ -21,8 +21,8 @@ struct crc_pmull_ctx { > > uint64x2_t rk7_rk8; > > }; > > > > -struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); > > -struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); > > +static struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); > > +static struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); > > > > /** > > Not sure it will still work after that. > > From what I see, these global variables are initialized once in > rte_net_crc_neon_init, and used as a const parameter in > crc32_eth_calc_pmull(). > > Changing them to static will create an instance of these variables for > each included file, which is not what we want. > > I think that the proper way to solve it would be to add the definition > in a new .c file, and only have a declaration in the .h. > > Hi Olivier, Thanks for the heads up, the second solution seems more viable and while implementing it I faced few Issues. GCC doesnt suport const vector instructions i.e. the following assignment throw as compiler error. static const struct crc_pmull_ctx crc32_eth_pmull = { .rk1_rk2 = vld1q_u64((uint64_t[2]){0xccaa009eLLU, 0x1751997d0LLU}), .rk5_rk6 = vld1q_u64((uint64_t[2]){0xccaa009eLLU, 0x163cd6124LLU}), .rk7_rk8 = vld1q_u64((uint64_t[2]){0x1f7011640LLU, 0x1db710641LLU}), } __rte_aligned(16); I have gotten path the error by modifying struct crc_pmull_ctx as follows: struct crc_pmull_ctx { union { uint64_t rk12[2]; uint64x2_t rk1_rk2; }; union { uint64_t rk56[2]; uint64x2_t rk5_rk6; }; union { uint64_t rk78[2]; uint64x2_t rk7_rk8; }; }; static const struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16) = { .rk12 = {0xccaa009eLLU, 0x1751997d0LLU}, .rk56 = {0xccaa009eLLU, 0x163cd6124LLU}, .rk78 = {0x1f7011640LLU, 0x1db710641LLU}, }; static const struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16) = { .rk12 = {0x189aeLLU, 0x8e10LLU}, .rk56 = {0x189aeLLU, 0x114aaLLU}, .rk78 = {0x11c581910LLU, 0x10811LLU}, }; I have checked the hex dump of the assignment with the current code and the above piece of code and they are similar. Let me know if my solution seems viable I will send the v2. > An even better way would be to make variable const and initialize it > with its content. It could even enhance performance. Something like: > > net_crc_neon.h: > > static const struct crc_pmull_ctx crc32_eth_pmull = { > <values...> > } __rte_aligned(16); > > static const struct crc_pmull_ctx crc16_ccitt_pmull = { > <values...> > } __rte_aligned(16); > Thanks, Pavan.
Le 25 avril 2018 17:52:00 GMT+02:00, Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> a écrit : >On Mon, Apr 23, 2018 at 11:00:09AM +0200, Olivier Matz wrote: >> On Fri, Apr 20, 2018 at 12:21:59AM +0530, Pavan Nikhilesh wrote: >> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> >> > --- >> > drivers/bus/dpaa/base/fman/netcfg_layer.c | 5 ----- >> > drivers/bus/dpaa/base/qbman/bman_driver.c | 4 ++-- >> > drivers/bus/dpaa/base/qbman/qman.c | 2 +- >> > drivers/bus/dpaa/base/qbman/qman_driver.c | 4 ++-- >> > drivers/bus/dpaa/base/qbman/qman_priv.h | 1 - >> > drivers/bus/dpaa/dpaa_bus.c | 2 +- >> > drivers/bus/fslmc/qbman/qbman_portal.c | 3 +-- >> > drivers/bus/fslmc/qbman/qbman_portal.h | 1 - >> > drivers/net/i40e/i40e_flow.c | 2 +- >> > drivers/net/qede/base/bcm_osal.c | 2 +- >> > drivers/raw/skeleton_rawdev/skeleton_rawdev.c | 2 +- >> > lib/librte_net/net_crc_neon.h | 4 ++-- >> > 12 files changed, 12 insertions(+), 20 deletions(-) >> >> [...] >> >> > diff --git a/lib/librte_net/net_crc_neon.h >b/lib/librte_net/net_crc_neon.h >> > index 63fa1d4a1..cb3da72ed 100644 >> > --- a/lib/librte_net/net_crc_neon.h >> > +++ b/lib/librte_net/net_crc_neon.h >> > @@ -21,8 +21,8 @@ struct crc_pmull_ctx { >> > uint64x2_t rk7_rk8; >> > }; >> > >> > -struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); >> > -struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); >> > +static struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); >> > +static struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); >> > >> > /** >> >> Not sure it will still work after that. >> >> From what I see, these global variables are initialized once in >> rte_net_crc_neon_init, and used as a const parameter in >> crc32_eth_calc_pmull(). >> >> Changing them to static will create an instance of these variables >for >> each included file, which is not what we want. >> >> I think that the proper way to solve it would be to add the >definition >> in a new .c file, and only have a declaration in the .h. >> >> >Hi Olivier, > >Thanks for the heads up, the second solution seems more viable and >while >implementing it I faced few Issues. GCC doesnt suport const vector >instructions >i.e. the following assignment throw as compiler error. > > static const struct crc_pmull_ctx crc32_eth_pmull = { > .rk1_rk2 = vld1q_u64((uint64_t[2]){0xccaa009eLLU, 0x1751997d0LLU}), > .rk5_rk6 = vld1q_u64((uint64_t[2]){0xccaa009eLLU, 0x163cd6124LLU}), > .rk7_rk8 = vld1q_u64((uint64_t[2]){0x1f7011640LLU, 0x1db710641LLU}), > } __rte_aligned(16); > >I have gotten path the error by modifying struct crc_pmull_ctx as >follows: > > struct crc_pmull_ctx { > union { > uint64_t rk12[2]; > uint64x2_t rk1_rk2; > }; > union { > uint64_t rk56[2]; > uint64x2_t rk5_rk6; > }; > union { > uint64_t rk78[2]; > uint64x2_t rk7_rk8; > }; > }; > > static const struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16) = >{ > .rk12 = {0xccaa009eLLU, 0x1751997d0LLU}, > .rk56 = {0xccaa009eLLU, 0x163cd6124LLU}, > .rk78 = {0x1f7011640LLU, 0x1db710641LLU}, > }; > > static const struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16) >= { > .rk12 = {0x189aeLLU, 0x8e10LLU}, > .rk56 = {0x189aeLLU, 0x114aaLLU}, > .rk78 = {0x11c581910LLU, 0x10811LLU}, > }; > >I have checked the hex dump of the assignment with the current code and >the >above piece of code and they are similar. > >Let me know if my solution seems viable I will send the v2. > Looks good, just wondering about possible endianness issues. Is arm architecture supported with both little and big endian in dpdk ? >> An even better way would be to make variable const and initialize it >> with its content. It could even enhance performance. Something like: >> >> net_crc_neon.h: >> >> static const struct crc_pmull_ctx crc32_eth_pmull = { >> <values...> >> } __rte_aligned(16); >> >> static const struct crc_pmull_ctx crc16_ccitt_pmull = { >> <values...> >> } __rte_aligned(16); >> > >Thanks, >Pavan.
diff --git a/drivers/bus/dpaa/base/fman/netcfg_layer.c b/drivers/bus/dpaa/base/fman/netcfg_layer.c index 3e956ce12..031c6f1aa 100644 --- a/drivers/bus/dpaa/base/fman/netcfg_layer.c +++ b/drivers/bus/dpaa/base/fman/netcfg_layer.c @@ -18,11 +18,6 @@ #include <rte_dpaa_logs.h> #include <netcfg.h> -/* Structure contains information about all the interfaces given by user - * on command line. - */ -struct netcfg_interface *netcfg_interface; - /* This data structure contaings all configurations information * related to usages of DPA devices. */ diff --git a/drivers/bus/dpaa/base/qbman/bman_driver.c b/drivers/bus/dpaa/base/qbman/bman_driver.c index 1381da363..b14b59052 100644 --- a/drivers/bus/dpaa/base/qbman/bman_driver.c +++ b/drivers/bus/dpaa/base/qbman/bman_driver.c @@ -15,9 +15,9 @@ /* * Global variables of the max portal/pool number this bman version supported */ -u16 bman_ip_rev; +static u16 bman_ip_rev; u16 bman_pool_max; -void *bman_ccsr_map; +static void *bman_ccsr_map; /*****************/ /* Portal driver */ diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c index 2810fdd26..96edfa759 100644 --- a/drivers/bus/dpaa/base/qbman/qman.c +++ b/drivers/bus/dpaa/base/qbman/qman.c @@ -625,7 +625,7 @@ struct qman_portal *qman_create_portal( #define MAX_GLOBAL_PORTALS 8 static struct qman_portal global_portals[MAX_GLOBAL_PORTALS]; -rte_atomic16_t global_portals_used[MAX_GLOBAL_PORTALS]; +static rte_atomic16_t global_portals_used[MAX_GLOBAL_PORTALS]; static struct qman_portal * qman_alloc_global_portal(void) diff --git a/drivers/bus/dpaa/base/qbman/qman_driver.c b/drivers/bus/dpaa/base/qbman/qman_driver.c index 07b29d55e..f6ecd6b28 100644 --- a/drivers/bus/dpaa/base/qbman/qman_driver.c +++ b/drivers/bus/dpaa/base/qbman/qman_driver.c @@ -20,9 +20,9 @@ u16 qm_channel_caam = QMAN_CHANNEL_CAAM; u16 qm_channel_pme = QMAN_CHANNEL_PME; /* Ccsr map address to access ccsrbased register */ -void *qman_ccsr_map; +static void *qman_ccsr_map; /* The qman clock frequency */ -u32 qman_clk; +static u32 qman_clk; static __thread int qmfd = -1; static __thread struct qm_portal_config qpcfg; diff --git a/drivers/bus/dpaa/base/qbman/qman_priv.h b/drivers/bus/dpaa/base/qbman/qman_priv.h index 9e4471e65..02f6301f0 100644 --- a/drivers/bus/dpaa/base/qbman/qman_priv.h +++ b/drivers/bus/dpaa/base/qbman/qman_priv.h @@ -139,7 +139,6 @@ struct qm_portal_config { #define QMAN_REV31 0x0301 #define QMAN_REV32 0x0302 extern u16 qman_ip_rev; /* 0 if uninitialised, otherwise QMAN_REVx */ -extern u32 qman_clk; int qm_set_wpm(int wpm); int qm_get_wpm(int *wpm); diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c index ffc90a702..18c79b157 100644 --- a/drivers/bus/dpaa/dpaa_bus.c +++ b/drivers/bus/dpaa/dpaa_bus.c @@ -50,7 +50,7 @@ struct rte_dpaa_bus rte_dpaa_bus; struct netcfg_info *dpaa_netcfg; /* define a variable to hold the portal_key, once created.*/ -pthread_key_t dpaa_portal_key; +static pthread_key_t dpaa_portal_key; unsigned int dpaa_svr_family; diff --git a/drivers/bus/fslmc/qbman/qbman_portal.c b/drivers/bus/fslmc/qbman/qbman_portal.c index 713ec9651..071450052 100644 --- a/drivers/bus/fslmc/qbman/qbman_portal.c +++ b/drivers/bus/fslmc/qbman/qbman_portal.c @@ -122,8 +122,7 @@ struct qbman_swp *qbman_swp_init(const struct qbman_swp_desc *d) p->vdq.valid_bit = QB_VALID_BIT; p->dqrr.next_idx = 0; p->dqrr.valid_bit = QB_VALID_BIT; - qman_version = p->desc.qman_version; - if ((qman_version & 0xFFFF0000) < QMAN_REV_4100) { + if ((p->desc.qman_version & 0xFFFF0000) < QMAN_REV_4100) { p->dqrr.dqrr_size = 4; p->dqrr.reset_bug = 1; } else { diff --git a/drivers/bus/fslmc/qbman/qbman_portal.h b/drivers/bus/fslmc/qbman/qbman_portal.h index 8bff0b4f4..dbea22a1b 100644 --- a/drivers/bus/fslmc/qbman/qbman_portal.h +++ b/drivers/bus/fslmc/qbman/qbman_portal.h @@ -7,7 +7,6 @@ #include "qbman_sys.h" #include <fsl_qbman_portal.h> -uint32_t qman_version; #define QMAN_REV_4000 0x04000000 #define QMAN_REV_4100 0x04010000 #define QMAN_REV_4101 0x04010001 diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c index d6f5e9923..93dd2d0ca 100644 --- a/drivers/net/i40e/i40e_flow.c +++ b/drivers/net/i40e/i40e_flow.c @@ -131,7 +131,7 @@ const struct rte_flow_ops i40e_flow_ops = { .flush = i40e_flow_flush, }; -union i40e_filter_t cons_filter; +static union i40e_filter_t cons_filter; enum rte_filter_type cons_filter_type = RTE_ETH_FILTER_NONE; /* Pattern matched ethertype filter */ diff --git a/drivers/net/qede/base/bcm_osal.c b/drivers/net/qede/base/bcm_osal.c index f550412f5..2b7df4d1a 100644 --- a/drivers/net/qede/base/bcm_osal.c +++ b/drivers/net/qede/base/bcm_osal.c @@ -19,7 +19,7 @@ /* Array of memzone pointers */ static const struct rte_memzone *ecore_mz_mapping[RTE_MAX_MEMZONE]; /* Counter to track current memzone allocated */ -uint16_t ecore_mz_count; +static uint16_t ecore_mz_count; unsigned long qede_log2_align(unsigned long n) { diff --git a/drivers/raw/skeleton_rawdev/skeleton_rawdev.c b/drivers/raw/skeleton_rawdev/skeleton_rawdev.c index 6bdbbb50d..6d154aab8 100644 --- a/drivers/raw/skeleton_rawdev/skeleton_rawdev.c +++ b/drivers/raw/skeleton_rawdev/skeleton_rawdev.c @@ -32,7 +32,7 @@ int skeleton_pmd_logtype; /* Count of instances */ -uint16_t skeldev_init_once; +static uint16_t skeldev_init_once; /**< Rawdev Skeleton dummy driver name */ #define SKELETON_PMD_RAWDEV_NAME rawdev_skeleton diff --git a/lib/librte_net/net_crc_neon.h b/lib/librte_net/net_crc_neon.h index 63fa1d4a1..cb3da72ed 100644 --- a/lib/librte_net/net_crc_neon.h +++ b/lib/librte_net/net_crc_neon.h @@ -21,8 +21,8 @@ struct crc_pmull_ctx { uint64x2_t rk7_rk8; }; -struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); -struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); +static struct crc_pmull_ctx crc32_eth_pmull __rte_aligned(16); +static struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16); /** * @brief Performs one folding round
Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> --- drivers/bus/dpaa/base/fman/netcfg_layer.c | 5 ----- drivers/bus/dpaa/base/qbman/bman_driver.c | 4 ++-- drivers/bus/dpaa/base/qbman/qman.c | 2 +- drivers/bus/dpaa/base/qbman/qman_driver.c | 4 ++-- drivers/bus/dpaa/base/qbman/qman_priv.h | 1 - drivers/bus/dpaa/dpaa_bus.c | 2 +- drivers/bus/fslmc/qbman/qbman_portal.c | 3 +-- drivers/bus/fslmc/qbman/qbman_portal.h | 1 - drivers/net/i40e/i40e_flow.c | 2 +- drivers/net/qede/base/bcm_osal.c | 2 +- drivers/raw/skeleton_rawdev/skeleton_rawdev.c | 2 +- lib/librte_net/net_crc_neon.h | 4 ++-- 12 files changed, 12 insertions(+), 20 deletions(-)