From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80040.outbound.protection.outlook.com [40.107.8.40]) by dpdk.org (Postfix) with ESMTP id DEA7C5F65 for ; Thu, 20 Sep 2018 17:20:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=nVPUKlv+gTWhTWHIf5Chj+OafVMvII8ubMJMfOkOdQM=; b=ZxEjhQLhM60PLBcWiPlsEvhIKO9e81xVUrnKH7uQpzwsEV6LoyKeTYbCsk3UNXVI6BiHpe5DEJ9nFYXbkRLycUDgp1SfU02N3DDma3Wt4YTH03zdIArxhEKMqE8oX0yxrEo/6JM7jaPsOah2AQAwaYaGp3ltU5Z6hChEGNCgOGY= Received: from AM6PR08MB3672.eurprd08.prod.outlook.com (20.177.115.29) by AM6PR08MB3094.eurprd08.prod.outlook.com (52.135.163.155) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1143.15; Thu, 20 Sep 2018 15:20:53 +0000 Received: from AM6PR08MB3672.eurprd08.prod.outlook.com ([fe80::589e:d3cf:9777:5ff9]) by AM6PR08MB3672.eurprd08.prod.outlook.com ([fe80::589e:d3cf:9777:5ff9%2]) with mapi id 15.20.1143.017; Thu, 20 Sep 2018 15:20:53 +0000 From: Honnappa Nagarahalli To: Jerin Jacob , "Phil Yang (Arm Technology China)" CC: "dev@dpdk.org" , nd , "kkokkilagadda@caviumnetworks.com" , "Gavin Hu (Arm Technology China)" , "ferruh.yigit@intel.com" Thread-Topic: [PATCH v2 2/3] kni: fix kni fifo synchronization Thread-Index: AQHUULwDi5PFjJpIFUyDUdvyMo0oh6T5RIGg Date: Thu, 20 Sep 2018 15:20:53 +0000 Message-ID: References: <1537363820-3827-1-git-send-email-phil.yang@arm.com> <1537364560-4124-1-git-send-email-phil.yang@arm.com> <1537364560-4124-2-git-send-email-phil.yang@arm.com> <20180920082846.GB19425@jerin> In-Reply-To: <20180920082846.GB19425@jerin> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Honnappa.Nagarahalli@arm.com; x-originating-ip: [217.140.111.135] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM6PR08MB3094; 6:z/xYRcP4d8VV3MxyqQRKwDK7ft4oyolgkg3lvrAitxqtMVyPGLE+KyhS90ONHteP5loz0XffCrvFNlupkgcnM6rmQOCCH2Ko316bPrp0bhTUZfjegDLUrs5eRHlTYxiAgZ4sGnLT67b7Vdu1EQD+YnMy/H76svJ/qk0KstPDqm8OmcYtSZ0H/nKCd3HTCLzJh7mHqH9h+dwv98dSBEEXrcWId2XjzVpz5q5m5mm1NVrH1haXLnTtHURkuMsGZKCvGf9SjbOkFaZX+N41cNPU8BNFZqdBnozF9TmMnRrz7JAJxoxf4HrKh36Q7HgGn7A95ROFo4nMo3g84C+8CXBAytuh53OSXInIO4mW7jsleNWoAXXkCD7pMKfFcCe7CGcmZ70Aq40RtgDnEMHcGPuuY+l0cMxgECEkAMK3g0VVL9b+53Dv5LGO//sRSG9D2yOPaZVfRk+6wfAj8bjxB97m7w==; 5:8NrnxIYN6n/+ZBqXYop2waoNtIaNB4HOVFD82me1/lVDHd+SYdIdMgKzat6Pwga7u7mu7g6RaUvg0NkeffqFwI2SRaiz+WSMxIwnX3RQY8C+xytSQ/2wREz2NsyLwUoYCEghdIUviPOuzVviSkoNTjz2O9HVwSdAwvTU6NnD+54=; 7:to5DgbeWsSh/VwT/VVD+gz3h451ZjyISiHgax4RDLMlBnCnPx2kit09r49ryIadsNVwwDtibpBGZ+S01QmiDRetuLMuvkT4Mq86am/S02EDJzM+kJQgN7roe5HgxP+7t1B0jVicFTDBit8nDerRb3JdxVGMJdEMd1JAX+B8ONzXElrR5EGxS+d/D1P1xzz1jxXLO6SKbbDlRNsLrOzCeNkQUZEE40IjZ4wY2qRG6YXvBObJUHtnBKjaOgJNBzuZT x-ms-exchange-antispam-srfa-diagnostics: SOS;SOR; x-ms-office365-filtering-correlation-id: a666b395-baa7-4c77-f544-08d61f0ca7ff x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(4534165)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:AM6PR08MB3094; x-ms-traffictypediagnostic: AM6PR08MB3094: nodisclaimer: True x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(228905959029699); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3002001)(10201501046)(93006095)(93001095)(3231355)(944501410)(52105095)(6055026)(149027)(150027)(6041310)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123562045)(20161123564045)(201708071742011)(7699051); SRVR:AM6PR08MB3094; BCL:0; PCL:0; RULEID:; SRVR:AM6PR08MB3094; x-forefront-prvs: 0801F2E62B x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(39860400002)(136003)(366004)(396003)(376002)(13464003)(57704003)(199004)(189003)(72206003)(55016002)(6246003)(25786009)(478600001)(4326008)(6436002)(8676002)(6636002)(186003)(14444005)(26005)(68736007)(5660300001)(8936002)(6506007)(86362001)(2906002)(99286004)(7696005)(102836004)(76176011)(229853002)(486006)(9686003)(14454004)(256004)(7736002)(476003)(97736004)(316002)(54906003)(93886005)(11346002)(74316002)(110136005)(305945005)(66066001)(5250100002)(106356001)(105586002)(53936002)(446003)(81156014)(81166006)(33656002)(3846002)(6116002)(2900100001)(71190400001)(71200400001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR08MB3094; H:AM6PR08MB3672.eurprd08.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: Q3lk2d3F9r7+2jkJVFkWTs7nmkT7wA6TCE+mX/UIPT8yuQjEg46g8TAXORat1NblZwhcaVeEft0hDoIGIumWVMETl0FJX7ej0APeQyPIfEtrqRf1yxtNTjBH2BJkOZSpNkZEiev4QxX0xn0H2A/DCvY8RhQtr3qImTupFvhuvcG8w8l33PHlaJaIaSpWYpdHlwyzxTyRobxtdEetNL3/kvB/ZF6JnRWzUlXdqAgc6CpzzJmshCUH5S0v/5eMR1j30K9dScfdHN/JyGkmmm6MOf2AabAXCLU4dxLzTSppuKop4JtG7XuxxCFLtWVuCHvjcBaNC4Xjpa6xzx3eb+0aLekGAvsNmXZKxPBTuyJrQRg= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: a666b395-baa7-4c77-f544-08d61f0ca7ff X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Sep 2018 15:20:53.6901 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR08MB3094 Subject: Re: [dpdk-dev] [PATCH v2 2/3] kni: fix kni fifo synchronization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Sep 2018 15:20:55 -0000 > -----Original Message----- > > Date: Wed, 19 Sep 2018 21:42:39 +0800 > > From: Phil Yang > > To: dev@dpdk.org > > CC: nd@arm.com, jerin.jacob@caviumnetworks.com, > > kkokkilagadda@caviumnetworks.com, Honnappa.Nagarahalli@arm.com, > > Gavin.Hu@arm.com > > Subject: [PATCH v2 2/3] kni: fix kni fifo synchronization > > X-Mailer: git-send-email 2.7.4 > > >=20 > + Ferruh Yigit >=20 > > > > With existing code in kni_fifo_put, rx_q values are not being updated > > before updating fifo_write. While reading rx_q in kni_net_rx_normal, > > This is causing the sync issue on other core. The same situation > > happens in kni_fifo_get as well. > > > > So syncing the values by adding C11 atomic memory barriers to make > > sure the values being synced before updating fifo_write and fifo_read. > > > > Fixes: 3fc5ca2 ("kni: initial import") > > Signed-off-by: Phil Yang > > Reviewed-by: Honnappa Nagarahalli > > Reviewed-by: Gavin Hu > > --- > > .../linuxapp/eal/include/exec-env/rte_kni_common.h | 5 ++++ > > lib/librte_kni/rte_kni_fifo.h | 30 ++++++++++++++= +++++++- > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > diff --git > > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h > > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h > > index cfa9448..1fd713b 100644 > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h > > @@ -54,8 +54,13 @@ struct rte_kni_request { > > * Writing should never overwrite the read position > > */ > > struct rte_kni_fifo { > > +#ifndef RTE_USE_C11_MEM_MODEL > > volatile unsigned write; /**< Next position to be written*/ > > volatile unsigned read; /**< Next position to be read */ > > +#else > > + unsigned write; /**< Next position to be written*/ > > + unsigned read; /**< Next position to be read */ > > +#endif > > unsigned len; /**< Circular buffer length */ > > unsigned elem_size; /**< Pointer size - for 32/64 bit = OS */ > > void *volatile buffer[]; /**< The buffer contains mbuf poin= ters */ > > diff --git a/lib/librte_kni/rte_kni_fifo.h > > b/lib/librte_kni/rte_kni_fifo.h index ac26a8c..f4171a1 100644 > > --- a/lib/librte_kni/rte_kni_fifo.h > > +++ b/lib/librte_kni/rte_kni_fifo.h > > @@ -28,8 +28,13 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void > > **data, unsigned num) { > > unsigned i =3D 0; > > unsigned fifo_write =3D fifo->write; > > - unsigned fifo_read =3D fifo->read; > > unsigned new_write =3D fifo_write; > > +#ifdef RTE_USE_C11_MEM_MODEL > > + unsigned fifo_read =3D __atomic_load_n(&fifo->read, > > + __ATOMIC_ACQUIRE); > > +#else > > + unsigned fifo_read =3D fifo->read; #endif >=20 > Correct. My apologies, did not follow your comment here. Do you want us to correct a= nything here? '#endif' is not appearing on the correct line in the email, b= ut it shows up fine on the patch work. >=20 >=20 > > > > for (i =3D 0; i < num; i++) { > > new_write =3D (new_write + 1) & (fifo->len - 1); @@ > > -39,7 +44,12 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void **data, > unsigned num) > > fifo->buffer[fifo_write] =3D data[i]; > > fifo_write =3D new_write; > > } > > +#ifdef RTE_USE_C11_MEM_MODEL > > + __atomic_store_n(&fifo->write, fifo_write, __ATOMIC_RELEASE); > > +#else > > + rte_smp_wmb(); > > fifo->write =3D fifo_write; > > +#endif >=20 > Correct. > > return i; > > } > > > > @@ -51,7 +61,12 @@ kni_fifo_get(struct rte_kni_fifo *fifo, void > > **data, unsigned num) { > > unsigned i =3D 0; > > unsigned new_read =3D fifo->read; > > +#ifdef RTE_USE_C11_MEM_MODEL > > + unsigned fifo_write =3D __atomic_load_n(&fifo->write, > > +__ATOMIC_ACQUIRE); #else > > unsigned fifo_write =3D fifo->write; > > +#endif >=20 > Correct. >=20 > > + > > for (i =3D 0; i < num; i++) { > > if (new_read =3D=3D fifo_write) > > break; > > @@ -59,7 +74,12 @@ kni_fifo_get(struct rte_kni_fifo *fifo, void **data, > unsigned num) > > data[i] =3D fifo->buffer[new_read]; > > new_read =3D (new_read + 1) & (fifo->len - 1); > > } > > +#ifdef RTE_USE_C11_MEM_MODEL > > + __atomic_store_n(&fifo->read, new_read, __ATOMIC_RELEASE); > > +#else > > + rte_smp_wmb(); > > fifo->read =3D new_read; > > +#endif >=20 > Correct. >=20 > > return i; > > } > > > > @@ -69,5 +89,13 @@ kni_fifo_get(struct rte_kni_fifo *fifo, void > > **data, unsigned num) static inline uint32_t kni_fifo_count(struct > > rte_kni_fifo *fifo) { > > +#ifdef RTE_USE_C11_MEM_MODEL > > + unsigned fifo_write =3D __atomic_load_n(&fifo->write, > > + __ATOMIC_ACQUIRE); > > + unsigned fifo_read =3D __atomic_load_n(&fifo->read, > > + __ATOMIC_ACQUIRE); >=20 > Isn't too heavy to have two __ATOMIC_ACQUIREs? a simple rte_smp_rmb() > would be enough here. Right? > or > Do we need __ATOMIC_ACQUIRE for fifo_write case? >=20 We also had some amount of debate internally on this: 1) We do not want to use rte_smp_rmb() as we want to keep the memory models= separated (for ex: while using C11, use C11 everywhere). It is also not su= fficient, please see 3) below. 2) This API can get called from writer or reader, so both the loads have to= be __ATOMIC_ACQUIRE 3) Other option is to use __ATOMIC_RELAXED. That would allow any loads/stor= es around of this API to get reordered, especially since this is an inline = function. This would put burden on the application to manage the ordering d= epending on its usage. It will also require the application to understand t= he implementation of this API. >=20 > Other than that, I prefer to avoid ifdef clutter by introducing two separ= ate file > just like ring C11 implementation. >=20 > I don't have strong opinion on this this part, I let KNI MAINTAINER to de= cide > on how to accommodate this change. I prefer to change this as well, I am open for suggestions. Introducing two separate files would be too much for this library. A better= way would be to have something similar to 'smp_store_release' provided by = the kernel. i.e. create #defines for loads/stores. Hide the clutter behind = the #defines. >=20 > > + return (fifo->len + fifo_write - fifo_read) & (fifo->len - 1); > > +#else > > return (fifo->len + fifo->write - fifo->read) & (fifo->len - > > 1); > > +#endif > > } > > -- > > 2.7.4 > >