> On Jan 25, 2017, at 9:57 AM, Richardson, Bruce <bruce.richard...@intel.com> > wrote: > > On Wed, Jan 25, 2017 at 03:59:55PM +0000, Wiles, Keith wrote: >> >> >> Sent from my iPhone >> >>> On Jan 25, 2017, at 7:48 AM, Bruce Richardson <bruce.richard...@intel.com> >>> wrote: >>> >>>> 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.richard...@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 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. >> > > 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?
I have people trying to run 2.1 and 2.2 versions of Pktgen. I can cut them off, but I would prefer not to. > >> 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. >> >> 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? > > /Bruce Regards, Keith
