On Tue, Feb 14, 2017 at 09:32:20AM +0100, Olivier Matz wrote:

Hi Bruce,

On Tue,  7 Feb 2017 14:12:38 +0000, Bruce Richardson
[off-list ref] wrote:

quoted

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 :)

quoted

* 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.

quoted

* 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.

Great, thanks. I'm aware of a few things that already need to be cleaned
up for V1 e.g. comments are not always correctly updated on functions.

/Bruce