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. 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? > 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
