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 0DD305424 for ; Wed, 25 Jan 2017 14:54:07 +0100 (CET) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP; 25 Jan 2017 05:54:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,284,1477983600"; d="scan'208";a="57379882" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.61]) by fmsmga005.fm.intel.com with SMTP; 25 Jan 2017 05:54:05 -0800 Received: by (sSMTP sendmail emulation); Wed, 25 Jan 2017 13:54:04 +0000 Date: Wed, 25 Jan 2017 13:54:04 +0000 From: Bruce Richardson To: Olivier MATZ Cc: dev@dpdk.org Message-ID: <20170125135404.GA24352@bricha3-MOBL3.ger.corp.intel.com> References: <20170125121456.GA24344@bricha3-MOBL3.ger.corp.intel.com> <20170125142052.7989e0ec@glumotte.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170125142052.7989e0ec@glumotte.dev.6wind.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.7.1 (2016-10-04) 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 13:54:08 -0000 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, > > > > 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. :-( > > If yes, I was recently thinking almost the same about mempool stats. The > need to enable stats at compilation makes them less usable. On the > other hand, I feel the mempool/ring stats may be useful, for instance > to check if mbufs are used from mempool cache, and not from common pool. > > For mempool, my conclusion was: > - Enabling stats (debug) changes the ABI, because it adds a field in > the structure, this is bad > - enabling stats is not the same than enabling debug, we should have 2 > different ifdefs > - if statistics don't cost a lot, they should be enabled by default, > because it's a good debug tool (ex: have a stats for each access to > common pool) > > 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 ;) Definitely agree. As well as being a compile time option, I also think that it's the wrong way to solve the problem. If we want to break out of a loop like that early, then we should look to do a non-blocking version of the APIs with a subsequent tail update call. That way an app can decide per-ring when to sleep or context switch, or can even to do other work while it waits. However, I wouldn't be in a rush to implement that without a compelling use-case. > > > > * 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 > > >