Hi Bruce,
On Wed, 11 Jan 2017 15:05:14 +0000, Bruce Richardson
<bruce.richard...@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
