From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 79DB7F04 for ; Wed, 20 Sep 2017 15:27:48 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Sep 2017 06:27:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,421,1500966000"; d="scan'208";a="137499049" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga002.jf.intel.com with ESMTP; 20 Sep 2017 06:27:44 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.167]) by IRSMSX107.ger.corp.intel.com ([169.254.10.65]) with mapi id 14.03.0319.002; Wed, 20 Sep 2017 14:27:42 +0100 From: "Dumitrescu, Cristian" To: Pavan Nikhilesh , "stephen@networkplumber.org" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 1/2] eal: move bitmap from lib sched Thread-Index: AQHTJ+dVMdP4eemg1UOioqhUYyVInaK91jjA Date: Wed, 20 Sep 2017 13:27:41 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BABD136@IRSMSX108.ger.corp.intel.com> References: <1504795187-11087-1-git-send-email-pbhagavatula@caviumnetworks.com> In-Reply-To: <1504795187-11087-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 1/2] eal: move bitmap from lib sched 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:27:49 -0000 > -----Original Message----- > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com] > Sent: Thursday, September 7, 2017 3:40 PM > To: Dumitrescu, Cristian ; > stephen@networkplumber.org > Cc: dev@dpdk.org; Pavan Nikhilesh > Subject: [dpdk-dev] [PATCH 1/2] eal: move bitmap from lib sched >=20 > The librte_sched uses rte_bitmap to manage large arrays of bits in an > optimized method so, moving it to eal/common would allow other libraries > and applications to use it. >=20 > Signed-off-by: Pavan Nikhilesh > --- > lib/librte_eal/common/Makefile | 1 + > .../common/include}/rte_bitmap.h | 100 +++++++++++++--= ------ > lib/librte_sched/Makefile | 5 +- > lib/librte_sched/rte_sched.c | 2 +- > 4 files changed, 70 insertions(+), 38 deletions(-) > rename lib/{librte_sched =3D> librte_eal/common/include}/rte_bitmap.h > (85%) >=20 > diff --git a/lib/librte_eal/common/Makefile > b/lib/librte_eal/common/Makefile > index e8fd67a..c2c6a7f 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_bitmap.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_bitmap.h > b/lib/librte_eal/common/include/rte_bitmap.h > similarity index 85% > rename from lib/librte_sched/rte_bitmap.h > rename to lib/librte_eal/common/include/rte_bitmap.h > index 010d752..0938c63 100644 > --- a/lib/librte_sched/rte_bitmap.h > +++ b/lib/librte_eal/common/include/rte_bitmap.h > @@ -65,6 +65,7 @@ extern "C" { > ***/ >=20 > #include > + > #include > #include > #include > @@ -72,36 +73,46 @@ extern "C" { > #include >=20 > #ifndef RTE_BITMAP_OPTIMIZATIONS > -#define RTE_BITMAP_OPTIMIZATIONS 1 > +#define RTE_BITMAP_OPTIMIZATIONS 1 > #endif >=20 > /* Slab */ > -#define RTE_BITMAP_SLAB_BIT_SIZE 64 > -#define RTE_BITMAP_SLAB_BIT_SIZE_LOG2 6 > -#define RTE_BITMAP_SLAB_BIT_MASK > (RTE_BITMAP_SLAB_BIT_SIZE - 1) > +#define RTE_BITMAP_SLAB_BIT_SIZE 64 > +#define RTE_BITMAP_SLAB_BIT_SIZE_LOG2 6 > +#define RTE_BITMAP_SLAB_BIT_MASK (RTE_BITMAP_SLAB_BIT_SIZE - > 1) >=20 > /* Cache line (CL) */ > -#define RTE_BITMAP_CL_BIT_SIZE (RTE_CACHE_LINE_SIZE * = 8) > -#define RTE_BITMAP_CL_BIT_SIZE_LOG2 > (RTE_CACHE_LINE_SIZE_LOG2 + 3) > -#define RTE_BITMAP_CL_BIT_MASK (RTE_BITMAP_CL_BIT_SIZE= - > 1) > +#define RTE_BITMAP_CL_BIT_SIZE (RTE_CACHE_LINE_SIZE * 8) > +#define RTE_BITMAP_CL_BIT_SIZE_LOG2 (RTE_CACHE_LINE_SIZE_LOG2 > + 3) > +#define RTE_BITMAP_CL_BIT_MASK (RTE_BITMAP_CL_BIT_SIZE - 1) >=20 > -#define RTE_BITMAP_CL_SLAB_SIZE (RTE_BITMAP_CL_BIT_SIZE= / > RTE_BITMAP_SLAB_BIT_SIZE) > -#define RTE_BITMAP_CL_SLAB_SIZE_LOG2 > (RTE_BITMAP_CL_BIT_SIZE_LOG2 - RTE_BITMAP_SLAB_BIT_SIZE_LOG2) > -#define RTE_BITMAP_CL_SLAB_MASK > (RTE_BITMAP_CL_SLAB_SIZE - 1) > +#define RTE_BITMAP_CL_SLAB_SIZE \ > + (RTE_BITMAP_CL_BIT_SIZE / RTE_BITMAP_SLAB_BIT_SIZE) > +#define RTE_BITMAP_CL_SLAB_SIZE_LOG2 \ > + (RTE_BITMAP_CL_BIT_SIZE_LOG2 - > RTE_BITMAP_SLAB_BIT_SIZE_LOG2) > +#define RTE_BITMAP_CL_SLAB_MASK (RTE_BITMAP_CL_SLAB_SIZE - 1) >=20 > /** Bitmap data structure */ > struct rte_bitmap { > /* Context for array1 and array2 */ > - uint64_t *array1; /**< Bitmap array1 */ > - uint64_t *array2; /**< Bitmap array2 */ > - uint32_t array1_size; /**< Number of 64-bit slabs in= array1 > that are actually used */ > - uint32_t array2_size; /**< Number of 64-bit slabs in= array2 > */ > + uint64_t *array1; > + /**< Bitmap array1 */ > + uint64_t *array2; > + /**< Bitmap array2 */ > + uint32_t array1_size; > + /**< Number of 64-bit slabs in array1 that are actually used */ > + uint32_t array2_size; > + /**< Number of 64-bit slabs in array2 */ >=20 > /* Context for the "scan next" operation */ > - uint32_t index1; /**< Bitmap scan: Index of current array1 slab */ > - uint32_t offset1; /**< Bitmap scan: Offset of current bit within > current array1 slab */ > - uint32_t index2; /**< Bitmap scan: Index of current array2 slab */ > - uint32_t go2; /**< Bitmap scan: Go/stop condition for current > array2 cache line */ > + uint32_t index1; > + /**< Bitmap scan: Index of current array1 slab */ > + uint32_t offset1; > + /**< Bitmap scan: Offset of current bit within current array1 slab */ > + uint32_t index2; > + /**< Bitmap scan: Index of current array2 slab */ > + uint32_t go2; > + /**< Bitmap scan: Go/stop condition for current array2 cache line */ >=20 > /* Storage space for array1 and array2 */ > uint8_t memory[]; > @@ -122,7 +133,8 @@ __rte_bitmap_mask1_get(struct rte_bitmap *bmp) > static inline void > __rte_bitmap_index2_set(struct rte_bitmap *bmp) > { > - bmp->index2 =3D (((bmp->index1 << > RTE_BITMAP_SLAB_BIT_SIZE_LOG2) + bmp->offset1) << > RTE_BITMAP_CL_SLAB_SIZE_LOG2); > + bmp->index2 =3D (((bmp->index1 << > RTE_BITMAP_SLAB_BIT_SIZE_LOG2) + > + bmp->offset1) << > RTE_BITMAP_CL_SLAB_SIZE_LOG2); > } >=20 > #if RTE_BITMAP_OPTIMIZATIONS > @@ -171,12 +183,18 @@ __rte_bitmap_get_memory_footprint(uint32_t > n_bits, > uint32_t n_cache_lines_array2; > uint32_t n_bytes_total; >=20 > - n_cache_lines_array2 =3D (n_bits + RTE_BITMAP_CL_BIT_SIZE - 1) / > RTE_BITMAP_CL_BIT_SIZE; > - n_slabs_array1 =3D (n_cache_lines_array2 + > RTE_BITMAP_SLAB_BIT_SIZE - 1) / RTE_BITMAP_SLAB_BIT_SIZE; > + n_cache_lines_array2 =3D (n_bits + RTE_BITMAP_CL_BIT_SIZE - 1) / > + RTE_BITMAP_CL_BIT_SIZE; > + n_slabs_array1 =3D (n_cache_lines_array2 + > RTE_BITMAP_SLAB_BIT_SIZE - 1) / > + RTE_BITMAP_SLAB_BIT_SIZE; > n_slabs_array1 =3D rte_align32pow2(n_slabs_array1); > - n_slabs_context =3D (sizeof(struct rte_bitmap) + > (RTE_BITMAP_SLAB_BIT_SIZE / 8) - 1) / (RTE_BITMAP_SLAB_BIT_SIZE / 8); > - n_cache_lines_context_and_array1 =3D (n_slabs_context + > n_slabs_array1 + RTE_BITMAP_CL_SLAB_SIZE - 1) / > RTE_BITMAP_CL_SLAB_SIZE; > - n_bytes_total =3D (n_cache_lines_context_and_array1 + > n_cache_lines_array2) * RTE_CACHE_LINE_SIZE; > + n_slabs_context =3D (sizeof(struct rte_bitmap) + > + (RTE_BITMAP_SLAB_BIT_SIZE / 8) - 1) / > + (RTE_BITMAP_SLAB_BIT_SIZE / 8); > + n_cache_lines_context_and_array1 =3D (n_slabs_context + > n_slabs_array1 + > + RTE_BITMAP_CL_SLAB_SIZE - 1) / > RTE_BITMAP_CL_SLAB_SIZE; > + n_bytes_total =3D (n_cache_lines_context_and_array1 + > + n_cache_lines_array2) * RTE_CACHE_LINE_SIZE; >=20 > if (array1_byte_offset) { > *array1_byte_offset =3D n_slabs_context * > (RTE_BITMAP_SLAB_BIT_SIZE / 8); > @@ -185,7 +203,8 @@ __rte_bitmap_get_memory_footprint(uint32_t > n_bits, > *array1_slabs =3D n_slabs_array1; > } > if (array2_byte_offset) { > - *array2_byte_offset =3D n_cache_lines_context_and_array1 * > RTE_CACHE_LINE_SIZE; > + *array2_byte_offset =3D n_cache_lines_context_and_array1 * > + RTE_CACHE_LINE_SIZE; > } > if (array2_slabs) { > *array2_slabs =3D n_cache_lines_array2 * > RTE_BITMAP_CL_SLAB_SIZE; > @@ -210,6 +229,7 @@ __rte_bitmap_scan_init(struct rte_bitmap *bmp) > * > * @param n_bits > * Number of bits in the bitmap > + * > * @return > * Bitmap memory footprint measured in bytes on success, 0 on error > */ > @@ -226,12 +246,14 @@ rte_bitmap_get_memory_footprint(uint32_t > n_bits) { > /** > * Bitmap initialization > * > - * @param mem_size > - * Minimum expected size of bitmap. > + * @param n_bits > + * Number of pre-allocated bits in array2. Must be non-zero and multip= le > of > + * 512. > * @param mem > * Base address of array1 and array2. > - * @param n_bits > - * Number of pre-allocated bits in array2. Must be non-zero and multip= le of > 512. > + * @param mem_size > + * Minimum expected size of bitmap. > + * > * @return > * Handle to bitmap instance. > */ > @@ -277,6 +299,7 @@ rte_bitmap_init(uint32_t n_bits, uint8_t *mem, > uint32_t mem_size) > * > * @param bmp > * Handle to bitmap instance > + * > * @return > * 0 upon success, error code otherwise > */ > @@ -312,6 +335,7 @@ rte_bitmap_reset(struct rte_bitmap *bmp) > * Handle to bitmap instance > * @param pos > * Bit position > + * > * @return > * 0 upon success, error code otherwise > */ > @@ -333,6 +357,7 @@ rte_bitmap_prefetch0(struct rte_bitmap *bmp, > uint32_t pos) > * Handle to bitmap instance > * @param pos > * Bit position > + * > * @return > * 0 when bit is cleared, non-zero when bit is set > */ > @@ -365,7 +390,8 @@ rte_bitmap_set(struct rte_bitmap *bmp, uint32_t > pos) > /* Set bit in array2 slab and set bit in array1 slab */ > index2 =3D pos >> RTE_BITMAP_SLAB_BIT_SIZE_LOG2; > offset2 =3D pos & RTE_BITMAP_SLAB_BIT_MASK; > - index1 =3D pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > RTE_BITMAP_CL_BIT_SIZE_LOG2); > + index1 =3D pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > + RTE_BITMAP_CL_BIT_SIZE_LOG2); > offset1 =3D (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) & > RTE_BITMAP_SLAB_BIT_MASK; > slab2 =3D bmp->array2 + index2; > slab1 =3D bmp->array1 + index1; > @@ -392,7 +418,8 @@ rte_bitmap_set_slab(struct rte_bitmap *bmp, > uint32_t pos, uint64_t slab) >=20 > /* Set bits in array2 slab and set bit in array1 slab */ > index2 =3D pos >> RTE_BITMAP_SLAB_BIT_SIZE_LOG2; > - index1 =3D pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > RTE_BITMAP_CL_BIT_SIZE_LOG2); > + index1 =3D pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > + RTE_BITMAP_CL_BIT_SIZE_LOG2); > offset1 =3D (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) & > RTE_BITMAP_SLAB_BIT_MASK; > slab2 =3D bmp->array2 + index2; > slab1 =3D bmp->array1 + index1; > @@ -449,7 +476,8 @@ rte_bitmap_clear(struct rte_bitmap *bmp, uint32_t > pos) > } >=20 > /* The array2 cache line is all-zeros, so clear bit in array1 slab */ > - index1 =3D pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > RTE_BITMAP_CL_BIT_SIZE_LOG2); > + index1 =3D pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > + RTE_BITMAP_CL_BIT_SIZE_LOG2); > offset1 =3D (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) & > RTE_BITMAP_SLAB_BIT_MASK; > slab1 =3D bmp->array1 + index1; > *slab1 &=3D ~(1lu << offset1); > @@ -500,7 +528,8 @@ __rte_bitmap_scan_read(struct rte_bitmap *bmp, > uint32_t *pos, uint64_t *slab) > uint64_t *slab2; >=20 > slab2 =3D bmp->array2 + bmp->index2; > - for ( ; bmp->go2 ; bmp->index2 ++, slab2 ++, bmp->go2 =3D bmp- > >index2 & RTE_BITMAP_CL_SLAB_MASK) { > + for ( ; bmp->go2 ; bmp->index2 ++, slab2 ++, > + bmp->go2 =3D bmp->index2 & > RTE_BITMAP_CL_SLAB_MASK) { > if (*slab2) { > *pos =3D bmp->index2 << > RTE_BITMAP_SLAB_BIT_SIZE_LOG2; > *slab =3D *slab2; > @@ -520,10 +549,10 @@ __rte_bitmap_scan_read(struct rte_bitmap *bmp, > uint32_t *pos, uint64_t *slab) > * > * @param bmp > * Handle to bitmap instance > - * @param pos > + * @param[out] pos > * When function call returns 1, pos contains the position of the next= set > * bit, otherwise not modified > - * @param slab > + * @param[out] slab > * When function call returns 1, slab contains the value of the entire= 64-bit > * slab where the bit indicated by pos is located. Slabs are always 64= -bit > * aligned, so the position of the first bit of the slab (this bit is = not > @@ -532,6 +561,7 @@ __rte_bitmap_scan_read(struct rte_bitmap *bmp, > uint32_t *pos, uint64_t *slab) > * after this slab, so the same slab will not be returned again if it > * contains more than one bit which is set. When function call returns= 0, > * slab is not modified. > + * > * @return > * 0 if there is no bit set in the bitmap, 1 otherwise > */ > diff --git a/lib/librte_sched/Makefile b/lib/librte_sched/Makefile > index 18274e7..072cd63 100644 > --- a/lib/librte_sched/Makefile > +++ b/lib/librte_sched/Makefile > @@ -55,7 +55,8 @@ 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 > +SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include :=3D rte_sched.h > +SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include +=3D rte_sched_common.h > rte_red.h > +SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include +=3D rte_approx.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..b3e0d4f 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -34,6 +34,7 @@ > #include > #include >=20 > +#include > #include > #include > #include > @@ -44,7 +45,6 @@ > #include >=20 > #include "rte_sched.h" > -#include "rte_bitmap.h" > #include "rte_sched_common.h" > #include "rte_approx.h" > #include "rte_reciprocal.h" > -- > 2.7.4 Hi Pavan, I think moving rte_bitmap.h to a common code area is a good idea, as it all= ows easier reuse by libs and apps. A few asks from my side: 1. Please do not do any cosmetic changes on rte_bitmap.h, it adds a lot of = code churn that makes review very difficult while adding no real value. Thi= s way, your patch will be very short and we don't need to worry about any b= ug introduction. 2. I want to retain maintainership of rte_bitmap.h, so please add a section= in MANTAINERS file under EAL: Bitmap M: Cristian Dumitrescu F: lib/librte_eal/common/include/rte_bitmap.h F: test/test/test_bitmap.c Regards, Cristian