From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00041.outbound.protection.outlook.com [40.107.0.41]) by dpdk.org (Postfix) with ESMTP id B833E1B19 for ; Fri, 21 Sep 2018 08:37:29 +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=Ugzupv6gWL6mKDqhYc4khW1zViY8pvZuv2kiLtg2SJU=; b=NEs4xwcRiUJ0ncglVl2CeeP8lImr/EBzf3FW+P88+J5qGFm0KXb4Ywa6yjd0ncYf3U7Prp8wpHzG0Q8JqU7h8qOjb8bO4yJFJjDJgTsJabniyx/jN/DhtyJ8X03KzFq8yunbXRlHB6B3CBK+ejC1OGvvh+z52IzmYS7lqxUJ6J4= Received: from AM6PR08MB3672.eurprd08.prod.outlook.com (20.177.115.29) by AM6PR08MB3014.eurprd08.prod.outlook.com (52.135.163.31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1143.15; Fri, 21 Sep 2018 06:37:28 +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 06:37:28 +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: AQHUULwDi5PFjJpIFUyDUdvyMo0oh6T5RIGggAAJlACAAOYDgIAACdiAgAACrDA= Date: Fri, 21 Sep 2018 06:37:27 +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> <20180921055529.GA15861@jerin> In-Reply-To: <20180921055529.GA15861@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; AM6PR08MB3014; 6:IyPFw4bn1SA3MTvzBvWcXW5YCsLutoJ9U0F59s9IJjwPOHMdKzRYUo0XyBE7vMYvXkdk37GSBnP/7xXVQgr7vw0wWMNOfLkr2Y5jxXt6nXoFWh3/WToGJIZHRZZPIXYlbP8RFLCQDuGGulhWyu2o3cBOcqQl43Besu7Daw7B8Q6l7R+F/GrxpRn7KzynvsIDdPIRqLyT7k3r6aHmn6T8+37qr53mgy+MxldJABwWOGhUeQy0OWPiUGUM1NSgKyeM0wmJiKhMbVOSzcIfyjP7hnwEL/BqFM6Id1ciEhE34Lp431a6dG0k06PLxA02B0RnuRxsdJ1nnHVgoknHiJJatJ58mKvlHZE6u6HDvuEuRelit90hloebE+uaTdWoyuLbi7CKogWs7vU/Rh5Nkav7INwKabhbdl5E7V4hkS2gYauDYsBkZGEaxn0BZy6DP7wxbaDXlYfSZg2Wu5Nez9ss9g==; 5:R/dNOI97vLUHNs5LYI+WzRtwiIWRzKAr0Dw0MwTs6KdACLGdQfMpJJ0p4I9nIaB1O7Lwt27MG46AVf8TAWNJoCgC+5+GqHQbznokKQl35x7Lz+tPfdbA7lPdCv7Osbh8qcjJ2/foup50liiNN3zHCebWtkanaAlXvNgRF8u3kBM=; 7:ADramqWSeU3pELUbOYTAol6DOTxsgEmFnKZsZ+TOC9b0m7NXUbCtJtdX/b+hVfhTY235hObOGpdZOT6kR2aaA5byBTxF6r9svOqDEcXJJx5LlQYP75/juVzj+yDNZgX036+xPGkXLzSyLl+TPtbXWd/VSJnX6XkaKsBnazr8khQZXQTizEB5bOU7tY6UTvDLLsv4vzpBNe2rQrqZXp0cjArE0ny7N8ro+7xEcZcbj1eGG2+yBmkdoLE5iUK7yFcg x-ms-exchange-antispam-srfa-diagnostics: SOS;SOR; x-ms-office365-filtering-correlation-id: 42d75d4e-bc91-4273-a6f8-08d61f8cb31b 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:AM6PR08MB3014; x-ms-traffictypediagnostic: AM6PR08MB3014: nodisclaimer: True x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(163750095850); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3231355)(944501410)(52105095)(93006095)(93001095)(3002001)(10201501046)(6055026)(149027)(150027)(6041310)(20161123564045)(20161123558120)(20161123560045)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051); SRVR:AM6PR08MB3014; BCL:0; PCL:0; RULEID:; SRVR:AM6PR08MB3014; x-forefront-prvs: 0802ADD973 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(396003)(136003)(346002)(39860400002)(366004)(376002)(199004)(57704003)(189003)(8676002)(316002)(105586002)(54906003)(26005)(6436002)(229853002)(14454004)(74316002)(9686003)(55016002)(6916009)(68736007)(186003)(2900100001)(6116002)(53936002)(3846002)(6246003)(7736002)(6506007)(305945005)(97736004)(86362001)(99286004)(72206003)(446003)(76176011)(2906002)(5250100002)(478600001)(11346002)(33656002)(25786009)(81166006)(66066001)(81156014)(5660300001)(102836004)(486006)(256004)(14444005)(8936002)(93886005)(4326008)(106356001)(476003)(7696005)(71190400001)(71200400001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR08MB3014; 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: esJPug8ZFDR9dMfr59tN3NCPnRWMl4M3h9XZjHuFhkdMPQ1dL4JUP5tvhW6Jv/koRby1Axh3oP6epfRI+VCACk204AxY61SRBx4DPpT98yXoFwpLIA3lT8OjWR/2vMLGR9Wx2sfkV/IJ8g5Uaw/y0aE7MDMQQWvuPgfliVI+5aMH0oGUE46JXImRlMU8/ad2epeilivmEzDU4j0248sGwzUHpPcMC7aSE89H2th3iaT7uK2OGC27t/lI+xqBOhSFBOACCUaFp3uWVOUMf2xfg/rOrOzL3bRfLsrBl91aHpR0IHDCoFhRuBH5Rof4y7aj+EY/7kOQeThPBi84KIa7ToO4Vto+M25/rs4zCN5CdP0= 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: 42d75d4e-bc91-4273-a6f8-08d61f8cb31b X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Sep 2018 06:37:27.9049 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR08MB3014 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 06:37:29 -0000 > > > > > > > > > > > > @@ -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_ACQU= IRE); > > > > > > + 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 also not sufficient, please see 3) below. > > > > > > 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 opt= imal. > 'LDAR' is a better option which is generated when C11 atomics are used. >=20 > Yes. But which one is optimal 1 x DMB ISHLD vs 2 x LDAR ? Good point. I am not sure which one is optimal, it needs to be measured. 'D= MB ISHLD' orders 'all' earlier loads against 'all' later loads and stores. = 'LDAR' orders the 'specific' load with 'all' later loads and stores. >=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 is 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. > > > > > > __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 can retry. > > > > > > I am in favor of performance effective implementation. > > > > The requirement on the correctness of the count depends on the usage of > this 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 || > > kni_fifo_count(kni->alloc_q) =3D=3D 0) { > > /** > > * If no free entry in tx_q or no entry in alloc_q, > > * drops skb and goes out. > > */ > > goto drop; > > } > > > > There is no retry here, the packet is dropped. >=20 > OK. Then pick an implementation which is an optimal this case. > I think, then rte_smp_rmb() makes sense here as > a) no #ifdef clutter > b) it is optimal compared to 2 x LDAR >=20 As I understand, one of the principals of using C11 model is to match the s= tore releases and load acquires. IMO, combining C11 memory model with barri= er based functions makes the code unreadable. I realized rte_smp_rmb() is required for x86 as well to prevent compiler re= ordering. We can add that in the non-C11 case. This way, we will have clean= code for both the options (similar to rte_ring). So, if 'RTE_USE_C11_MEM_MODEL' is set to 'n', then the 'rte_smp_rmb' would = be used. We can look at handling the #ifdef clutter based on Ferruh's feedback. >=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 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. > > > > > > No Strong opinion on this, leaving to KNI Maintainer. > > Will wait on this before re-spinning the patch > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > + return (fifo->len + fifo_write - fifo_read) & > > > > > > +(fifo->len - 1); #else > > > > > > return (fifo->len + fifo->write - fifo->read) & > > > > > > (fifo->len > > > > > > - 1); Requires rte_smp_rmb() for x86 to prevent compiler reordering. > > > > > > +#endif > > > > > > } > > > > > > -- > > > > > > 2.7.4 > > > > > >