From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v4 2/2] ring: move the atomic load of head above the loop Date: Fri, 02 Nov 2018 10:36:33 +0100 Message-ID: <15443120.J8yKmjSJbt@xps> References: <1541066031-29125-1-git-send-email-gavin.hu@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: Stephen Hemminger , "dev@dpdk.org" , "olivier.matz@6wind.com" , "chaozhu@linux.vnet.ibm.com" , "bruce.richardson@intel.com" , "konstantin.ananyev@intel.com" , "jerin.jacob@caviumnetworks.com" , "stable@dpdk.org" , nd To: "Gavin Hu (Arm Technology China)" , Honnappa Nagarahalli Return-path: In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 02/11/2018 08:15, Gavin Hu (Arm Technology China): > > > -----Original Message----- > > From: Honnappa Nagarahalli > > Sent: Friday, November 2, 2018 12:31 PM > > To: Gavin Hu (Arm Technology China) ; Stephen > > Hemminger > > Cc: dev@dpdk.org; thomas@monjalon.net; olivier.matz@6wind.com; > > chaozhu@linux.vnet.ibm.com; bruce.richardson@intel.com; > > konstantin.ananyev@intel.com; jerin.jacob@caviumnetworks.com; > > stable@dpdk.org; nd > > Subject: RE: [PATCH v4 2/2] ring: move the atomic load of head above the > > loop > > > > > > > > > > On Thu, 1 Nov 2018 17:53:51 +0800 > > Gavin Hu wrote: > > > > > +* **Updated the ring library with C11 memory model.** > > > + > > > + Updated the ring library with C11 memory model including the following > > changes: > > > + > > > + * Synchronize the load and store of the tail > > > + * Move the atomic load of head above the loop > > > + > > > > Does this really need to be in the release notes? Is it a user visible change or > > just an internal/optimization and fix. > > > > [Gavin] There is no api changes, but this is a significant change as ring is > > fundamental and widely used, it decreases latency by 25% in our tests, it may > > do even better for cases with more contending producers/consumers or > > deeper depth of rings. > > > > [Honnappa] I agree with Stephen. Release notes should be written from > > DPDK user perspective. In the rte_ring case, the user has the option of > > choosing between c11 and non-c11 algorithms. Performance would be one > > of the criteria to choose between these 2 algorithms. IMO, it probably makes > > sense to indicate that the performance of c11 based algorithm has been > > improved. However, I do not know what DPDK has followed historically > > regarding performance optimizations. I would prefer to follow whatever has > > been followed so far. > > I do not think that we need to document the details of the internal changes > > since it does not help the user make a decision. > > I read through the online guidelines for release notes, besides API and new features, resolved issues which are significant and not newly introduced in this release cycle, should also be included. > In this case, the resolved issue existed for long time, across multiple release cycles and ring is a core lib, so it should be a candidate for release notes. > > https://doc.dpdk.org/guides-18.08/contributing/patches.html > section 5.5 says: > Important changes will require an addition to the release notes in doc/guides/rel_notes/. > See the Release Notes section of the Documentation Guidelines for details. > https://doc.dpdk.org/guides-18.08/contributing/documentation.html#doc-guidelines > "Developers should include updates to the Release Notes with patch sets that relate to any of the following sections: > New Features > Resolved Issues (see below) > Known Issues > API Changes > ABI Changes > Shared Library Versions > Resolved Issues should only include issues from previous releases that have been resolved in the current release. Issues that are introduced and then fixed within a release cycle do not have to be included here." > > Suggested order in release notes items: > * Core libs (EAL, mempool, ring, mbuf, buses) > * Device abstraction libs and PMDs I agree with Honnappa. You don't need to give details, but can explain that performance of C11 version is improved.