On Tue, Jan 31, 2017 at 11:41:42AM +0000, Bruce Richardson wrote:

On Tue, Jan 31, 2017 at 11:53:49AM +0100, Olivier Matz wrote:

quoted

On Wed, 25 Jan 2017 17:29:18 +0000, "Ananyev, Konstantin"
[off-list ref] wrote:

quoted

quoted

quoted

quoted

Bonus question:
* Do we know how widely used the enq_bulk/deq_bulk functions
are? They are useful for unit tests, so they do have uses, but
I think it would be good if we harmonized the return values
between bulk and burst functions. Right now:
   enq_bulk  - only enqueues all elements or none. Returns 0
for all, or negative error for none.
   enq_burst - enqueues as many elements as possible. Returns
the number enqueued.  

I do use the apis in pktgen and the difference in return values
has got me once. Making them common would be great,  but the
problem is  

backward compat to old versions I would need to have an ifdef in
pktgen now. So it seems like we moved the problem to the
application.  

quoted

Yes, an ifdef would be needed, but how many versions of DPDK back
do you support? Could the ifdef be removed again after say, 6
months? 

quoted

I would like to see the old API kept and a new API with the new
behavior. I know it adds another API but one of the API would be
nothing  

more than wrapper function if not a macro.  

quoted

Would that be more reasonable then changing the ABI?  

Technically, this would be an API rather than ABI change, since the
functions are inlined in the code. However, it's not the only API
change I'm looking to make here - I'd like to have all the
functions start returning details of the state of the ring, rather
than have the watermarks facility. If we add all new functions for
this and keep the old ones around, we are just increasing our
maintenance burden.

I'd like other opinions here. Do we see increasing the API surface
as the best solution, or are we ok to change the APIs of a key
library like the rings one?  

I am ok with changing API to make both _bulk and _burst return the
same thing. Konstantin 

I agree that the _bulk() functions returning 0 or -err can be confusing.
But it has at least one advantage: it explicitly shows that if user ask
for N enqueues/dequeues, it will either get N or 0, not something
between.

Changing the API of the existing _bulk() functions looks a bit
dangerous to me. There's probably a lot of code relying on the ring
API, and changing its behavior may break it.

I'd prefer to deprecate the old _bulk and _burst functions, and
introduce a new api, maybe something like:

  rte_ring_generic_dequeue(ring, objs, n, behavior, flags)
  -> return nb_objs or -err

Don't like the -err, since it's not a valid value that can be used e.g.
in simple loops in the case that the user doesn't care about the exact
reason for error. I prefer having zero returned on error, with rte_errno
set appropriately, since then it is trivial for apps to ignore error
values they don't care about.
It also makes the APIs in a ring library consistent in that all will set
rte_errno on error, rather than returning the error code. It's not right
for rte_ring_create and rte_ring_lookup to return an error code since
they return pointers, not integer values.

As for deprecating the functions - I'm not sure about that. I think the
names of the existing functions are ok, and should be kept. I've a new
patchset of cleanups for rte_rings in the works. Let me try and finish
that and send it out as an RFC and we'll see what you think then.

Sorry, I realised on re-reading this reply seemed overly negative,
sorry. I can actually see the case for deprecating both sets of
functions to allow us to "start afresh". If we do so, are we as well to
just replace the whole library with a new one, e.g. rte_fifo, which
would allow us the freedom to keep e.g. functions with "burst" in the
name if we so wish? If might also allow an easier transition.

Regards,
/Bruce