From: Bruce Richardson <bruce.richardson@intel.com>
To: "Wiles, Keith" <keith.wiles@intel.com>
Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] rte_ring features in use (or not)
Date: Wed, 25 Jan 2017 16:57:40 +0000 [thread overview]
Message-ID: <20170125165740.GA33248@bricha3-MOBL3.ger.corp.intel.com> (raw)
In-Reply-To: <647B0F09-667B-41D2-A8E1-964F71D4C365@intel.com>
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.richardson@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.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 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?
/Bruce
next prev parent reply other threads:[~2017-01-25 16:57 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-25 12:14 Bruce Richardson
2017-01-25 12:16 ` Bruce Richardson
2017-01-25 13:20 ` Olivier MATZ
2017-01-25 13:54 ` Bruce Richardson
2017-01-25 14:48 ` Bruce Richardson
2017-01-25 15:59 ` Wiles, Keith
2017-01-25 16:57 ` Bruce Richardson [this message]
2017-01-25 17:29 ` Ananyev, Konstantin
2017-01-31 10:53 ` Olivier Matz
2017-01-31 11:41 ` Bruce Richardson
2017-01-31 12:10 ` Bruce Richardson
2017-01-31 13:27 ` Olivier Matz
2017-01-31 13:46 ` Bruce Richardson
2017-01-25 22:27 ` Wiles, Keith
2017-01-25 16:39 ` Stephen Hemminger
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 00/19] ring cleanup and generalization Bruce Richardson
2017-02-14 8:32 ` Olivier Matz
2017-02-14 9:39 ` Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 01/19] app/pdump: fix duplicate macro definition Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 02/19] ring: remove split cacheline build setting Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 03/19] ring: create common structure for prod and cons metadata Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 04/19] ring: add a function to return the ring size Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 05/19] crypto/null: use ring size function Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 06/19] ring: eliminate duplication of size and mask fields Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 07/19] ring: remove debug setting Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 08/19] ring: remove the yield when waiting for tail update Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 09/19] ring: remove watermark support Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 10/19] ring: make bulk and burst fn return vals consistent Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 11/19] ring: allow enq fns to return free space value Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 12/19] examples/quota_watermark: use ring space for watermarks Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 13/19] ring: allow dequeue fns to return remaining entry count Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 14/19] ring: reduce scope of local variables Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 15/19] ring: separate out head index manipulation for enq/deq Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 16/19] ring: create common function for updating tail idx Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 17/19] ring: allow macros to work with any type of object Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 18/19] ring: add object size parameter to memory size calculation Bruce Richardson
2017-02-07 14:12 ` [dpdk-dev] [PATCH RFCv3 19/19] ring: add event ring implementation Bruce Richardson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170125165740.GA33248@bricha3-MOBL3.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=keith.wiles@intel.com \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).