[dpdk-dev] [RFC v6 2/6] lib/ring: apis to support configurable element size

[Honnappa Nagarahalli]

> 
> 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.nagarahalli at arm.com>
> > Reviewed-by: Dharmik Thakkar <dharmik.thakkar at arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu at arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang at 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.