> -----Original Message----- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Bruce Richardson > Sent: Wednesday, January 25, 2017 4:58 PM > To: Wiles, Keith <keith.wi...@intel.com> > Cc: Olivier MATZ <olivier.m...@6wind.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] rte_ring features in use (or not) > > 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 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?
I am ok with changing API to make both _bulk and _burst return the same thing. Konstantin
