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

[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...@arm.com>; Stephen
> Hemminger <step...@networkplumber.org>
> Cc: dev@dpdk.org; tho...@monjalon.net; olivier.m...@6wind.com;
> chao...@linux.vnet.ibm.com; bruce.richard...@intel.com;
> konstantin.anan...@intel.com; jerin.ja...@caviumnetworks.com;
> sta...@dpdk.org; nd <n...@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...@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