From: "Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Ilya Maximets <i.maximets@samsung.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: nd <nd@arm.com>, "thomas@monjalon.net" <thomas@monjalon.net>,
"jerinj@marvell.com" <jerinj@marvell.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"Nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
"chaozhu@linux.vnet.ibm.com" <chaozhu@linux.vnet.ibm.com>
Subject: Re: [dpdk-dev] [PATCH v2] ring: enforce reading the tails before ring operations
Date: Fri, 8 Mar 2019 15:05:14 +0000 [thread overview]
Message-ID: <VI1PR08MB31670DE4179E1194F5410D428F4D0@VI1PR08MB3167.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB9772580136556E78@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, March 8, 2019 8:13 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Ilya
> Maximets <i.maximets@samsung.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com; Richardson,
> Bruce <bruce.richardson@intel.com>; chaozhu@linux.vnet.ibm.com
> Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
>
>
>
> > -----Original Message-----
> > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > Sent: Friday, March 8, 2019 4:23 AM
> > To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> olivier.matz@6wind.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>;
> chaozhu@linux.vnet.ibm.com
> > Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
> >
> >
> >
> > > -----Original Message-----
> > > From: Gavin Hu (Arm Technology China)
> > > Sent: Thursday, March 7, 2019 6:45 PM
> > > To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com;
> > > bruce.richardson@intel.com; konstantin.ananyev@intel.com;
> > > chaozhu@linux.vnet.ibm.com
> > > Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ilya Maximets <i.maximets@samsung.com>
> > > > Sent: Thursday, March 7, 2019 5:48 PM
> > > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > > dev@dpdk.org
> > > > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > > > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > > > Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > > > operations
> > > >
> > > > On 07.03.2019 12:27, Gavin Hu (Arm Technology China) wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Ilya Maximets <i.maximets@samsung.com>
> > > > >> Sent: Thursday, March 7, 2019 4:52 PM
> > > > >> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > > > >> dev@dpdk.org
> > > > >> Cc: nd <nd@arm.com>; thomas@monjalon.net;
> jerinj@marvell.com;
> > > > >> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> > > Nagarahalli
> > > > >> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > > > >> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > > > >> operations
> > > > >>
> > > > >> On 07.03.2019 9:45, gavin hu wrote:
> > > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> may
> > > get
> > > > >>> reordered after reading or writing the ring slots, which corrupts
> the
> > > ring
> > > > >>> and stale data is observed.
> > > > >>>
> > > > >>> This issue was reported by NXP on 8-A72 DPAA2 board. The
> problem
> > > is
> > > > >> most
> > > > >>> likely caused by missing the acquire semantics when reading
> cons.tail
> > > (in
> > > > >>> SP enqueue) or prod.tail (in SC dequeue) which makes it possible
> to
> > > > read
> > > > >> a
> > > > >>> stale value from the ring slots.
> > > > >>>
> > > > >>> For MP (and MC) case, rte_atomic32_cmpset() already provides
> the
> > > > >> required
> > > > >>> ordering. This patch is to prevent reading and writing the ring
> slots get
> > > > >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> > > > >>
> > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > > >> reordered
> > > > >> before reading the tail. However, to prevent *writing* the ring get
> > > > >> reordered
> > > > >> *before reading* the tail you need a full memory barrier, i.e.
> > > > >> rte_smp_mb().
> > > > >
> > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > > WMB(ST
> > > > Option) orders ST/ST.
> > > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> and
> > > DSB
> > > > <option> parameter in
> > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > >
> > > > I see. But you have to change the rte_smp_rmb() function definition
> in
> > > > lib/librte_eal/common/include/generic/rte_atomic.h and assure that
> all
> > > > other architectures follows same rules.
> > > > Otherwise, this change is logically wrong, because read barrier in
> current
> > > > definition could not be used to order Load with Store.
> > > >
> > >
> > > Good points, let me re-think how to handle for other architectures.
> > > Full MB is required for other architectures(x86? Ppc?), but for arm,
> read
> > > barrier(load/store and load/load) is enough.
> >
> > Hi Ilya,
> >
> > I would expand the rmb definition to cover load/store, in addition to
> load/load.
> > For X86, as a strong memory order model, rmb is actually equivalent to
> mb,
>
> That's not exactly the case, on x86 we have:
> smp_rmb == compiler_barrier
> smp_mb is a proper memory barrier.
>
> Konstantin
Sorry I did not make it clear.
Anyway, on x86, smp_rmb, as a compiler barrier, applies to load/store, not only load/load.
This is the case also for arm, arm64, ppc32, ppc64.
I will submit a patch to expand the definition of this API.
>
> > as implemented as a compiler barrier: rte_compiler_barrier,
> > arm32 is also this case.
> > For PPC, both 32 and 64-bit, rmb=wmb=mb, lwsync/sync orders
> load/store, load/load, store/load, store/store, looking at the table on this
> > page:
> > https://www.ibm.com/developerworks/systems/articles/powerpc.html
> >
> > In summary, we are safe to expand this definition for all the
> architectures DPDK support?
> > Any comments are welcome!
> >
> > BR. Gavin
> >
next prev parent reply other threads:[~2019-03-08 15:05 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-06 3:07 [dpdk-dev] [PATCH v1] " gavin hu
[not found] ` <CGME20190306114906eucas1p19c2572b1fe777e1eb0ca96d2e47295bd@eucas1p1.samsung.com>
2019-03-06 11:49 ` [dpdk-dev] [v1] " Ilya Maximets
2019-03-07 6:50 ` Gavin Hu (Arm Technology China)
2019-03-07 6:45 ` [dpdk-dev] [PATCH v2] " gavin hu
2019-03-07 8:52 ` Ilya Maximets
2019-03-07 9:27 ` Gavin Hu (Arm Technology China)
2019-03-07 9:48 ` Ilya Maximets
2019-03-07 10:44 ` Gavin Hu (Arm Technology China)
2019-03-07 11:17 ` Ananyev, Konstantin
2019-03-08 3:21 ` Honnappa Nagarahalli
2019-03-08 5:27 ` Gavin Hu (Arm Technology China)
2019-03-08 16:33 ` Ananyev, Konstantin
2019-03-10 20:47 ` Honnappa Nagarahalli
2019-03-11 13:58 ` Ananyev, Konstantin
2019-03-08 4:23 ` Gavin Hu (Arm Technology China)
2019-03-08 5:06 ` Honnappa Nagarahalli
2019-03-08 12:13 ` Ananyev, Konstantin
2019-03-08 15:05 ` Gavin Hu (Arm Technology China) [this message]
2019-03-08 15:50 ` Ananyev, Konstantin
2019-03-08 23:18 ` Thomas Monjalon
2019-03-08 23:48 ` Honnappa Nagarahalli
2019-03-09 10:28 ` Gavin Hu (Arm Technology China)
2019-03-12 16:58 ` [dpdk-dev] [PATCH v3 0/1] ring: enforce reading the tail before reading ring slots Gavin Hu
2019-03-12 16:58 ` [dpdk-dev] [PATCH v3 1/1] " Gavin Hu
2019-03-13 8:12 ` Nipun Gupta
2019-03-15 13:26 ` Ananyev, Konstantin
2019-03-15 13:26 ` Ananyev, Konstantin
2019-03-28 0:21 ` Thomas Monjalon
2019-03-28 0:21 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VI1PR08MB31670DE4179E1194F5410D428F4D0@VI1PR08MB3167.eurprd08.prod.outlook.com \
--to=gavin.hu@arm.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=bruce.richardson@intel.com \
--cc=chaozhu@linux.vnet.ibm.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=i.maximets@samsung.com \
--cc=jerinj@marvell.com \
--cc=konstantin.ananyev@intel.com \
--cc=nd@arm.com \
--cc=nipun.gupta@nxp.com \
--cc=olivier.matz@6wind.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).