On Wed, Sep 20, 2017 at 01:27:41PM +0000, Dumitrescu, Cristian wrote: > > -----Original Message----- > > From: Pavan Nikhilesh [mailto:pbhagavat...@caviumnetworks.com] > > Sent: Thursday, September 7, 2017 3:40 PM > > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; > > step...@networkplumber.org > > Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavat...@caviumnetworks.com> > > Subject: [dpdk-dev] [PATCH 1/2] eal: move bitmap from lib sched > > > > 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. > > > > Signed-off-by: Pavan Nikhilesh <pbhagavat...@caviumnetworks.com> > > --- > > 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 => librte_eal/common/include}/rte_bitmap.h > > (85%) > > > > 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 += rte_hexdump.h rte_devargs.h rte_bus.h > > rte_dev.h rte_vdev.h > > INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h > > INC += rte_malloc.h rte_keepalive.h rte_time.h > > INC += rte_service.h rte_service_component.h > > +INC += rte_bitmap.h > > > > GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h > > GENERIC_INC += 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" { > > ***/ > > > > #include <string.h> > > + > > #include <rte_common.h> > > #include <rte_debug.h> > > #include <rte_memory.h> > > @@ -72,36 +73,46 @@ extern "C" { > > #include <rte_prefetch.h> > > > > #ifndef RTE_BITMAP_OPTIMIZATIONS > > -#define RTE_BITMAP_OPTIMIZATIONS 1 > > +#define RTE_BITMAP_OPTIMIZATIONS 1 > > #endif > > > > /* 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) > > > > /* 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) > > > > -#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) > > > > /** 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 */ > > > > /* 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 */ > > > > /* 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 = (((bmp->index1 << > > RTE_BITMAP_SLAB_BIT_SIZE_LOG2) + bmp->offset1) << > > RTE_BITMAP_CL_SLAB_SIZE_LOG2); > > + bmp->index2 = (((bmp->index1 << > > RTE_BITMAP_SLAB_BIT_SIZE_LOG2) + > > + bmp->offset1) << > > RTE_BITMAP_CL_SLAB_SIZE_LOG2); > > } > > > > #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; > > > > - n_cache_lines_array2 = (n_bits + RTE_BITMAP_CL_BIT_SIZE - 1) / > > RTE_BITMAP_CL_BIT_SIZE; > > - n_slabs_array1 = (n_cache_lines_array2 + > > RTE_BITMAP_SLAB_BIT_SIZE - 1) / RTE_BITMAP_SLAB_BIT_SIZE; > > + n_cache_lines_array2 = (n_bits + RTE_BITMAP_CL_BIT_SIZE - 1) / > > + RTE_BITMAP_CL_BIT_SIZE; > > + n_slabs_array1 = (n_cache_lines_array2 + > > RTE_BITMAP_SLAB_BIT_SIZE - 1) / > > + RTE_BITMAP_SLAB_BIT_SIZE; > > n_slabs_array1 = rte_align32pow2(n_slabs_array1); > > - n_slabs_context = (sizeof(struct rte_bitmap) + > > (RTE_BITMAP_SLAB_BIT_SIZE / 8) - 1) / (RTE_BITMAP_SLAB_BIT_SIZE / 8); > > - n_cache_lines_context_and_array1 = (n_slabs_context + > > n_slabs_array1 + RTE_BITMAP_CL_SLAB_SIZE - 1) / > > RTE_BITMAP_CL_SLAB_SIZE; > > - n_bytes_total = (n_cache_lines_context_and_array1 + > > n_cache_lines_array2) * RTE_CACHE_LINE_SIZE; > > + n_slabs_context = (sizeof(struct rte_bitmap) + > > + (RTE_BITMAP_SLAB_BIT_SIZE / 8) - 1) / > > + (RTE_BITMAP_SLAB_BIT_SIZE / 8); > > + n_cache_lines_context_and_array1 = (n_slabs_context + > > n_slabs_array1 + > > + RTE_BITMAP_CL_SLAB_SIZE - 1) / > > RTE_BITMAP_CL_SLAB_SIZE; > > + n_bytes_total = (n_cache_lines_context_and_array1 + > > + n_cache_lines_array2) * RTE_CACHE_LINE_SIZE; > > > > if (array1_byte_offset) { > > *array1_byte_offset = n_slabs_context * > > (RTE_BITMAP_SLAB_BIT_SIZE / 8); > > @@ -185,7 +203,8 @@ __rte_bitmap_get_memory_footprint(uint32_t > > n_bits, > > *array1_slabs = n_slabs_array1; > > } > > if (array2_byte_offset) { > > - *array2_byte_offset = n_cache_lines_context_and_array1 * > > RTE_CACHE_LINE_SIZE; > > + *array2_byte_offset = n_cache_lines_context_and_array1 * > > + RTE_CACHE_LINE_SIZE; > > } > > if (array2_slabs) { > > *array2_slabs = 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 multiple > > 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 multiple > > 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 = pos >> RTE_BITMAP_SLAB_BIT_SIZE_LOG2; > > offset2 = pos & RTE_BITMAP_SLAB_BIT_MASK; > > - index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > > RTE_BITMAP_CL_BIT_SIZE_LOG2); > > + index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > > + RTE_BITMAP_CL_BIT_SIZE_LOG2); > > offset1 = (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) & > > RTE_BITMAP_SLAB_BIT_MASK; > > slab2 = bmp->array2 + index2; > > slab1 = bmp->array1 + index1; > > @@ -392,7 +418,8 @@ rte_bitmap_set_slab(struct rte_bitmap *bmp, > > uint32_t pos, uint64_t slab) > > > > /* Set bits in array2 slab and set bit in array1 slab */ > > index2 = pos >> RTE_BITMAP_SLAB_BIT_SIZE_LOG2; > > - index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > > RTE_BITMAP_CL_BIT_SIZE_LOG2); > > + index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > > + RTE_BITMAP_CL_BIT_SIZE_LOG2); > > offset1 = (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) & > > RTE_BITMAP_SLAB_BIT_MASK; > > slab2 = bmp->array2 + index2; > > slab1 = bmp->array1 + index1; > > @@ -449,7 +476,8 @@ rte_bitmap_clear(struct rte_bitmap *bmp, uint32_t > > pos) > > } > > > > /* The array2 cache line is all-zeros, so clear bit in array1 slab */ > > - index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > > RTE_BITMAP_CL_BIT_SIZE_LOG2); > > + index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 + > > + RTE_BITMAP_CL_BIT_SIZE_LOG2); > > offset1 = (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) & > > RTE_BITMAP_SLAB_BIT_MASK; > > slab1 = bmp->array1 + index1; > > *slab1 &= ~(1lu << offset1); > > @@ -500,7 +528,8 @@ __rte_bitmap_scan_read(struct rte_bitmap *bmp, > > uint32_t *pos, uint64_t *slab) > > uint64_t *slab2; > > > > slab2 = bmp->array2 + bmp->index2; > > - for ( ; bmp->go2 ; bmp->index2 ++, slab2 ++, bmp->go2 = bmp- > > >index2 & RTE_BITMAP_CL_SLAB_MASK) { > > + for ( ; bmp->go2 ; bmp->index2 ++, slab2 ++, > > + bmp->go2 = bmp->index2 & > > RTE_BITMAP_CL_SLAB_MASK) { > > if (*slab2) { > > *pos = bmp->index2 << > > RTE_BITMAP_SLAB_BIT_SIZE_LOG2; > > *slab = *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) += rte_sched.c > > rte_red.c rte_approx.c > > SRCS-$(CONFIG_RTE_LIBRTE_SCHED) += rte_reciprocal.c > > > > # install includes > > -SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include := rte_sched.h > > rte_bitmap.h rte_sched_common.h rte_red.h rte_approx.h > > -SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include += rte_reciprocal.h > > +SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include := rte_sched.h > > +SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include += rte_sched_common.h > > rte_red.h > > +SYMLINK-$(CONFIG_RTE_LIBRTE_SCHED)-include += rte_approx.h > > > > 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 <stdio.h> > > #include <string.h> > > > > +#include <rte_bitmap.h> > > #include <rte_common.h> > > #include <rte_log.h> > > #include <rte_memory.h> > > @@ -44,7 +45,6 @@ > > #include <rte_mbuf.h> > > > > #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 > allows 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. This > way, your patch will be very short and we don't need to worry about any bug > 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 <cristian.dumitre...@intel.com> > F: lib/librte_eal/common/include/rte_bitmap.h > F: test/test/test_bitmap.c > > Regards, > Cristian > Sure will spin up a v2.
Thanks, Pavan
