From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id A7B0811D4 for ; Tue, 14 Feb 2017 10:39:58 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga105.fm.intel.com with ESMTP; 14 Feb 2017 01:39:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,160,1484035200"; d="scan'208";a="44176015" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.61]) by orsmga002.jf.intel.com with SMTP; 14 Feb 2017 01:39:56 -0800 Received: by (sSMTP sendmail emulation); Tue, 14 Feb 2017 09:39:54 +0000 Date: Tue, 14 Feb 2017 09:39:54 +0000 From: Bruce Richardson To: Olivier Matz Cc: thomas.monjalon@6wind.com, keith.wiles@intel.com, konstantin.ananyev@intel.com, stephen@networkplumber.org, dev@dpdk.org Message-ID: <20170214093953.GA18792@bricha3-MOBL3.ger.corp.intel.com> References: <20170125121456.GA24344@bricha3-MOBL3.ger.corp.intel.com> <1486476777-24768-1-git-send-email-bruce.richardson@intel.com> <20170214093220.5d2c30e7@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170214093220.5d2c30e7@platinum> 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] [PATCH RFCv3 00/19] ring cleanup and generalization 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: Tue, 14 Feb 2017 09:39:59 -0000 On Tue, Feb 14, 2017 at 09:32:20AM +0100, Olivier Matz wrote: > Hi Bruce, > > On Tue, 7 Feb 2017 14:12:38 +0000, Bruce Richardson > wrote: > > This patchset make a set of, sometimes non-backward compatible, > > cleanup changes to the rte_ring code in order to improve it. The > > resulting code is shorter*, since the existing functions are > > restructured to reduce code duplication, as well as being more > > consistent in behaviour. The specific changes made are explained in > > each patch which makes that change. > > > > Key incompatibilities: > > * The biggest, and probably most controversial change is that to the > > enqueue and dequeue APIs. The enqueue/deq burst and bulk functions > > have their function prototypes changed so that they all return an > > additional parameter, indicating the size of next call which is > > guaranteed to succeed. In case on enq, this is the number of > > available slots on the ring, and in case of deq, it is the number of > > objects which can be pulled. As well as this, the return value from > > the bulk functions have been changed to make them compatible with the > > burst functions. In all cases, the functions to enq/deq a set of objs > > now return the number of objects processed, 0 or N, in the case of > > bulk functions, 0, N or any value in between in the case of the burst > > ones. [Due to the extra parameter, the compiler will flag all > > instances of the function to allow the user to also change the return > > value logic at the same time] > > * The parameters to the single object enq/deq functions have not been > > changed. Because of that, the return value is also unmodified - as > > the compiler cannot automatically flag this to the user. > > > > Potential further cleanups: > > * To a certain extent the rte_ring structure has gone from being a > > whole ring structure, including a "ring" element itself, to just > > being a header which can be reused, along with the head/tail update > > functions to create new rings. For now, the enqueue code works by > > assuming that the ring data goes immediately after the header, but > > that can be changed to allow specialised ring implementations to put > > additional metadata of their own after the ring header. I didn't see > > this as being needed right now, but it may be worth considering for a > > V1 patchset. > > * There are 9 enqueue functions and 9 dequeue functions in > > rte_ring.h. I suspect not all of those are used, so personally I > > would consider dropping the functions to enqueue/dequeue a single > > value using single or multi semantics, i.e. drop > > rte_ring_sp_enqueue > > rte_ring_mp_enqueue > > rte_ring_sc_dequeue > > rte_ring_mc_dequeue > > That would still leave a single enqueue and dequeue function for > > working with a single object at a time. > > * It should be possible to merge the head update code for enqueue and > > dequeue into a single function. The key difference between the two > > is the calculation of how far the index can be moved. I felt that the > > functions for moving the head index are sufficiently complicated > > with many parameters to them already, that trying to merge in more > > code would impede readability. However, if so desired this change can > > be made at a later stage without affecting ABI or API. > > > > PERFORMANCE: > > I've run performance autotests on a couple of (Intel) platforms. > > Looking particularly at the core-2-core results, which I expect are > > the main ones of interest, the performance after this patchset is a > > few cycles per packet faster in my testing. I'm hoping it should be > > at least neutral perf-wise. > > > > REQUEST FOR FEEDBACK: > > * Are all of these changes worth making? > > I've quickly browsed all the patches. I think yes, we should do it: it > brings a good cleanup, removing features we don't need, restructuring > the code, and also adding the feature you need :) > > > > * Should they be made in existing ring code, or do we look to provide > > a new fifo library to completely replace the ring one? > > I think it's ok to have it in the existing code. Breaking the ABI > is never suitable, but I think having 2 libs would be even more > confusing. > > > > * How does the implementation of new ring types using this code > > compare vs that of the previous RFCs? > > I prefer this version, especially compared to the first RFC. > > > Thanks for this big rework. I'll dive into the patches a do a more > exhaustive review soon. > Great, thanks. I'm aware of a few things that already need to be cleaned up for V1 e.g. comments are not always correctly updated on functions. /Bruce