Re: [dpdk-stable] [PATCH v4 2/2] ring: move the atomic load of head above the loop

[Thomas Monjalon <thomas@monjalon.net>]

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) <Gavin.Hu@arm.com>; Stephen
> > Hemminger <stephen@networkplumber.org>
> > 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 <nd@arm.com>
> > Subject: RE: [PATCH v4 2/2] ring: move the atomic load of head above the
> > loop
> > 
> > <Fixing this to make the reply inline, making email plain text>
> > 
> > 
> > On Thu, 1 Nov 2018 17:53:51 +0800
> > Gavin Hu <mailto:gavin.hu@arm.com> 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.