Re: [dpdk-dev] rte_ring features in use (or not)

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Jan 25, 2017 at 01:54:04PM +0000, Bruce Richardson wrote:
> On Wed, Jan 25, 2017 at 02:20:52PM +0100, Olivier MATZ wrote:
> > On Wed, 25 Jan 2017 12:14:56 +0000, Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > > Hi all,
> > > 
> > > while looking at the rte_ring code, I'm wondering if we can simplify
> > > that a bit by removing some of the code it in that may not be used.
> > > Specifically:
> > > 
> > > * Does anyone use the NIC stats functionality for debugging? I've
> > >   certainly never seen it used, and it's presence makes the rest less
> > >   readable. Can it be dropped?
> > 
> > What do you call NIC stats? The stats that are enabled with
> > RTE_LIBRTE_RING_DEBUG?
> 
> Yes. By NIC I meant ring. :-(
> > 
<snip>
> > For the ring, in my opinion, the stats could be fully removed.
> 
> That is my thinking too. For mempool, I'd wait to see the potential
> performance hits before deciding whether or not to enable by default.
> Having them run-time enabled may also be an option too - if the branches
> get predicted properly, there should be little to no impact as we avoid
> all the writes to the stats, which is likely to be where the biggest hit
> is.
> 
> > 
> > 
> > > * RTE_RING_PAUSE_REP_COUNT is set to be disabled at build time, and
> > >   so does anyone actually use this? Can it be dropped?
> > 
> > This option looks like a hack to use the ring in conditions where it
> > should no be used (preemptable threads). And having a compile-time
> > option for this kind of stuff is not in vogue ;)
> 
<snip>
> > 
> > 
> > > * Who uses the watermarks feature as is? I know we have a sample app
> > >   that uses it, but there are better ways I think to achieve the same
> > >   goal while simplifying the ring implementation. Rather than have a
> > > set watermark on enqueue, have both enqueue and dequeue functions
> > > return the number of free or used slots available in the ring (in
> > > case of enqueue, how many free there are, in case of dequeue, how
> > > many items are available). Easier to implement and far more useful to
> > > the app.
> > 
> > +1
> > 
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 think it would be better if bulk and burst both returned the number
  enqueued, and only differed in the case of the behaviour when not all
  elements could be enqueued.
  
  That would mean an API change for enq_bulk, where it would return only
  0 or N, rather than 0 or negative. While we can map one set of return
  values to another inside the rte_ring library, I'm not sure I see a
  good reason to keep the old behaviour except for backward compatibility.
  Changing it makes it easier to switch between the two functions in
  code, and avoids confusion as to what the return values could be. Is
  it worth doing so? [My opinion is yes!]
  

Regards,
/Bruce