From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id AC87F293C for ; Wed, 25 Jan 2017 16:59:58 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jan 2017 07:59:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,284,1477983600"; d="scan'208";a="1087194013" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 25 Jan 2017 07:59:57 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 25 Jan 2017 07:59:57 -0800 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.230]) by fmsmsx124.amr.corp.intel.com ([169.254.8.190]) with mapi id 14.03.0248.002; Wed, 25 Jan 2017 07:59:57 -0800 From: "Wiles, Keith" To: "Richardson, Bruce" CC: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] rte_ring features in use (or not) Thread-Index: AQHSdwSwvh3h4M7sA0qJyI1ITrdeEaFJs8YAgAAJRwCAAA8cgP//jfLu Date: Wed, 25 Jan 2017 15:59:55 +0000 Message-ID: <647B0F09-667B-41D2-A8E1-964F71D4C365@intel.com> References: <20170125121456.GA24344@bricha3-MOBL3.ger.corp.intel.com> <20170125142052.7989e0ec@glumotte.dev.6wind.com> <20170125135404.GA24352@bricha3-MOBL3.ger.corp.intel.com>, <20170125144809.GA26936@bricha3-MOBL3.ger.corp.intel.com> In-Reply-To: <20170125144809.GA26936@bricha3-MOBL3.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] rte_ring features in use (or not) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Jan 2017 15:59:59 -0000 Sent from my iPhone > On Jan 25, 2017, at 7:48 AM, Bruce Richardson wrote: >=20 >> 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 >>> wrote: >>>> Hi all, >>>>=20 >>>> 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: >>>>=20 >>>> * 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? >>>=20 >>> What do you call NIC stats? The stats that are enabled with >>> RTE_LIBRTE_RING_DEBUG? >>=20 >> Yes. By NIC I meant ring. :-( >>>=20 > >>> For the ring, in my opinion, the stats could be fully removed. >>=20 >> 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. >>=20 >>>=20 >>>=20 >>>> * RTE_RING_PAUSE_REP_COUNT is set to be disabled at build time, and >>>> so does anyone actually use this? Can it be dropped? >>>=20 >>> 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 ;) >>=20 > >>>=20 >>>=20 >>>> * 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. >>>=20 >>> +1 >>>=20 > 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 compa= t 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 wra= pper function if not a macro.=20 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. >=20 > 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!] >=20 >=20 > Regards, > /Bruce