From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
by dpdk.org (Postfix) with ESMTP id EF3A75F16;
Mon, 5 Nov 2018 10:44:54 +0100 (CET)
Received: from rsa59-2-82-233-193-189.fbx.proxad.net ([82.233.193.189]
helo=droids-corp.org)
by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256)
(Exim 4.89) (envelope-from <olivier.matz@6wind.com>)
id 1gJbST-0003lr-Ho; Mon, 05 Nov 2018 10:46:11 +0100
Received: by droids-corp.org (sSMTP sendmail emulation);
Mon, 05 Nov 2018 10:44:45 +0100
Date: Mon, 5 Nov 2018 10:44:45 +0100
From: Olivier Matz <olivier.matz@6wind.com>
To: "Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>, "thomas@monjalon.net" <thomas@monjalon.net>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"chaozhu@linux.vnet.ibm.com" <chaozhu@linux.vnet.ibm.com>,
"konstantin.ananyev@intel.com" <konstantin.ananyev@intel.com>,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
"stable@dpdk.org" <stable@dpdk.org>
Message-ID: <20181105094445.oc36ksxstg56ztkc@platinum>
References: <1541066031-29125-1-git-send-email-gavin.hu@arm.com>
<1541157688-40012-3-git-send-email-gavin.hu@arm.com>
<20181102114344.GA13324@bricha3-MOBL.ger.corp.intel.com>
<VI1PR08MB316746F49E356F622E703BB18FC80@VI1PR08MB3167.eurprd08.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <VI1PR08MB316746F49E356F622E703BB18FC80@VI1PR08MB3167.eurprd08.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [dpdk-stable] [PATCH v5 2/2] ring: move the atomic load of head
above the loop
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 05 Nov 2018 09:44:55 -0000
Hi,
On Sat, Nov 03, 2018 at 01:19:29AM +0000, Gavin Hu (Arm Technology China) wrote:
>
>
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Friday, November 2, 2018 7:44 PM
> > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; stephen@networkplumber.org;
> > olivier.matz@6wind.com; chaozhu@linux.vnet.ibm.com;
> > konstantin.ananyev@intel.com; jerin.jacob@caviumnetworks.com;
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; stable@dpdk.org
> > Subject: Re: [PATCH v5 2/2] ring: move the atomic load of head above the
> > loop
> >
> > On Fri, Nov 02, 2018 at 07:21:28PM +0800, Gavin Hu wrote:
> > > In __rte_ring_move_prod_head, move the __atomic_load_n up and out
> > of
> > > the do {} while loop as upon failure the old_head will be updated,
> > > another load is costly and not necessary.
> > >
> > > This helps a little on the latency,about 1~5%.
> > >
> > > Test result with the patch(two cores):
> > > SP/SC bulk enq/dequeue (size: 8): 5.64 MP/MC bulk enq/dequeue (size:
> > > 8): 9.58 SP/SC bulk enq/dequeue (size: 32): 1.98 MP/MC bulk
> > > enq/dequeue (size: 32): 2.30
> > >
> > > Fixes: 39368ebfc606 ("ring: introduce C11 memory model barrier
> > > option")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > > Reviewed-by: Steve Capper <steve.capper@arm.com>
> > > Reviewed-by: Ola Liljedahl <Ola.Liljedahl@arm.com>
> > > Reviewed-by: Jia He <justin.he@arm.com>
> > > Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > Tested-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > ---
> > > doc/guides/rel_notes/release_18_11.rst | 7 +++++++
> > > lib/librte_ring/rte_ring_c11_mem.h | 10 ++++------
> > > 2 files changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/doc/guides/rel_notes/release_18_11.rst
> > > b/doc/guides/rel_notes/release_18_11.rst
> > > index 376128f..b68afab 100644
> > > --- a/doc/guides/rel_notes/release_18_11.rst
> > > +++ b/doc/guides/rel_notes/release_18_11.rst
> > > @@ -69,6 +69,13 @@ New Features
> > > checked out against that dma mask and rejected if out of range. If more
> > than
> > > one device has addressing limitations, the dma mask is the more
> > restricted one.
> > >
> > > +* **Updated the ring library with C11 memory model.**
> > > +
> > > + Updated the ring library with C11 memory model, in our tests the
> > > + changes decreased latency by 27~29% and 3~15% for MPMC and SPSC
> > cases respectively.
> > > + The real improvements may vary with the number of contending lcores
> > > + and the size of ring.
> > > +
> > Is this a little misleading, and will users expect massive performance
> > improvements generally? The C11 model seems to be used only on some,
> > but not all, arm platforms, and then only with "make" builds.
> >
> > config/arm/meson.build: ['RTE_USE_C11_MEM_MODEL', false]]
> > config/common_armv8a_linuxapp:CONFIG_RTE_USE_C11_MEM_MODEL=y
> > config/common_base:CONFIG_RTE_USE_C11_MEM_MODEL=n
> > config/defconfig_arm64-thunderx-linuxapp-
> > gcc:CONFIG_RTE_USE_C11_MEM_MODEL=n
> >
> > /Bruce
>
> Thank you Bruce for the review, to limit the scope of improvement, I rewrite the note as follows, could you help review? Feel free to change anything if you like.
> " Updated the ring library with C11 memory model, running ring_perf_autotest on Cavium ThunderX2 platform, the changes decreased latency by 27~29% and 3~15% for MPMC and SPSC cases (2 lcores) respectively. Note the changes help the relaxed memory ordering architectures (arm, ppc) only when CONFIG_RTE_USE_C11_MEM_MODEL=y was configured, no impact on strong memory ordering architectures like x86. To what extent they help the real use cases depends on other factors, like the number of contending readers/writers, size of the ring, whether or not it is on the critical path."
I prefer your initial proposal which is more concise. What about
something like this?
* **Updated the C11 memory model version of ring library.**
The latency is decreased for architectures using the C11 memory model
version of the ring library.
On Cavium ThunderX2 platform, the changes decreased latency by 27~29%
and 3~15% for MPMC and SPSC cases respectively (with 2 lcores). The
real improvements may vary with the number of contending lcores and
the size of ring.
About the patch itself:
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Thanks