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

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

Hi Bruce,

On Fri, 13 Jan 2017 15:00:54 +0000, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Fri, Jan 13, 2017 at 03:23:34PM +0100, Olivier Matz wrote:
> > 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
> >  
> 
> Hi Olivier,
> 
> Thanks for the feedback.
> 
> Yes, I thought about that parameterized sizes solution too, but I did
> not pursue it primarily because I was worried about the performance
> hits. It would mean that the actual copies of the data elements would
> have to be done via memcpy calls - or switches based on size - rather
> than assignments, as now. Given that all these calls to
> enqueue/dequeue are inlined, that could really hurt performance, as
> the size of the elements to be copied are unknown to the compiler at
> compile time - as the size is stored in the struct, and not available
> from the API call.

Maybe it's worth checking the impact. The size check could be done only
once per bulk, so it may not cost that much.

It's also possible to have a particular case for pointer size, and
use a memcpy for other sizes.


> The compiler type-checking, I really like, being a
> believer in having the compiler do as much work as possible for us,
> but it is a nice-to-have rather than a mandatory requirement. :-)
> 
> Am I right in assuming that the main issue that you see with the patch
> is the use of macros may lead to problems with maintainability with
> the code?

Yes, from my experience, having unusual macros leads to loose time when
trying to understand, use or change the code.


> For me, while macros may not be the nicest solution to the problem:
> * it does keep everything in rte_ring exactly as it was before - no
> API and ABI issues
> * it should be completely hidden from the end user - most applications
>   should never need to use the typed ring directly. Instead apps
> should instead use rte_ring and rte_event_ring headers.
> * The majority of the code is still regular C, and the macros don't
>   effect readability much IMHO. Also, it's comparatively rare that
> there are changes being made to the ring library. [Though I have a few
>   follow-on ideas myself!].
> * It gives us the maximum support from the compiler for type checking
>   and error reporting based on that 
> 
> This patchset is not for 17.02 so we have some time to consider our
> options, though I would like some resolution on this early in the
> 17.05 timeframe so as to reuse any solution inside any software
> eventdevs we create.


Yes, I hear your arguments. I don't have much to oppose except
readability. Hmm the fact that init functions become static inline
also bothers me a bit. All functions are static inline, so it closes the
door to de-inline functions in the future.

I think having a performance test showing storing the elt size in the
ring structure has a deep impact would help to reach a consensus
faster :)


Regards,
Olivier