From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id ECAD9F94 for ; Wed, 20 Sep 2017 15:10:57 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP; 20 Sep 2017 06:10:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,421,1500966000"; d="scan'208";a="314217898" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by fmsmga004.fm.intel.com with ESMTP; 20 Sep 2017 06:10:55 -0700 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by irsmsx105.ger.corp.intel.com (163.33.3.28) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 20 Sep 2017 14:10:54 +0100 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.167]) by irsmsx112.ger.corp.intel.com ([169.254.1.142]) with mapi id 14.03.0319.002; Wed, 20 Sep 2017 14:10:54 +0100 From: "Dumitrescu, Cristian" To: Pavan Nikhilesh , "stephen@networkplumber.org" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v6 1/3] eal: introduce integer divide through reciprocal Thread-Index: AQHTJvojE7A8Qr+u3kW7tmmCdbbPjaK90hWw Date: Wed, 20 Sep 2017 13:10:53 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BABD0BA@IRSMSX108.ger.corp.intel.com> References: <1504693294-2100-1-git-send-email-pbhagavatula@caviumnetworks.com> In-Reply-To: <1504693294-2100-1-git-send-email-pbhagavatula@caviumnetworks.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v6 1/3] eal: introduce integer divide through reciprocal X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Sep 2017 13:10:58 -0000 Hi Pavan, I think moving rte_reciprocal.[hc] to a common code area like EAL is a very= good idea, so thanks for doing this work! One ask from my side: please do not change the existing code. 1. Keep the existing name for the 32-bit API functions and data structures = (no _u32 name suffix), add the _u64 suffix just for the new API functions t= hat you add. - If you want, you can create aliases with _32 name suffix for the existin= g 32-bit API - The only change to rte_sched.c should be header include line: #include "= rte_reciprocal.h" -> #include 2. Do not do any cosmetic changes in existing code in rte_reciprocal.[hc] (= such as adding CR+LF), as they result in lots of code churn for no real val= ue Once these changes are done, the size of your patch set is reduced consider= ably. > -----Original Message----- > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com] > Sent: Wednesday, September 6, 2017 11:22 AM > To: Dumitrescu, Cristian ; > stephen@networkplumber.org > Cc: dev@dpdk.org; Pavan Bhagavatula > > Subject: [dpdk-dev] [PATCH v6 1/3] eal: introduce integer divide through > reciprocal >=20 > From: Pavan Bhagavatula >=20 > In some use cases of integer division, denominator remains constant and > numerator varies. It is possible to optimize division for such specific > scenarios. >=20 > The librte_sched uses rte_reciprocal to optimize division so, moving it t= o > eal/common would allow other libraries and applications to use it. >=20 > Signed-off-by: Pavan Nikhilesh > Reviewed-by: Anatoly Burakov > --- >=20 > v6 changes: > - remove cache alignment from rte_reciprocal_u{32/64}structures as they > would > be embedded in other structures. >=20 > v5 changes: > - fix test print strings >=20 > v4 changes: > - minor fix for test cases > - fix u32 divisor generation >=20 > v3 changes: > - fix x86_32 compilation issue > - fix improper licence in test >=20 > v2 changes: > - fix compilation issues with .map files > - add test cases for correctness and performance > - remove extra licence inclusion > - fix coding style issues >=20 > lib/librte_eal/bsdapp/eal/Makefile | 1 + > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 7 +++= ++++ > lib/librte_eal/common/Makefile | 1 + > lib/{librte_sched =3D> librte_eal/common/include}/rte_reciprocal.h | 6 += +++-- > lib/{librte_sched =3D> librte_eal/common}/rte_reciprocal.c | 6 += +++-- > lib/librte_eal/linuxapp/eal/Makefile | 1 + > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 7 +++= ++++ > lib/librte_sched/Makefile | 2 -- > lib/librte_sched/rte_sched.c | 2 +- > 9 files changed, 26 insertions(+), 7 deletions(-) > rename lib/{librte_sched =3D> librte_eal/common/include}/rte_reciprocal.= h > (87%) > rename lib/{librte_sched =3D> librte_eal/common}/rte_reciprocal.c (96%) >=20 > diff --git a/lib/librte_eal/bsdapp/eal/Makefile > b/lib/librte_eal/bsdapp/eal/Makefile > index 005019e..56f9804 100644 > --- a/lib/librte_eal/bsdapp/eal/Makefile > +++ b/lib/librte_eal/bsdapp/eal/Makefile > @@ -88,6 +88,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D > malloc_elem.c > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D malloc_heap.c > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D rte_keepalive.c > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D rte_service.c > +SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D rte_reciprocal.c >=20 > # from arch dir > SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) +=3D rte_cpuflags.c > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map > b/lib/librte_eal/bsdapp/eal/rte_eal_version.map > index aac6fd7..90d7258 100644 > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map > @@ -237,3 +237,10 @@ EXPERIMENTAL { > rte_service_unregister; >=20 > } DPDK_17.08; > + > +DPDK_17.11 { > + global: > + > + rte_reciprocal_value; > + > +} DPDK_17.08; > diff --git a/lib/librte_eal/common/Makefile > b/lib/librte_eal/common/Makefile > index e8fd67a..a680b2d 100644 > --- a/lib/librte_eal/common/Makefile > +++ b/lib/librte_eal/common/Makefile > @@ -42,6 +42,7 @@ INC +=3D rte_hexdump.h rte_devargs.h rte_bus.h > rte_dev.h rte_vdev.h > INC +=3D rte_pci_dev_feature_defs.h rte_pci_dev_features.h > INC +=3D rte_malloc.h rte_keepalive.h rte_time.h > INC +=3D rte_service.h rte_service_component.h > +INC +=3D rte_reciprocal.h >=20 > GENERIC_INC :=3D rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.= h > GENERIC_INC +=3D rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h > diff --git a/lib/librte_sched/rte_reciprocal.h > b/lib/librte_eal/common/include/rte_reciprocal.h > similarity index 87% > rename from lib/librte_sched/rte_reciprocal.h > rename to lib/librte_eal/common/include/rte_reciprocal.h > index 5e21f09..b6d752f 100644 > --- a/lib/librte_sched/rte_reciprocal.h > +++ b/lib/librte_eal/common/include/rte_reciprocal.h > @@ -29,13 +29,15 @@ struct rte_reciprocal { > uint8_t sh1, sh2; > }; >=20 > -static inline uint32_t rte_reciprocal_divide(uint32_t a, struct rte_reci= procal > R) > +static inline uint32_t > +rte_reciprocal_divide(uint32_t a, struct rte_reciprocal R) > { > uint32_t t =3D (uint32_t)(((uint64_t)a * R.m) >> 32); >=20 > return (t + ((a - t) >> R.sh1)) >> R.sh2; > } >=20 > -struct rte_reciprocal rte_reciprocal_value(uint32_t d); > +struct rte_reciprocal > +rte_reciprocal_value(uint32_t d); >=20 Please remove these cosmetic changes that result in code churn for no real = value. > #endif /* _RTE_RECIPROCAL_H_ */ > diff --git a/lib/librte_sched/rte_reciprocal.c > b/lib/librte_eal/common/rte_reciprocal.c > similarity index 96% > rename from lib/librte_sched/rte_reciprocal.c > rename to lib/librte_eal/common/rte_reciprocal.c > index 652f023..7ab99b4 100644 > --- a/lib/librte_sched/rte_reciprocal.c > +++ b/lib/librte_eal/common/rte_reciprocal.c > @@ -41,7 +41,8 @@ > /* find largest set bit. > * portable and slow but does not matter for this usage. > */ > -static inline int fls(uint32_t x) > +static inline int > +fls(uint32_t x) > { > int b; >=20 > @@ -53,7 +54,8 @@ static inline int fls(uint32_t x) > return 0; > } >=20 > -struct rte_reciprocal rte_reciprocal_value(uint32_t d) > +struct rte_reciprocal > +rte_reciprocal_value(uint32_t d) > { > struct rte_reciprocal R; > uint64_t m; Please remove these cosmetic changes that result in code churn for no real = value. > diff --git a/lib/librte_eal/linuxapp/eal/Makefile > b/lib/librte_eal/linuxapp/eal/Makefile > index 90bca4d..98f3b8e 100644 > --- a/lib/librte_eal/linuxapp/eal/Makefile > +++ b/lib/librte_eal/linuxapp/eal/Makefile > @@ -100,6 +100,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D > malloc_elem.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D malloc_heap.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D rte_keepalive.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D rte_service.c > +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D rte_reciprocal.c >=20 > # from arch dir > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) +=3D rte_cpuflags.c > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map > b/lib/librte_eal/linuxapp/eal/rte_eal_version.map > index 3a8f154..2070cba 100644 > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map > @@ -242,3 +242,10 @@ EXPERIMENTAL { > rte_service_unregister; >=20 > } DPDK_17.08; > + > +DPDK_17.11 { > + global: > + > + rte_reciprocal_value; > + > +} DPDK_17.08; > diff --git a/lib/librte_sched/Makefile b/lib/librte_sched/Makefile > index 18274e7..569656b 100644 > --- a/lib/librte_sched/Makefile > +++ b/lib/librte_sched/Makefile > @@ -52,10 +52,8 @@ LIBABIVER :=3D 1 > # all source are stored in SRCS-y > # > SRCS-$(CONFIG_RTE_LIBRTE_SCHED) +=3D rte_sched.c rte_red.c rte_approx.c > -SRCS-$(CONFIG_RTE_LIBRTE_SCHED) +=3D rte_reciprocal.c >=20 > # install includes > SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include :=3D rte_sched.h > rte_bitmap.h rte_sched_common.h rte_red.h rte_approx.h > -SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include +=3D rte_reciprocal.h >=20 > include $(RTE_SDK)/mk/rte.lib.mk > diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c > index b7cba11..3b8ccaa 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -42,12 +42,12 @@ > #include > #include > #include > +#include >=20 > #include "rte_sched.h" > #include "rte_bitmap.h" > #include "rte_sched_common.h" > #include "rte_approx.h" > -#include "rte_reciprocal.h" >=20 > #ifdef __INTEL_COMPILER > #pragma warning(disable:2259) /* conversion may lose significant bits */ > -- > 2.7.4 Regards, Cristian