Re: [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes

[Olivier Matz <olivier.matz@6wind.com>]

Hi Bruce,

On Wed, 11 Jan 2017 15:05:14 +0000, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> The rte_ring library in DPDK provides an excellent high-performance
> mechanism which can be used for passing pointers between cores and
> for other tasks such as buffering. However, it does have a number
> of limitations:
> 
> * type information of pointers is lost, as it works with void pointers
> * typecasting is needed when using enqueue/dequeue burst functions,
>   since arrays of other types cannot be automatically cast to void **
> * the data to be passed through the ring itself must be no bigger than
>   a pointer
> 
> While the first two limitations are an inconvenience, the final one is
> one that can prevent use of rte_rings in cases where their
> functionality is needed. The use-case which has inspired the patchset
> is that of eventdev. When working with rte_events, each event is a
> 16-byte structure consisting of a pointer and some metadata e.g.
> priority and type. For these events, what is passed around between
> cores is not pointers to events, but the events themselves. This
> makes existing rings unsuitable for use by applications working with
> rte_events, and also for use internally inside any software
> implementation of an eventdev.
> 
> For rings to handle events or other similarly sized structures, e.g.
> NIC descriptors, etc., we then have two options - duplicate rte_ring
> code to create new ring implementations for each of those types, or
> generalise the existing code using macros so that the data type
> handled by each rings is a compile time paramter. This patchset takes
> the latter approach, and once applied would allow us to add an
> rte_event_ring type to DPDK using a header file containing:
> 
> #define RING_TYPE struct rte_event
> #define RING_TYPE_NAME rte_event
> #include <rte_typed_ring.h>
> #undef RING_TYPE_NAME
> #undef RING_TYPE
> 
> [NOTE: the event_ring is not defined in this set, since it depends on
> the eventdev implementation not present in the main tree]
> 
> If we want to elimiate some of the typecasting on our code when
> enqueuing and dequeuing mbuf pointers, an rte_mbuf_ring type can be
> similarly created using the same number of lines of code.
> 
> The downside of this generalisation is that the code for the rings now
> has far more use of macros in it. However, I do not feel that overall
> readability suffers much from this change, the since the changes are
> pretty much just search-replace onces. There should also be no ABI
> compatibility issues with this change, since the existing rte_ring
> structures remain the same.

I didn't dive deeply in the patches, just had a quick look. I
understand the need, and even if I really don't like the "#define +
#include" way to create a new specific ring (for readability,
grepability), that may be a solution to your problem.

I think using a similar approach than in sys/queue.h would be even
worse in terms of readability.


What do you think about the following approach?

- add a new elt_size in rte_ring structure

- update create/enqueue/dequeue/... functions to manage the elt size

- change:
    rte_ring_enqueue_bulk(struct rte_ring *r,
      void * const *obj_table, unsigned n)
  to:
    rte_ring_enqueue_bulk(struct rte_ring *r, void *obj_table,
      unsigned n) 

  This relaxes the type for the API in the function. In the caller,
  the type of obj_table would be:
  - (void **) in case of a ring of pointers
  - (uint8_t *) in case of a ring of uint8_t
  - (struct rte_event *) in case of a ring of rte_event
  ...

  I think (I have not tested it) it won't break compilation since
  any type can be implicitly casted into a void *. Also, I'd say it
  is possible to avoid breaking the ABI.

- deprecate or forbid calls to:
    rte_ring_mp_enqueue(struct rte_ring *r, void *obj)
    (and similar)

  Because with a ring of pointers, obj is the pointer, passed by value.
  For other types, we would need
    rte_ring_mp_enqueue(struct rte_ring *r, <TYPE> obj)

  Maybe we could consider using a macro here.


The drawbacks I see are:
- a dynamic elt_size may slightly decrease performance
- it still uses casts to (void *), so there is no type checking


Regards,
Olivier