From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30063.outbound.protection.outlook.com [40.107.3.63]) by dpdk.org (Postfix) with ESMTP id 0D5BEF04 for ; Fri, 21 Sep 2018 07:48:45 +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=ZW1UCZIwzH7H78UhrdaQ9vSJWCEHG28QoqrWBDocyeQ=; b=MsSTQZ5f+ViExJrNwK7aFvIwJICPOVamUdvhxR4qqSsB+DzSIugdozNJ0sBecpEv4GF7Qm6XhUMvfIXbrYFNbx0gXoOUGPX+pcSQNV6CW3EemCAlopntawoD5r2djfKTdwr1RYYS73vu8AvTWsc0r4G12L3fGo6s3fx7Nmn1As4= Received: from AM6PR08MB3672.eurprd08.prod.outlook.com (20.177.115.29) by AM6PR08MB3670.eurprd08.prod.outlook.com (20.177.115.27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1143.18; Fri, 21 Sep 2018 05:48:44 +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; Fri, 21 Sep 2018 05:48:44 +0000 From: Honnappa Nagarahalli To: Jerin Jacob CC: "Phil Yang (Arm Technology China)" , "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: AQHUULwDi5PFjJpIFUyDUdvyMo0oh6T5RIGggAAJlACAAOYDgA== Date: Fri, 21 Sep 2018 05:48:44 +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> <20180920153700.GA9459@jerin> In-Reply-To: <20180920153700.GA9459@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; AM6PR08MB3670; 6:BMjGFEQFZhhO1dZLUKrGeqUJoMEU5KFDstEmwsJXe4nivMjmcyof2gdRj1R5DoN45gneLIfaBt1/BM+r1nKtB62NRlynYwtbZ1ZufpzOyryCGueomaFeV4siLoYU/IhWgImytT8YPXwQFiaROZ5yKk9LK3kJMkFrBDkmsLXv+j5Qs2RbOtd/B/HfKmEa0BeEY2vYh1bp8i5zAWKCqLLZ3ET/+rhWFp1wIADsqKrMvjWd3uL+p0FNwY4Ok3IY2HjX7Nd/Mik9JTIUEYR9/4A6lf251VLtdbEOFsNvivsT4rBNC7ZB0fzaYFJmcezKYT9KpdONlQFGeiYz1s1abplQHw84gyLA4A7EvVk/bmkuXl+TImBWCq77RTQFvAVh/5JM9kDM8DgAzNQIzoP4r2JfBlCc1Pv8m4aiWu7bTqg76U39mSlXQ4rmbeMLwinz/YBYghFEc5ILUsAJF45v+VvrqA==; 5:KPN9c7f/3QQuXTPfzjVL5WfFGXqtUAc+fyh/UgslywpoaaZh0UTwM7sCZT2TysnCqfQP0/3eJZeBfpS+rmjQkeAPyJp1dHwKOTTS87iDkWmth1DxSjlJPhC/PMhiVwM+RkPo/aWIRI/8u2k/mhyHLyRBwyz5ACIoY9NgRBs1ww4=; 7:xdgfZSvwKd/c7K/kKX9lvaQhwSUB5IO3edZXkObpKow9U4+I2Ket3axa/C+NB7gi3mdW9pG4oJuvxQ/ZPQlpXrJ+SRbQAACEqoJdNeOxbmZtVFgbZis1I4bDZnY7AUHHy9XOV1ujXRwkiylyHOUFD9zjkEZqG6DWCP2jqQfEVgNH1zGLbaEC1jZkbIx/I3h6h/kOscArEe+bdWdsoYTnOU1oYPi+TfMbkYiGpO0GV4/T6jmd1eceJUlEuUgx3zzS x-ms-exchange-antispam-srfa-diagnostics: SOS;SOR; x-ms-office365-filtering-correlation-id: 42063fc1-8006-4996-4f50-08d61f85e4c1 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:AM6PR08MB3670; x-ms-traffictypediagnostic: AM6PR08MB3670: nodisclaimer: True x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(163750095850)(228905959029699); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3231355)(944501410)(52105095)(93006095)(93001095)(3002001)(10201501046)(6055026)(149027)(150027)(6041310)(20161123564045)(20161123558120)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(201708071742011)(7699051); SRVR:AM6PR08MB3670; BCL:0; PCL:0; RULEID:; SRVR:AM6PR08MB3670; x-forefront-prvs: 0802ADD973 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(376002)(346002)(396003)(366004)(39860400002)(136003)(189003)(199004)(13464003)(57704003)(478600001)(76176011)(72206003)(86362001)(6506007)(6916009)(2906002)(93886005)(2900100001)(102836004)(5660300001)(9686003)(6246003)(97736004)(486006)(316002)(7736002)(14454004)(305945005)(53936002)(26005)(7696005)(3846002)(11346002)(54906003)(476003)(105586002)(81156014)(446003)(33656002)(106356001)(6116002)(68736007)(81166006)(186003)(4326008)(6436002)(8936002)(25786009)(74316002)(8676002)(55016002)(99286004)(5250100002)(66066001)(14444005)(229853002)(71200400001)(256004)(71190400001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR08MB3670; H:AM6PR08MB3672.eurprd08.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: RY5J4txikXVQ/QrekRxAYGsIN6QxFM5529dIx8lc0jmnS/xWyXg47Da8XxFwHTbPviaFfwKzDLPVQF3O1CcyfdzOZa+egqWOfFW56D0mdPwZMs4DoWJn7iD2VVw4oOO5oAUmiKs7m9KBVVozjyb2mVSX0G4ZL0QDesext0SLaE3GRoxYXCnEiKrc+E4/ZrvqcfzJm3aZfskly/MMNLl0WLowkR8aCCVBoDD6xEbu9GjlO5LvVH1FseIRFp5X1wUIAu5iKvqrqGn90YEGDdJ8OulUndbFl5WH5kLYlk/J3HlapHMkrKdCSEPcNcj8Y+xXk62EqnlRpvrdIMduqNScxXDPTO7frv7nvbVF7TKsq6Q= 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: 42063fc1-8006-4996-4f50-08d61f85e4c1 X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Sep 2018 05:48:44.7638 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR08MB3670 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: Fri, 21 Sep 2018 05:48:46 -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 > > > > > > > > > > + Ferruh Yigit > > > > > > > > > > > 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_re= ad. > > > > > > > > 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 writt= en*/ > > > > volatile unsigned read; /**< Next position to be read = */ > > > > +#else > > > > + unsigned write; /**< Next position to be writt= en*/ > > > > + 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 = pointers */ > > > > 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 > > > > > > Correct. > > > > My apologies, did not follow your comment here. Do you want us to corre= ct > anything here? '#endif' is not appearing on the correct line in the email= , but it > shows up fine on the patch work. >=20 > No. What I meant is, code is correct. >=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 > > > > > > 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 > > > > > > Correct. > > > > > > > + > > > > 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 > > > > > > Correct. > > > > > > > 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); > > > > > > 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? > > > > > 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 als= o not > sufficient, please see 3) below. >=20 > But Nothing technically wrong in using rte_smp_rmb() here in terms > functionally and code generated by the compiler. rte_smp_rmb() generates 'DMB ISHLD'. This works fine, but it is not optimal= . 'LDAR' is a better option which is generated when C11 atomics are used. >=20 > > 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/stores around of this API to get reordered, especially since this i= s an > inline function. This would put burden on the application to manage the > ordering depending on its usage. It will also require the application to > understand the implementation of this API. >=20 > __ATOMIC_RELAXED may be fine too for _count() case as it may not very > important to get the exact count for the exact very moment, Application c= an > retry. >=20 > I am in favor of performance effective implementation. The requirement on the correctness of the count depends on the usage of thi= s function. I see the following usage: In the file kni_net.c, function: kni_net_tx: if (kni_fifo_free_count(kni->tx_q) =3D=3D 0 || = =20 kni_fifo_count(kni->alloc_q) =3D=3D 0) { = =20 /** = =20 * If no free entry in tx_q or no entry in alloc_q, = =20 * drops skb and goes out. = =20 */ = =20 goto drop; = =20 } There is no retry here, the packet is dropped. >=20 > > > > > > > > Other than that, I prefer to avoid ifdef clutter by introducing two > > > separate file just like ring C11 implementation. > > > > > > I don't have strong opinion on this this part, I let KNI MAINTAINER > > > to decide 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 be= tter > 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 behin= d the > #defines. >=20 > No Strong opinion on this, leaving to KNI Maintainer. Will wait on this before re-spinning the patch >=20 > This patch needs to split by two, > a) Fixes for non C11 implementation(i.e new addition to rte_smp_wmb()) > b) add support for C11 implementation. Agree >=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 > > > >