>
> On Sun, Oct 20, 2019 at 07:22:56PM -0500, Honnappa Nagarahalli wrote:
> > Current APIs assume ring elements to be pointers. However, in many use
> > cases, the size can be different. Add new APIs to support configurable
> > ring element sizes.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > Reviewed-by: Dharmik Thakkar <dharmik.thak...@arm.com>
> > Reviewed-by: Gavin Hu <gavin...@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com>
> > ---
> > lib/librte_ring/Makefile | 3 +-
> > lib/librte_ring/meson.build | 4 +
> > lib/librte_ring/rte_ring.c | 44 +-
> > lib/librte_ring/rte_ring.h | 1 +
> > lib/librte_ring/rte_ring_elem.h | 946 +++++++++++++++++++++++++++
> > lib/librte_ring/rte_ring_version.map | 2 +
> > 6 files changed, 991 insertions(+), 9 deletions(-) create mode
> > 100644 lib/librte_ring/rte_ring_elem.h
>
> (...)
>
> > +/* the actual enqueue of pointers on the ring.
> > + * Placed here since identical code needed in both
> > + * single and multi producer enqueue functions.
> > + */
> > +#define ENQUEUE_PTRS_ELEM(r, ring_start, prod_head, obj_table, esize, n)
> do { \
> > + if (esize == 4) \
> > + ENQUEUE_PTRS_32(r, ring_start, prod_head, obj_table, n); \
> > + else if (esize == 8) \
> > + ENQUEUE_PTRS_64(r, ring_start, prod_head, obj_table, n); \
> > + else if (esize == 16) \
> > + ENQUEUE_PTRS_128(r, ring_start, prod_head, obj_table, n); \ }
> while
> > +(0)
>
> My initial thinking was that it could be a static inline functions instead of
> macros. I see that patches 5 and 6 are changing it. I wonder however if
> patches
> 5 and 6 shouldn't be merged and moved before this
> one: it would avoid to introduce new macros that will be removed after.
Patch 2, 5 and 6 implement different methods to do the copy of elements. We can
drop 5, as 6 proves to be better than 5 in my tests. The question on choosing
between 2 and 6 is still open. If we go with 2, I will convert the macros into
inline functions.
>
> (...)
>
> > +/**
> > + * @internal Enqueue several objects on the ring
> > + *
> > + * @param r
> > + * A pointer to the ring structure.
> > + * @param obj_table
> > + * A pointer to a table of void * pointers (objects).
> > + * @param esize
> > + * The size of ring element, in bytes. It must be a multiple of 4.
> > + * Currently, sizes 4, 8 and 16 are supported. This should be the same
> > + * as passed while creating the ring, otherwise the results are
> > undefined.
>
> The comment "It must be a multiple of 4" and "Currently, sizes 4, 8 and 16 are
> supported" are redundant (it appears several times in the file). The second
> one
> should be removed by patch 5 (I think it is missing?).
>
> But if patch 5 and 6 are moved before this one, only "It must be a multiple of
> 4" would be needed I think, and there would be no transition with only 3
> supported sizes.
(refer to the comment above) if 2 is chosen, then, I would like to remove the
restriction of limited sizes by adding a for loop around the 32b copy. 64b and
128b will remain the same to meet the existing performance.
