RE: [PATCH] bitmap: add scan init at given position

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Volodymyr,

Thanks for your patch, comments below under your code:

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, June 1, 2023 4:26 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Volodymyr Fialko
> <vfialko@marvell.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; anoobj@marvell.com
> Subject: Re: [PATCH] bitmap: add scan init at given position
> 
> Cristian, please could you review this patch?
> 
> 14/04/2023 10:39, Volodymyr Fialko:
> > Currently, in the case when we search for a bit set after a particular
> > value, the bitmap has to be scanned from the beginning and
> > rte_bitmap_scan() has to be called multiple times until we hit the value.
> >
> > Add a new __rte_bitmap_scan_init_at() function to initialize scan state at
> > the given position, this will allow getting the next bit set after some
> > value within one rte_bitmap_scan() call.
> >
> > Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> [...]
> > +__rte_experimental
> > +static inline void
> > +__rte_bitmap_scan_init_at(struct rte_bitmap *bmp, uint32_t pos)
> > +{
> > +	bmp->index1 = pos >> (RTE_BITMAP_SLAB_BIT_SIZE_LOG2 +
> RTE_BITMAP_CL_BIT_SIZE_LOG2);
> > +	bmp->offset1 = (pos >> RTE_BITMAP_CL_BIT_SIZE_LOG2) &
> RTE_BITMAP_SLAB_BIT_MASK;
> > +	bmp->index2 = pos >> RTE_BITMAP_SLAB_BIT_SIZE_LOG2;
> > +	bmp->go2 = 1;
> > +}
> 
> It is supposed to be an internal (inlined) function
> but it is not used.
> 

My understanding is your proposed procedure for scanning starting at an offset is:
1. Call the new function: __rte_bitmap_scan_init_at()
2. Call the regular function: rte_bitmap_scan()

I think this procedure is not ideal, therefore I suggest we create a new API function which has an additional offset argument:

	rte_bitmap_scan_from_offset(struct rte_bitmap *bmp, uint32_t offset, uint32_t *pos, uint64_t *slab).

Under the hood, the new API should call an internal function similar to yours to start the scan at a given offset (while aborting any scan that might be in progress). Makes sense?

BTW, do we need to declare the experimental functions defined in a header file to the library map file? I don't see this in the patch, but the patch seems to compile and link fine ...

Regards,
Cristian