Re: [PATCH RFCv3 00/19] ring cleanup and generalization

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

Hi Bruce,

On Tue,  7 Feb 2017 14:12:38 +0000, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> This patchset make a set of, sometimes non-backward compatible,
> cleanup changes to the rte_ring code in order to improve it. The
> resulting code is shorter*, since the existing functions are
> restructured to reduce code duplication, as well as being more
> consistent in behaviour. The specific changes made are explained in
> each patch which makes that change.
> 
> Key incompatibilities:
> * The biggest, and probably most controversial change is that to the
>   enqueue and dequeue APIs. The enqueue/deq burst and bulk functions
> have their function prototypes changed so that they all return an
> additional parameter, indicating the size of next call which is
> guaranteed to succeed. In case on enq, this is the number of
> available slots on the ring, and in case of deq, it is the number of
> objects which can be pulled. As well as this, the return value from
> the bulk functions have been changed to make them compatible with the
> burst functions. In all cases, the functions to enq/deq a set of objs
> now return the number of objects processed, 0 or N, in the case of
> bulk functions, 0, N or any value in between in the case of the burst
> ones. [Due to the extra parameter, the compiler will flag all
> instances of the function to allow the user to also change the return
> value logic at the same time]
> * The parameters to the single object enq/deq functions have not been 
>   changed. Because of that, the return value is also unmodified - as
> the compiler cannot automatically flag this to the user.
> 
> Potential further cleanups:
> * To a certain extent the rte_ring structure has gone from being a
> whole ring structure, including a "ring" element itself, to just
> being a header which can be reused, along with the head/tail update
> functions to create new rings. For now, the enqueue code works by
> assuming that the ring data goes immediately after the header, but
> that can be changed to allow specialised ring implementations to put
> additional metadata of their own after the ring header. I didn't see
> this as being needed right now, but it may be worth considering for a
> V1 patchset.
> * There are 9 enqueue functions and 9 dequeue functions in
> rte_ring.h. I suspect not all of those are used, so personally I
> would consider dropping the functions to enqueue/dequeue a single
> value using single or multi semantics, i.e. drop 
>     rte_ring_sp_enqueue
>     rte_ring_mp_enqueue
>     rte_ring_sc_dequeue
>     rte_ring_mc_dequeue
>   That would still leave a single enqueue and dequeue function for
> working with a single object at a time.
> * It should be possible to merge the head update code for enqueue and
>   dequeue into a single function. The key difference between the two
> is the calculation of how far the index can be moved. I felt that the
>   functions for moving the head index are sufficiently complicated
> with many parameters to them already, that trying to merge in more
> code would impede readability. However, if so desired this change can
> be made at a later stage without affecting ABI or API.
> 
> PERFORMANCE:
> I've run performance autotests on a couple of (Intel) platforms.
> Looking particularly at the core-2-core results, which I expect are
> the main ones of interest, the performance after this patchset is a
> few cycles per packet faster in my testing. I'm hoping it should be
> at least neutral perf-wise.
> 
> REQUEST FOR FEEDBACK:
> * Are all of these changes worth making?

I've quickly browsed all the patches. I think yes, we should do it: it
brings a good cleanup, removing features we don't need, restructuring
the code, and also adding the feature you need :)


> * Should they be made in existing ring code, or do we look to provide
> a new fifo library to completely replace the ring one?

I think it's ok to have it in the existing code. Breaking the ABI
is never suitable, but I think having 2 libs would be even more
confusing.


> * How does the implementation of new ring types using this code
> compare vs that of the previous RFCs?

I prefer this version, especially compared to the first RFC.


Thanks for this big rework. I'll dive into the patches a do a more
exhaustive review soon.

Regards,
Olivier