Hi Volodymyr,
Thanks for your patch, comments below under your code:
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Thursday, June 1, 2023 4:26 PM
> To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Volodymyr Fialko
> <vfia...@marvell.com>
> Cc: dev@dpdk.org; jer...@marvell.com; ano...@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 <vfia...@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
