Re: [PATCH 1/5] ring: allow rings with non power-of-2 sizes

On Fri, Jun 30, 2017 at 02:24:38PM +0200, Olivier Matz wrote:

On Fri, 30 Jun 2017 12:32:27 +0100, Bruce Richardson
[off-list ref] wrote:

quoted

On Fri, Jun 30, 2017 at 11:40:46AM +0200, Olivier Matz wrote:

quoted

Hi Bruce,

Few comments inline.

On Wed,  7 Jun 2017 14:36:16 +0100, Bruce Richardson [off-list ref] wrote:  

quoted

The rte_rings traditionally have only supported having ring sizes as powers
of 2, with the actual usable space being the size - 1. In some cases, for
example, with an eventdev where we want to precisely control queue depths
for latency, we need to allow ring sizes which are not powers of two so we
add in an additional ring capacity value to allow that. For existing rings,
this value will be size-1, i.e. the same as the mask, but if the new
EXACT_SZ flag is passed on ring creation, the ring will have exactly the
usable space requested, although the underlying memory size may be bigger.

Signed-off-by: Bruce Richardson <redacted>  

Specifying the exact wanted size looks to be a better API than the current
one, which is to give the power of two above the wanted value, knowing there
will be only size-1 elements available.

What would you think about deprecating ring init/creation without this
flag in a few versions? Then, later, we could remove this flag and
the new behavior would become the default.
 

I'm not really sure it's necessary. For the vast majority of cases what
we have now is fine, and it ensures that we maximise the ring space. I'd
tend to keep the new behaviour for those cases where we really need it.

It's a trade off between what we hide and expose:
* current scheme hides the fact that you get one entry less than you ask
  for, but the memory space is as expected.
* new scheme gives you exactly the entries you ask for, but hides the
  fact that you could be using up to double the memory space for the
  ring.

Yes, having 2 different behavior is precisely what I don't really like.
Giving the number of entries the user asks for is straightforward, which
is a good thing for an API. And hiding the extra consumed memory looks
acceptable, this can be documented.

That said, if we decide to deprecate it, there's no need to do it right
now.

quoted

quoted

quoted

@@ -845,69 +857,63 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p)
 }
 
 /**
- * Test if a ring is full.
+ * Return the number of entries in a ring.
  *
  * @param r
  *   A pointer to the ring structure.
  * @return
- *   - 1: The ring is full.
- *   - 0: The ring is not full.
+ *   The number of entries in the ring.
  */
-static inline int
-rte_ring_full(const struct rte_ring *r)
+static inline unsigned
+rte_ring_count(const struct rte_ring *r)
 {
 	uint32_t prod_tail = r->prod.tail;
 	uint32_t cons_tail = r->cons.tail;
-	return ((cons_tail - prod_tail - 1) & r->mask) == 0;
+	return (prod_tail - cons_tail) & r->mask;
 }  

When used on a mc/mp ring, this function can now return a value
which is higher than the ring capacity. Even if this function is
not really accurate when used while the ring is use, I think we
should maximize the result with r->capacity.

This will also avoid rte_ring_free_count() to return a overflowed
value.
  

How does it return an overflowing value? I think in the MP/MC case the
tests made each time around the loop for cmpset should ensure we never
overflow.

Here we are reading r->prod.tail and r->cons.tail without
synchronization, nor memory barriers. So basically, there is no
guarantee that the values are consistent.

Before, the returned value could be wrong, but always in the
interval [0, mask].

Now, rte_ring_count() is still in the interval [0, mask], but the
capacity of the ring is lower than mask. If rte_ring_count() returns
mask, it would cause rte_ring_free_count() to return a value lower than
0 (overflowed since the result is unsigned).

I'm still not convinced that this can actually happen, but just in case,
I'll add in the extra check in V2.

/Bruce