From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH] drivers: cleanup unnecessary global variables Date: Thu, 26 Apr 2018 10:29:07 +0200 Message-ID: References: <20180419185159.11266-1-pbhagavatula@caviumnetworks.com> <20180423090009.vpzv74r3uknh4jnx@platinum> <20180425155158.GA14975@ltp-pvn> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org To: Pavan Nikhilesh , thomas@monjalon.net, jerin.jacob@caviumnetworks.com, hemant.agrawal@nxp.com, beilei.xing@intel.com, rasesh.mody@cavium.com, harish.patil@cavium.com, jianbo.liu@arm.com Return-path: Received: from mail-wr0-f194.google.com (mail-wr0-f194.google.com [209.85.128.194]) by dpdk.org (Postfix) with ESMTP id E50E26CC5 for ; Thu, 26 Apr 2018 10:30:20 +0200 (CEST) Received: by mail-wr0-f194.google.com with SMTP id d1-v6so55802407wrj.13 for ; Thu, 26 Apr 2018 01:30:20 -0700 (PDT) In-Reply-To: <20180425155158.GA14975@ltp-pvn> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Le 25 avril 2018 17:52:00 GMT+02:00, Pavan Nikhilesh a =C3=A9crit : >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 >> > --- >> > drivers/bus/dpaa/base/fman/netcfg_layer=2Ec | 5 ----- >> > drivers/bus/dpaa/base/qbman/bman_driver=2Ec | 4 ++-- >> > drivers/bus/dpaa/base/qbman/qman=2Ec | 2 +- >> > drivers/bus/dpaa/base/qbman/qman_driver=2Ec | 4 ++-- >> > drivers/bus/dpaa/base/qbman/qman_priv=2Eh | 1 - >> > drivers/bus/dpaa/dpaa_bus=2Ec | 2 +- >> > drivers/bus/fslmc/qbman/qbman_portal=2Ec | 3 +-- >> > drivers/bus/fslmc/qbman/qbman_portal=2Eh | 1 - >> > drivers/net/i40e/i40e_flow=2Ec | 2 +- >> > drivers/net/qede/base/bcm_osal=2Ec | 2 +- >> > drivers/raw/skeleton_rawdev/skeleton_rawdev=2Ec | 2 +- >> > lib/librte_net/net_crc_neon=2Eh | 4 ++-- >> > 12 files changed, 12 insertions(+), 20 deletions(-) >> >> [=2E=2E=2E] >> >> > diff --git a/lib/librte_net/net_crc_neon=2Eh >b/lib/librte_net/net_crc_neon=2Eh >> > index 63fa1d4a1=2E=2Ecb3da72ed 100644 >> > --- a/lib/librte_net/net_crc_neon=2Eh >> > +++ b/lib/librte_net/net_crc_neon=2Eh >> > @@ -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=2E >> >> 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()=2E >> >> Changing them to static will create an instance of these variables >for >> each included file, which is not what we want=2E >> >> I think that the proper way to solve it would be to add the >definition >> in a new =2Ec file, and only have a declaration in the =2Eh=2E >> >> >Hi Olivier, > >Thanks for the heads up, the second solution seems more viable and >while >implementing it I faced few Issues=2E GCC doesnt suport const vector >instructions >i=2Ee=2E the following assignment throw as compiler error=2E > > static const struct crc_pmull_ctx crc32_eth_pmull =3D { > =2Erk1_rk2 =3D vld1q_u64((uint64_t[2]){0xccaa009eLLU, 0x1751997d0LLU}= ), > =2Erk5_rk6 =3D vld1q_u64((uint64_t[2]){0xccaa009eLLU, 0x163cd6124LLU}= ), > =2Erk7_rk8 =3D 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) =3D >{ > =2Erk12 =3D {0xccaa009eLLU, 0x1751997d0LLU}, > =2Erk56 =3D {0xccaa009eLLU, 0x163cd6124LLU}, > =2Erk78 =3D {0x1f7011640LLU, 0x1db710641LLU}, > }; > > static const struct crc_pmull_ctx crc16_ccitt_pmull __rte_aligned(16) >=3D { > =2Erk12 =3D {0x189aeLLU, 0x8e10LLU}, > =2Erk56 =3D {0x189aeLLU, 0x114aaLLU}, > =2Erk78 =3D {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=2E > >Let me know if my solution seems viable I will send the v2=2E > Looks good, just wondering about possible endianness issues=2E Is arm arch= itecture 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=2E It could even enhance performance=2E Something like= : >> >> net_crc_neon=2Eh: >> >> static const struct crc_pmull_ctx crc32_eth_pmull =3D { >> >> } __rte_aligned(16); >> >> static const struct crc_pmull_ctx crc16_ccitt_pmull =3D { >> >> } __rte_aligned(16); >> > >Thanks, >Pavan=2E