From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422739AbcFCU6h (ORCPT ); Fri, 3 Jun 2016 16:58:37 -0400 Received: from mail-bn1bon0142.outbound.protection.outlook.com ([157.56.111.142]:61664 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932807AbcFCU62 (ORCPT ); Fri, 3 Jun 2016 16:58:28 -0400 Authentication-Results: linux.vnet.ibm.com; dkim=none (message not signed) header.d=none;linux.vnet.ibm.com; dmarc=none action=none header.from=hpe.com; Message-ID: <5751EF53.3080205@hpe.com> Date: Fri, 3 Jun 2016 16:57:55 -0400 From: Waiman Long User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12 MIME-Version: 1.0 To: xinhui CC: Peter Zijlstra , Arnd Bergmann , , , Subject: Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian References: <1464862148-5672-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> <4399273.0kije2Qdx5@wuerfel> <20160602110200.GZ3190@twins.programming.kicks-ass.net> <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> In-Reply-To: <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [72.71.243.26] X-ClientProxiedBy: CO2PR11CA0031.namprd11.prod.outlook.com (10.141.242.169) To TU4PR84MB0317.NAMPRD84.PROD.OUTLOOK.COM (10.162.186.27) X-MS-Office365-Filtering-Correlation-Id: 78dc55e4-2378-4169-b421-08d38bf1c61b X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;2:K3kAbhae2xWxxuMdc0n3rfnh5KXxijzw0vTZ2PgI3p+DIxP7x3EEvHcmbLxVJRj0sZ1TTsb8c70Eh0mCk5E/k/lC6rF2LkLC7h/mtuk7TMeZjrvpN0WhULySvORjtTm+q/vLKJFcFMzsJ/z3MJB6HH7wWY+ahqTngYLZZJbVCvP9R70IMZOOro3tkZ4Ed9mT;3:wQmy2KL/ppCLanHqXLSwxQvezlEjV2nHZDmIqO2h/XimKjIU8vyM+TkninNOTge5c/0mGR6UlL5nwJbTIRtb5fPmpwfNluXEAXXvYUnjnPItY/sOT1X49PWEubaxX2nW X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0317; X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;25:uJ2G4Vi9g6fdBfYrPuZLUvPqNvQWCSqlzj9WhAfdPq88z3ppED3bPte2MIdrQEkFYA/AmagOAguj+sSznhGwKDhvG4zzD0vbqyxuOooq6o9jJuY7LyFCHd/gtmoNbH0FKqEkKR2VOaxuoDNNYpK6EswQGZ9mLzTPAu2DHM0J91qiRQhCWyiMHaPVrgMj6oFfUZyhzZ0A57aQ0YCIQbhTyZsVolnMEJ6Yn01E7Fl/Bk3ZwPaFZFuSV289VdRvG14Wn8tGZ9n8ONKD6UeG9REacdMVXdtBOO6wyk0A1XZJeI0mauLdRMZjy2lw0ec58YHY9ZuXrwoY6iH5X/rzcZSQjpaQexOqz7nuXmbiiA11QyQQKrz83s4xnFwZDshZ9kl3VaO6bMfQoCRThMn4ncdmQoZc/CWJN8NWPPXLheN7MmbsHWhoC4eU2rTTbEq50DO/e9bahtBf/B0ZkQju0HBWMaFoLz3ARt9qiD87Gd91c8oaz0eoT9P3BiIWPdJt3es+oEXCMoAhziUyZ02JUmUbJDSrZIsiVHZ5zu4JdWKogLkAz18lZYMXOX62mbDrrkBzyLQcg4qfuk9755IrAuyAZXURYnjsXk/U8P5qW1SM4+I784GJUrMmtZv//1wYcEGvdo1x5P2fuZpAsh8oju8wji5sT/0tiSPDIOCJZpT/tT/jSdxU0roqvIf1CrIddc0b X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;20:ZIZhubtLllQs48iyXl6yPQO8jZsgLBV0ASy+RZy0IfyhdUxy4WZUiGus78Y+TQNBmWF968YxZg+nRPEp8uYBU8AGBHwtO5nzcdHjQvflFjaWViAilSbhgJH+2GcYku6wc0OxcopgTS+AWad+HRrHyjdyTEO8rDM9eEsTWk2lYT7cUCyVQkwRpGm+6ZvEvMgfqmB98pB/p15sbbFRUobWz0uJvmKACKXwcKn3QZOYtlpob3FFHz5OnaIYPVeUHiDIcwNtLYtwLP+Y7X2Nb9R3dysNE4MtksJnYxwGR54PQ2QiwjOm/yOgWFBwGSxRvEsMV1urYCImWCURATAbA3qXOfbrYpw/CS4ma+E1FUWiH8QhBoJ8Js4Gp94NS7C8BA9hAf3Am2s56A08ObPyX314twsBDk1USbEaBYvh/72eL71buNn0ax87h83P9AqXhW3fR2xdmbRi7YeqlgBFVEKu7Ub5lJt75bVw79VBOlAsaIMWP+os19Dvu5hP0Btm4Otv;4:iJHmzOnLM96h+iMzXLyM80776ml68eWOBmiyZUZnHhUY9ZUt/79Tu2v37fkJwf8qkbsErozzzr6q4NB5mYuJKCJMtMcgb5BbGWnC+fqAsdq0TyIBk6zv6Q4fczbLK172mLrjbZIAbQe02XlOZ+qZ7Azdnesl+I9so9WrnZ+hSYulXrFgRLryNE5PinLw4ReI+T3bx/BwQWP/hYbpZoLFapfYZlx/zGevPFUZby0bOPJuhgFziKObg1KSwqC7Eo5MkBkBgY3NdSIGMrXKpFldDfDEu+hUR5Q9IrkxU0PvrlMDQvp29pCzFGnH4xTvRZebvUUbAxx5lc31uQmdfGCjgL6BYL89vyUaZ3Wn8d/LCjgeGYrU2FO6M6UZCI0cTex5 X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046);SRVR:TU4PR84MB0317;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0317; X-Forefront-PRVS: 0962D394D2 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(51444003)(24454002)(377454003)(50986999)(5008740100001)(47776003)(2906002)(65816999)(2950100001)(117156001)(5004730100002)(76176999)(59896002)(77096005)(87266999)(65806001)(92566002)(65956001)(4326007)(54356999)(36756003)(80316001)(50466002)(66066001)(8676002)(586003)(83506001)(3846002)(2870700001)(6116002)(42186005)(4001350100001)(86362001)(110136002)(23676002)(93886004)(81166006)(33656002)(189998001)(8666004)(7059030);DIR:OUT;SFP:1102;SCL:1;SRVR:TU4PR84MB0317;H:[192.168.142.145];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtUVTRQUjg0TUIwMzE3OzIzOnhKVndac3dxV0ViYkJoSFUvcHRJcjh0SUVP?= =?utf-8?B?ZjNqNzVuR1dUVGZ0dFB6ak9PdkMxRFhhN3Q4b0dnTG1HUksxZWN1NVhOeXpI?= =?utf-8?B?N09OQVphbTluUFIyN3lXNGRjNnF5b1h3MTAyY2lmMzlFMTBoVXhyRmxORFl2?= =?utf-8?B?eWxsUjVneFF0bnc2V2Z2MHlMN2tla3VzcHdRTlpwYTFvWmM0Y3BQc0VKZmF0?= =?utf-8?B?bU9ST3ZmL29sSVVyQno1QUY4SENyWXJHWnh4dHhNYm1FYXJ0TEZDMTZRRFNV?= =?utf-8?B?bWxuV2hDc1NEdjRiR3ZRYVRlVnNKODJVc1ZLdk1OL2liQ2VtODdoVjZCcC9R?= =?utf-8?B?WTBIM05TdmdRNjZYSzF6YlR4QytFaUl4b05xRGUvUGxnUEppZjBLL1ZoMDJW?= =?utf-8?B?c3FEZDhtR2lDSHJWUjhDWlFJaTlDQVBmM0tIUkx6SzNOWmUyTE5vWHorNnBF?= =?utf-8?B?bzl1QVdKMlR6Z2Y5RGNyNkg2MGVWbnZKbHA4b1lKMVVNeWFCMER4MlZmcFZ6?= =?utf-8?B?ZDlVQXpPL3RjeU05MUNIZTJaWmJ6S2c3a0VQU0RhbENuSjExM1ZVRENkUVJa?= =?utf-8?B?N2JkdzE1WlFnVHZjeWs3NVVtUy92bDRwVFhJU3BMYVRVV1Q1SlBNTW8veTZI?= =?utf-8?B?a3NpbmR5WS84UFBZYm5YVDJ2SldpK1g2NGlNVk9qdkg5d0ZBVFZTcjVwelMw?= =?utf-8?B?eWNzY0NnZEJncC83T1psN0JoYXk4T2lIQjV1RDdSRWhJTmR4QW5RNE92R1Bu?= =?utf-8?B?c1dUb1VDNG1BUHJIYi9VRnF5dzRvRzdFUG5aZkR5N3p1Vi9FOXlyOUZoVmh6?= =?utf-8?B?UFFwV3Y0T2RwdGZPVUlCRVZXTVdMTkdScG43N1JFVWJ5UlI1dnJDZlB4czJ6?= =?utf-8?B?K2xJOS84VGt5ZTFMT1FQRmhWeWVYbDVjeEN0bEpxRU40a2ZHaXFDRk9DWXRK?= =?utf-8?B?ZGNJRUxuVlJXYzR4ajhEYW0wMGIvZnBRMHd2Nkt0N3Y4UnFiakJBdi9GelZN?= =?utf-8?B?QU5FY29ZREVtWFl1WlRmTlFYTVhVa3BYMEYzMFYyYjdkN05XTktHZ2RlSzNC?= =?utf-8?B?K2tqSllIRHdwMWNUZmtRZWZLNWJxL2xWUCtLZUJieEdPaVJFWXFxMmExVE85?= =?utf-8?B?V21OMEVpV3h6ZGhkR1gwMThsWEgyZjlEWk1tQlVOeVgwR0c1MXEyU1FiSUxq?= =?utf-8?B?SkE0NXk4RitMMHhyY1ZwSWM3aGw4dmRFZjJmWGdHY3J6eWw0NTRqREZOalZ0?= =?utf-8?B?MlZBSFNDanVtSmd2dGJLYUMxaWJYZVpqQ2Q5Nm5oQWtHblByY0JRQXZuQVBq?= =?utf-8?B?a2pYKzFSZ0NtM1BrWmx6eitaMzVKZDFhUUxrWDhwUTQ2TVc2RVRqZ0UxMUdN?= =?utf-8?B?bWRDN2U1bUwwU0d4RE03OEJVZWFHL1pZemFoNUxYWXAzajdtSkw1bTRxZ3NG?= =?utf-8?B?bDluaWFvdWdTb3RuTHNrRVQ0czltUUJUWU9JQjg5bDkxYjdOaEFGRlhXMmdt?= =?utf-8?Q?Forl9IvCfcMrqn4daLQwaKP8QNkce0w+lhiUi+08qjo5bY?= X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;5:UCvOEQJqfdCwq/mieINLXjkbeguxCXty0o/VotWD85fnFrwtu/GDKmiGwzM7fWK2MSyt0YD82fx2pm/MdRfhr+fWrXrNc0AkMzeU67jvzbAqYdbJ10btydwRluHvMC2rbk3HIzs1YlqJ5YSPz0QDNw==;24:GJSsPs3aE/b0vrbgXT5IdX+2+iyJB7IN6MGEMYHh1KjeZFZiktb/B1KLKI90W5rMuFVOCGB6R0Pw7sNcV7KQYLCm0EiVb6J/wco+PJTKbWo=;7:5GkNY4vrhF4PuQlMK2oMP834DHtLb0Bvbu+e8IbyqsFbwOivEO96sH+3qPSzQ3C0LDLaetYYTMmM0TORUHEOlSV4Zdka0kufFJPS8WE9umDmuZDPXr1fYDQ7Vfs8J0HWvB3RqWvElizrdDqhf8SgFiCh9L7pdDM3EI2dGykknOfsc18vq8MtsjdEc4Sgm0njx8RFGeCMMgORBZqMWGWeCWWuueasKfA9xsYTgKpV/Us= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2016 20:58:07.2078 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: TU4PR84MB0317 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/03/2016 03:17 AM, xinhui wrote: > > On 2016年06月02日 19:02, Peter Zijlstra wrote: >> On Thu, Jun 02, 2016 at 12:44:51PM +0200, Arnd Bergmann wrote: >>> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote: >>>> diff --git a/include/asm-generic/qrwlock.h >>>> b/include/asm-generic/qrwlock.h >>>> index 54a8e65..eadd7a3 100644 >>>> --- a/include/asm-generic/qrwlock.h >>>> +++ b/include/asm-generic/qrwlock.h >>>> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct >>>> qrwlock *lock) >>>> */ >>>> static inline void queued_write_unlock(struct qrwlock *lock) >>>> { >>>> - smp_store_release((u8 *)&lock->cnts, 0); >>>> + (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts); >>>> } >>> >>> Isn't this more expensive than the existing version? >> >> Yes, loads. And while this might be a suitable fix for asm-generic, it >> will introduce a fairly large regression on x86 (which is currently the >> only user of this). >> > well, to show respect to struct __qrwlock private field. > We can keep smp_store_release((u8 *)&lock->cnts, 0) in little_endian > machine. > as this should be quick and no performance issue to all other > archs(although there is only 1 now) > > BUT, We need use (void)atomic_sub_return_release(_QW_LOCKED, > &lock->cnts) in big_endian machine. > because it's bad to export struct __qrwlock and set its private field > to NULL. > > How about code like below. > > static inline void queued_write_unlock(struct qrwlock *lock) > { > #ifdef __BIG_ENDIAN > (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts); > #else > smp_store_release((u8 *)&lock->cnts, 0); > #endif > } > > BUT I think that would make thing a little complex to understand. :( > So at last, in my opinion, I suggest my patch :) > any thoughts? Another alternative is to make queued_write_unlock() overrideable from asm/qrwlock.h, just like what we did with queued_spin_unlock(). Cheers, Longman From mboxrd@z Thu Jan 1 00:00:00 1970 From: Waiman Long Subject: Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian Date: Fri, 3 Jun 2016 16:57:55 -0400 Message-ID: <5751EF53.3080205@hpe.com> References: <1464862148-5672-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> <4399273.0kije2Qdx5@wuerfel> <20160602110200.GZ3190@twins.programming.kicks-ass.net> <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> Sender: linux-kernel-owner@vger.kernel.org To: xinhui Cc: Peter Zijlstra , Arnd Bergmann , linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, waiman.long@hp.com List-Id: linux-arch.vger.kernel.org On 06/03/2016 03:17 AM, xinhui wrote: > > On 2016=E5=B9=B406=E6=9C=8802=E6=97=A5 19:02, Peter Zijlstra wrote: >> On Thu, Jun 02, 2016 at 12:44:51PM +0200, Arnd Bergmann wrote: >>> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote: >>>> diff --git a/include/asm-generic/qrwlock.h=20 >>>> b/include/asm-generic/qrwlock.h >>>> index 54a8e65..eadd7a3 100644 >>>> --- a/include/asm-generic/qrwlock.h >>>> +++ b/include/asm-generic/qrwlock.h >>>> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct=20 >>>> qrwlock *lock) >>>> */ >>>> static inline void queued_write_unlock(struct qrwlock *lock) >>>> { >>>> - smp_store_release((u8 *)&lock->cnts, 0); >>>> + (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts); >>>> } >>> >>> Isn't this more expensive than the existing version? >> >> Yes, loads. And while this might be a suitable fix for asm-generic, = it >> will introduce a fairly large regression on x86 (which is currently = the >> only user of this). >> > well, to show respect to struct __qrwlock private field. > We can keep smp_store_release((u8 *)&lock->cnts, 0) in little_endian=20 > machine. > as this should be quick and no performance issue to all other=20 > archs(although there is only 1 now) > > BUT, We need use (void)atomic_sub_return_release(_QW_LOCKED,=20 > &lock->cnts) in big_endian machine. > because it's bad to export struct __qrwlock and set its private field= =20 > to NULL. > > How about code like below. > > static inline void queued_write_unlock(struct qrwlock *lock) > { > #ifdef __BIG_ENDIAN > (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts); > #else > smp_store_release((u8 *)&lock->cnts, 0); > #endif > } > > BUT I think that would make thing a little complex to understand. :( > So at last, in my opinion, I suggest my patch :) > any thoughts? Another alternative is to make queued_write_unlock() overrideable from=20 asm/qrwlock.h, just like what we did with queued_spin_unlock(). Cheers, Longman