From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <konstantin.ananyev@intel.com>
Received: from mga14.intel.com (mga14.intel.com [192.55.52.115])
 by dpdk.org (Postfix) with ESMTP id 0903A37AC
 for <dev@dpdk.org>; Sat, 23 Jul 2016 12:15:00 +0200 (CEST)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
 by fmsmga103.fm.intel.com with ESMTP; 23 Jul 2016 03:14:54 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.28,408,1464678000"; d="scan'208";a="738624938"
Received: from irsmsx153.ger.corp.intel.com ([163.33.192.75])
 by FMSMGA003.fm.intel.com with ESMTP; 23 Jul 2016 03:14:53 -0700
Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by
 IRSMSX153.ger.corp.intel.com ([169.254.9.206]) with mapi id 14.03.0248.002;
 Sat, 23 Jul 2016 11:14:51 +0100
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Thomas Monjalon
 <thomas.monjalon@6wind.com>
CC: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>, "dev@dpdk.org"
 <dev@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH] lib: change rte_ring dequeue to guarantee
 ordering before tail update
Thread-Index: AQHR5MEAWScYtg9T2kKIsNyA6C8w66AlsRIAgAAV21A=
Date: Sat, 23 Jul 2016 10:14:51 +0000
Message-ID: <2601191342CEEE43887BDE71AB97725836B81292@irsmsx105.ger.corp.intel.com>
References: <20160715043951.32040-1-juhamatti.kuusisaari@coriant.com>
 <2601191342CEEE43887BDE71AB97725836B7E32F@irsmsx105.ger.corp.intel.com>
 <14017551.U6D1dIIx0P@xps13> <20160723060515.GA13747@localhost.localdomain>
 <CA+PrjLGmaYppXVLR190Qn3B-JQVsvY1ZHGQexvKXs0_MOt8FmA@mail.gmail.com>
 <20160723093621.GA18376@localhost.localdomain>
In-Reply-To: <20160723093621.GA18376@localhost.localdomain>
Accept-Language: en-IE, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH] lib: change rte_ring dequeue to guarantee
 ordering before tail update
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Sat, 23 Jul 2016 10:15:01 -0000

Hi lads,

> On Sat, Jul 23, 2016 at 11:02:33AM +0200, Thomas Monjalon wrote:
> > 2016-07-23 8:05 GMT+02:00 Jerin Jacob <jerin.jacob@caviumnetworks.com>:
> > > On Thu, Jul 21, 2016 at 11:26:50PM +0200, Thomas Monjalon wrote:
> > >> > > Consumer queue dequeuing must be guaranteed to be done fully
> > >> > > before the tail is updated. This is not guaranteed with a read b=
arrier, changed to a write barrier just before tail update which in
> practice guarantees correct order of reads and writes.
> > >> > >
> > >> > > Signed-off-by: Juhamatti Kuusisaari
> > >> > > <juhamatti.kuusisaari@coriant.com>
> > >> >
> > >> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > >>
> > >> Applied, thanks
> > >
> > > There was ongoing discussion on this
> > > http://dpdk.org/ml/archives/dev/2016-July/044168.html
> >
> > Sorry Jerin, I forgot this email.
> > The problem is that nobody replied to your email and you did not nack
> > the v2 of this patch.

It's probably my bad.
I acked the patch before Jerin response, and forgot to reply later.=20

> >
> > > This change may not be required as it has the performance impact.
> >
> > We need to clearly understand what is the performance impact (numbers
> > and use cases) on one hand, and is there a real bug fixed by this
> > patch on the other hand?
>=20
> IHMO, there is no real bug here. rte_smb_rmb() provides the LOAD-STORE ba=
rrier to make sure tail pointer WRITE happens only after prior
> LOADS.

Yep, from what I read at the link Jerin provided, indeed it seems rte_smp_r=
mb() is enough for the arm arch here...
For ppc, as I can see both rte_smp_rmb()/rte_smp_wmb() emits the same instr=
uction.

>=20
> Thoughts?

Wonder how big is a performance impact?
If there is a real one, I suppose we can revert the patch?
Konstantin=20

>=20
> >
> > Please guys make things clear and we'll revert if needed.