On 4/3/2020 10:49 PM, Andrzej Ostruszka wrote:
Hello Suanming
Please find my comments below. However please note that so far I have
never used DPDK bitmaps so I might not be the best person to comment -
this patch needs some attention so I spent some time on it.
Overall I'm fine with the changes however since this is a performance
enhancement I've added some remarks/comments.
Hi Andrzej , thanks for your suggestions.
On 3/10/20 9:21 AM, Suanming Mou wrote:
Currently, in the case to use bitmap as resource allocator, after
bitmap creation, all the bitmap bits should be set to indicate the
bit available. Every time when allocate one bit, search for the set
bits and clear it to make it in use.
Add a new rte_bitmap_init_with_all_set() function to have a quick
fill up the bitmap bits.
Comparing with the case create the bitmap as empty and set the bitmap
one by one, the new function costs less cycles.
Signed-off-by: Suanming Mou <suanmi...@mellanox.com>
---
lib/librte_eal/common/include/rte_bitmap.h | 32 ++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/lib/librte_eal/common/include/rte_bitmap.h
b/lib/librte_eal/common/include/rte_bitmap.h
index 6b846f2..36b32e4 100644
--- a/lib/librte_eal/common/include/rte_bitmap.h
+++ b/lib/librte_eal/common/include/rte_bitmap.h
@@ -483,6 +483,38 @@ struct rte_bitmap {
return 0;
}
+/**
+ * Bitmap initialization with all bits set
+ *
+ * @param n_bits
+ * Number of pre-allocated bits in array2.
+ * @param mem
+ * Base address of array1 and array2.
+ * @param mem_size
+ * Minimum expected size of bitmap.
+ * @return
+ * Handle to bitmap instance.
+ */
+static inline struct rte_bitmap *
+rte_bitmap_init_with_all_set(uint32_t n_bits, uint8_t *mem, uint32_t mem_size)
+{
+ uint32_t i;
+ uint32_t slabs = n_bits / RTE_BITMAP_SLAB_BIT_SIZE;
+ struct rte_bitmap *bmp = rte_bitmap_init(n_bits, mem, mem_size);
+
+ if (!bmp)
+ return NULL;
+ /* Fill the arry2 byte aligned bits. */
+ memset(bmp->array2, 0xff, slabs * sizeof(bmp->array2[0]));
In rte_bitmap_init() we clear memory with 0 and now we set it with 1s.
Maybe separating the configuration from the actual initialization would
be better? So that you call __rte_bitmap_init() and later zero in
rte_bitmap_init() and set to 1s here.
Good idea. In fact, the first proposal was to add a new function which
can set all the n_bits.
Since currently, the bitmap struct does not contain n_bits, the
rte_bitmap_init_with_all_set() was introduced.
+ /* Fill the arry1 bits. */
+ for (i = 0; i < n_bits; i += RTE_BITMAP_CL_BIT_SIZE)
+ rte_bitmap_set(bmp, i);
Maybe you could here also compute the number of array1 bytes that can be
set to FF and use memset() and for the remaining user rte_bitmap_set()?
Right now you are also touching array2 memory which was already set above.
The RTE_BITMAP_CL_BIT_SIZE is 512 with cache_line size 64. Maybe for
most of the cases which creates the bitmap less than 4K bits will not
have chance with the memset.
Anyway, will add it.
+ /* Fill the arry2 left not byte aligned bits. */
+ for (i = slabs * RTE_BITMAP_SLAB_BIT_SIZE; i < n_bits; i++)
+ rte_bitmap_set(bmp, i);
+ return bmp;
+}
+
With regards
Andrzej Ostruszka
