Message ID | VI1PR08MB31672DC75C7056EE577F14FA8F6E0@VI1PR08MB3167.eurprd08.prod.outlook.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 20DD02B96; Mon, 28 May 2018 10:15:15 +0200 (CEST) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0076.outbound.protection.outlook.com [104.47.1.76]) by dpdk.org (Postfix) with ESMTP id 027B52583 for <dev@dpdk.org>; Mon, 28 May 2018 10:15:13 +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=prL/09Yn7/LDO4TFceSGunwgygOUWO0N0kUQhYn/GCE=; b=HKvUuY0Q4g/GVmkqpW+ZNeeWmmcWAxHycxqIpyqrPzupAS9WH6lAEw0xWj3WTpSSdKdv73nXI3SKwJHoEeviJuf7bR1MpWMAI4BW9YYI3WieL9DqDbnsqe5aLh8IkC+wdFy1j3xJ0TNuvaj8P9d/JNWuIGyjgUEioR+U1iCpbiU= Received: from VI1PR08MB3167.eurprd08.prod.outlook.com (52.133.15.142) by VI1PR08MB1216.eurprd08.prod.outlook.com (10.166.198.135) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.797.11; Mon, 28 May 2018 08:15:11 +0000 Received: from VI1PR08MB3167.eurprd08.prod.outlook.com ([fe80::a105:3320:f324:a919]) by VI1PR08MB3167.eurprd08.prod.outlook.com ([fe80::a105:3320:f324:a919%13]) with mapi id 15.20.0797.015; Mon, 28 May 2018 08:15:11 +0000 From: Gavin Hu <Gavin.Hu@arm.com> To: Andy Green <andy@warmcat.com>, "dev@dpdk.org" <dev@dpdk.org> Thread-Topic: [dpdk-dev] [PATCH 1/2] ring: fix declaration after code Thread-Index: AQHT9iu0ckV4BE1KoEeCg9JXEF0Jb6RExGew Date: Mon, 28 May 2018 08:15:11 +0000 Message-ID: <VI1PR08MB31672DC75C7056EE577F14FA8F6E0@VI1PR08MB3167.eurprd08.prod.outlook.com> References: <152747443129.35192.15673273827095899997.stgit@localhost.localdomain> <152747454575.35192.11685010940587705005.stgit@localhost.localdomain> In-Reply-To: <152747454575.35192.11685010940587705005.stgit@localhost.localdomain> 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=Gavin.Hu@arm.com; x-originating-ip: [113.29.88.7] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; VI1PR08MB1216; 7:K6iEswRGaBFfJA85CBzbVcgYuV0d0ZNFH1YP9J5N00ZT1Ms6V2+TbadwQl4Y0B7EGKaxaICIxBl5K2hp/fVjVp3XNiWDqLfn20dlM81LotmagjMlAD/cG3HEyORUdjxEoG1OSygIoE3WefUQ9TaS47/2qFv8YDaNokUnlKcOEqs4dVr6rY4EFyJ/4ImSmbqOkES3K3B9XRveosnEK7KgEjYfVWYoHxjQ/jG+ETB5MTdGOYcm4gOhsNb4zUaQOmSq x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(48565401081)(4534165)(7168020)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:VI1PR08MB1216; x-ms-traffictypediagnostic: VI1PR08MB1216: x-microsoft-antispam-prvs: <VI1PR08MB121632F969AEC6B42C2421098F6E0@VI1PR08MB1216.eurprd08.prod.outlook.com> x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3002001)(93006095)(93001095)(3231254)(944501410)(52105095)(10201501046)(6055026)(149027)(150027)(6041310)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(20161123558120)(20161123564045)(6072148)(201708071742011)(7699016); SRVR:VI1PR08MB1216; BCL:0; PCL:0; RULEID:; SRVR:VI1PR08MB1216; x-forefront-prvs: 06860EDC7B x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39380400002)(366004)(376002)(346002)(39860400002)(396003)(189003)(13464003)(199004)(40434004)(2900100001)(53936002)(5660300001)(2501003)(6246003)(5250100002)(66066001)(5890100001)(86362001)(6116002)(9686003)(305945005)(229853002)(81156014)(74316002)(6436002)(3846002)(72206003)(478600001)(14454004)(55016002)(8936002)(55236004)(476003)(68736007)(97736004)(3660700001)(486006)(81166006)(446003)(6506007)(102836004)(8676002)(33656002)(26005)(2906002)(7736002)(316002)(11346002)(7696005)(53546011)(76176011)(110136005)(186003)(59450400001)(106356001)(25786009)(99286004)(105586002)(3280700002); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR08MB1216; H:VI1PR08MB3167.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: LuRwhzq2ufZfSduTncsf6Ranceth/A1cg+bA3KbouHRcspzAQ/MiD7SCCU6DwSFqXHerrmU0Pv/L1Fh+bjZu7h+FSwY2JkPreX412Tj8umEXasmqvBXWqW5X+rhwOqLPtWjUC0J/L6IFGUgD9siCCdBpNtIUKJ6KJ91Y9P4UVIk7wPkGl5eJl2BGEPMgNT34 spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: a19b54fb-9ea8-42c2-4284-08d5c4732243 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: a19b54fb-9ea8-42c2-4284-08d5c4732243 X-MS-Exchange-CrossTenant-originalarrivaltime: 28 May 2018 08:15:11.6972 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR08MB1216 Subject: Re: [dpdk-dev] [PATCH 1/2] ring: fix declaration after code X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://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: <https://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Checks
Context | Check | Description |
---|---|---|
ci/checkpatch | warning | coding style issues |
ci/Intel-compilation | fail | apply issues |
Commit Message
Gavin Hu
May 28, 2018, 8:15 a.m. UTC
Hi Andy, See my inline comments. -Gavin -----Original Message----- From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green Sent: Monday, May 28, 2018 10:29 AM To: dev@dpdk.org Subject: [dpdk-dev] [PATCH 1/2] ring: fix declaration after code On gcc 5.4.0 / native aarch64 from Ubuntu 16.04: /home/agreen/lagopus/src/dpdk/build/include/ rte_ring_c11_mem.h: In function '__rte_ring_move_prod_head': /home/agreen/lagopus/src/dpdk/build/include/ rte_ring_c11_mem.h:69:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] const uint32_t cons_tail = r->cons.tail; ^ /home/agreen/lagopus/src/dpdk/build/include/ rte_ring_c11_mem.h: In function '__rte_ring_move_cons_head': /home/agreen/lagopus/src/dpdk/build/include/ rte_ring_c11_mem.h:136:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] const uint32_t prod_tail = r->prod.tail; ^ Signed-off-by: Andy Green <andy@warmcat.com> Fixes: 39368ebfc6 ("ring: introduce C11 memory model barrier option") --- lib/librte_ring/rte_ring_c11_mem.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) * (the result is always modulo 32 bits even if we have @@ -129,11 +131,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, /* move cons.head atomically */ do { +const uint32_t prod_tail = r->prod.tail; + /* Restore n as it may change every loop */ n = max; *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); -const uint32_t prod_tail = r->prod.tail; + /* The subtraction is done between two unsigned 32bits value * (the result is always modulo 32 bits even if we have * cons_head > prod_tail). So 'entries' is always between 0 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Comments
On 05/28/2018 04:15 PM, Gavin Hu wrote: > do { > +const uint32_t cons_tail = r->cons.tail; > + > /* Reset n to the initial burst count */ > n = max; > > *old_head = __atomic_load_n(&r->prod.head, > __ATOMIC_ACQUIRE); > -const uint32_t cons_tail = r->cons.tail; > + > [Gavin Hu] The ACQUIRE and RELEASE pair protects anything that between the two must be visible to other threads when they perform an acquire operation on the same memory address. Your changes broke this semantics. I advise to move the declaration before and keep the assignment in the old place. I see, thanks for the tip. How about just get rid of this temp altogether if access to it is locked during this sequence anyway? It's not like we had to sample it after the lock then, or it's bringing anything else to the party. So instead of cons_tail / prod_tail at all, replace directly with r->cons.tail / r->prod.tail at the single usage for each. -Andy
-----Original Message----- From: Andy Green <andy@warmcat.com> Sent: Monday, May 28, 2018 4:47 PM To: Gavin Hu <Gavin.Hu@arm.com>; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 1/2] ring: fix declaration after code On 05/28/2018 04:15 PM, Gavin Hu wrote: > do { > +const uint32_t cons_tail = r->cons.tail; > + > /* Reset n to the initial burst count */ > n = max; > > *old_head = __atomic_load_n(&r->prod.head, > __ATOMIC_ACQUIRE); > -const uint32_t cons_tail = r->cons.tail; > + > [Gavin Hu] The ACQUIRE and RELEASE pair protects anything that between the two must be visible to other threads when they perform an acquire operation on the same memory address. Your changes broke this semantics. I advise to move the declaration before and keep the assignment in the old place. I see, thanks for the tip. How about just get rid of this temp altogether if access to it is locked during this sequence anyway? It's not like we had to sample it after the lock then, or it's bringing anything else to the party. So instead of cons_tail / prod_tail at all, replace directly with r->cons.tail / r->prod.tail at the single usage for each. [Gavin Hu] I think it is ok. -Andy IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff --git a/lib/librte_ring/rte_ring_c11_mem.h b/lib/librte_ring/rte_ring_c11_mem.h index cb3f82b1a..3cc339558 100644 --- a/lib/librte_ring/rte_ring_c11_mem.h +++ b/lib/librte_ring/rte_ring_c11_mem.h @@ -61,12 +61,14 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp, int success; do { +const uint32_t cons_tail = r->cons.tail; + /* Reset n to the initial burst count */ n = max; *old_head = __atomic_load_n(&r->prod.head, __ATOMIC_ACQUIRE); -const uint32_t cons_tail = r->cons.tail; + [Gavin Hu] The ACQUIRE and RELEASE pair protects anything that between the two must be visible to other threads when they perform an acquire operation on the same memory address. Your changes broke this semantics. I advise to move the declaration before and keep the assignment in the old place. /* * The subtraction is done between two unsigned 32bits value