From mboxrd@z Thu Jan 1 00:00:00 1970 From: Waiman Long Subject: Re: [PATCH v2] locking/qrwlock: Let qrwlock has same layout regardless of the endian Date: Fri, 17 Jun 2016 15:06:33 -0400 Message-ID: <57644A39.9080509@hpe.com> References: <1465983109-3217-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-bl2on0101.outbound.protection.outlook.com ([65.55.169.101]:59795 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753126AbcFQTWI (ORCPT ); Fri, 17 Jun 2016 15:22:08 -0400 In-Reply-To: <1465983109-3217-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Pan Xinhui Cc: linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, ingo@redhat.com, peterz@infradead.org, arnd@arndb.de, will.deacon@arm.com On 06/15/2016 05:31 AM, Pan Xinhui wrote: > This patch aims to get rid of endianness in queued_write_unlock(). We > want to set __qrwlock->wmode to NULL, however the address is not > &lock->cnts in big endian machine. That causes queued_write_unlock() > write NULL to the wrong field of __qrwlock. > > Actually qrwlock can have same layout, IOW we can remove the #if > __little_endian in struct __qrwlock. With such modification, we only > need define some _QW* and _QR* with corresponding values in different > endian systems. > > Suggested-by: Will Deacon > Signed-off-by: Pan Xinhui > --- > change from v1: > A typo fix which is really bad... > thanks Will for the carefull review. :) > --- > include/asm-generic/qrwlock.h | 15 +++++++++++---- > kernel/locking/qrwlock.c | 10 ++++------ > 2 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h > index 54a8e65..28fb94a 100644 > --- a/include/asm-generic/qrwlock.h > +++ b/include/asm-generic/qrwlock.h > @@ -27,11 +27,18 @@ > /* > * Writer states& reader shift and bias > */ > -#define _QW_WAITING 1 /* A writer is waiting */ > -#define _QW_LOCKED 0xff /* A writer holds the lock */ > -#define _QW_WMASK 0xff /* Writer mask */ > +#ifdef __LITTLE_ENDIAN > #define _QR_SHIFT 8 /* Reader count shift */ > -#define _QR_BIAS (1U<< _QR_SHIFT) > +#define _QW_SHIFT 0 /* Writer mode shift */ > +#else > +#define _QR_SHIFT 0 /* Reader count shift */ > +#define _QW_SHIFT 24 /* Writer mode shift */ > +#endif > + > +#define _QW_WAITING (1U<< _QW_SHIFT) /* A writer is waiting */ > +#define _QW_LOCKED (0xffU<< _QW_SHIFT) /* A writer holds the lock */ > +#define _QW_WMASK (0xffU<< _QW_SHIFT) /* Writer mask */ > +#define _QR_BIAS (1U<< _QR_SHIFT) > > /* > * External function declarations > diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c > index fec0823..57d66cf 100644 > --- a/kernel/locking/qrwlock.c > +++ b/kernel/locking/qrwlock.c > @@ -30,18 +30,15 @@ struct __qrwlock { > union { > atomic_t cnts; > struct { > -#ifdef __LITTLE_ENDIAN > u8 wmode; /* Writer mode */ > u8 rcnts[3]; /* Reader counts */ > -#else > - u8 rcnts[3]; /* Reader counts */ > - u8 wmode; /* Writer mode */ > -#endif > }; > }; > arch_spinlock_t lock; > }; > > +#define _QW_MODEVAL(v) ((v)>> _QW_SHIFT) I know what you are doing here, but it is a bit hard to understand it just by looking at the name of the macro itself. Maybe some other names like _QW_MASKVAL() or_QW_BYTEVAL(). You may also want to have a line of comment about it. Other than that, I don't see any problem with it. Acked-by: Waiman Long > + > /** > * rspin_until_writer_unlock - inc reader count& spin until writer is gone > * @lock : Pointer to queue rwlock structure > @@ -127,7 +124,8 @@ void queued_write_lock_slowpath(struct qrwlock *lock) > struct __qrwlock *l = (struct __qrwlock *)lock; > > if (!READ_ONCE(l->wmode)&& > - (cmpxchg_relaxed(&l->wmode, 0, _QW_WAITING) == 0)) > + (cmpxchg_relaxed(&l->wmode, 0, > + _QW_MODEVAL(_QW_WAITING)) == 0)) > break; > > cpu_relax_lowlatency(); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755075AbcFQTWL (ORCPT ); Fri, 17 Jun 2016 15:22:11 -0400 Received: from mail-bl2on0101.outbound.protection.outlook.com ([65.55.169.101]:59795 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753126AbcFQTWI (ORCPT ); Fri, 17 Jun 2016 15:22:08 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=waiman.long@hpe.com; Message-ID: <57644A39.9080509@hpe.com> Date: Fri, 17 Jun 2016 15:06:33 -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: Pan Xinhui CC: , , , , , Subject: Re: [PATCH v2] locking/qrwlock: Let qrwlock has same layout regardless of the endian References: <1465983109-3217-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> In-Reply-To: <1465983109-3217-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [71.168.64.123] X-ClientProxiedBy: BY2PR06CA023.namprd06.prod.outlook.com (10.141.250.141) To AT5PR84MB0307.NAMPRD84.PROD.OUTLOOK.COM (10.162.138.29) X-MS-Office365-Filtering-Correlation-Id: 70c987da-b693-48d7-c466-08d396e28d67 X-Microsoft-Exchange-Diagnostics: 1;AT5PR84MB0307;2:DkmNU3qr5x8UzVHj3UMJuJd4o0jPcJJO7VjUWZeE8D5DlWrP7jYgGBIDYyk8F2ImJjIcgrYAOK3lK8hmtxRvrPqDpzyVLIp5QSN8N3SjfLvs7do+TnzQiWYKqEIJUdas4VJ7pzmyQzw8RPapcGGaSEw32a3/GhJbjB4xPhpcJp6lPwTqao9tLXm0i8rKH7Kc;3:X0cwRqJ+qLicSBlnJXhDtjJ9DVrtDYhI11mI8Mh/1aOhKpdLgfRUpFg+EC2tntZ+n7pfNmiqp0ftZyFD0/0yWdiShb7hK6j+8hwjHxCHavdtjGR6w/Qnc4ms7KnO7Clv;25:qIMkLCiuqzgYAYCnskpKBemnUl975llTmIzSiEIdUF3S9GPikdsBjm2SlSKTY8yK90U9JRxA7FbGQ1uqsz991VXhCy05Ian76rk6OzpY7xZBqYAYhhi5nPsPHp5399gQYBLeZJUTBFEL7UCqixnX4OdeKdBufvrMgm8a6GTC7JgIy7chQHZoXX8cx28jKmuPwVphsPloCClb6NNu/nfVhROAHUBvhiEUFJmfGCheoybY+Xor8y/S2ap9UUUn6QnvyyrLgfvSi3w/GSfcHLrKauU2M6SGv1NvPpoiQViuX7I2wx6aKWyZPpqA2yqYrgrGdeR5gBYEKy6yCtv5nU4jXuxowb5GDGWj5z30Mby1+QvhejoH2V/3aMT1WqLceTS+o6ohMJbA59TJOqL6fIPsNZ5Yle+mxJ0JCepFGX7MLcI= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AT5PR84MB0307; X-Microsoft-Exchange-Diagnostics: 1;AT5PR84MB0307;20:kNfK4gjUipcquSaG4d6jAgb+mu9Jrf4YP5XLUlxT3jW6yVwtCRY1VmE9E91P9vyt7zSCBQEbLN1Q3UC4e5gCStszpcSBV6PGvaqnbMZ9aBvzM1fTujokbYpCsR9J0NerjSv8d0jmPa08w/6WD/xCuONbOcChL5q/3mkXfRMp83x3vAQxSVWb7n0kG+PGu+OjF535Wz+E5BAiXjVT32/7HS3lPPB0lH0sH3bEb+XYLN/MmBzlvdbcXU/+A55Y+WBaepUBg24Gx/J7llxTUGOGn/EMin/IkYmF+vC5fK1bdH3wXd1ACOQBjLOqZpaO2zvxNsI3GfCy1Zc5xoSytNOXoew15KcUTJOJsUll/AaK2bw4lphwrja/ZBPIEL+NvDyMrUxBMoZk6egaXCG56mi6EmI95zEt/Ymhcy5kosJpqkzBGYRbAv63ssss+gf1tWDa6VKHd0UEpZDcxw9gHD+7Zvs+FtjXapedR3GWPoS+q7ogAO7tLIQfXgpuq/6t0wzV X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(227479698468861)(180628864354917)(104084551191319); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046);SRVR:AT5PR84MB0307;BCL:0;PCL:0;RULEID:;SRVR:AT5PR84MB0307; X-Microsoft-Exchange-Diagnostics: 1;AT5PR84MB0307;4:QILV+kwTfyaKS0ITE1emAmYGbC6F+9o+4kxb9eJT7/sC2Bq0j6xPOAImFAQbsOlwL5U79Lx/LNU2O0t++0aPinyWPiNkWgNDA3SUFzE1qkGpQqZZDJP8BhhRKLeATImaPh1cw0OQYuygAd+ckm9v2aoTPeCPHJaSlEu77xSyGsJOnLCkHsgNVZbREvyBISvstYAOEx8JI47HPYuoQEIsTVLiVt2hmOZGKnIarWC6eBQ/2mBc3Vvf/AXLobB+dVgxtCh/nwc/PEfiiDeWwQlfb/ecP6QzV5VmnKugV2/QHjrNP4H2TItpTJngkXk2iQ/7CcklyCMPmX+e9Q4G5oZeIvuF7HFPn6zstNNOEOd336jPsfg8YjUmidnQrZEoQTB5p/t6lvP5erqqgKVafU7lbZpdwP+uT7jGuiZt0Y9ZokrtZyJzTixPwUx9TxZ75CMxpAdceEAJvHu2pjTxWI7dd5fjUN/Z38sOq5npc3zIwoA= X-Forefront-PRVS: 09760A0505 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(7916002)(199003)(24454002)(189002)(377454003)(68736007)(42186005)(33656002)(47776003)(117156001)(65806001)(92566002)(66066001)(101416001)(59896002)(50466002)(4326007)(87266999)(64126003)(54356999)(5004730100002)(50986999)(23756003)(65816999)(65956001)(106356001)(110136002)(4001350100001)(105586002)(36756003)(586003)(76176999)(97736004)(3846002)(2950100001)(6116002)(77096005)(83506001)(86362001)(99136001)(230700001)(189998001)(2906002)(80316001)(19580405001)(19580395003)(8676002)(81166006)(81156014)(7846002)(26953001);DIR:OUT;SFP:1102;SCL:1;SRVR:AT5PR84MB0307;H:[192.168.142.156];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;AT5PR84MB0307;23:Eq2FsRDUZCC2CCmq//tTzPNYLmMDA2rhIx9GhPz?= =?iso-8859-1?Q?Bac0Ye2uvYsb63n1NeWh5CZZXv8q6+0p0L6hitq8BvgUdC5GrFwcFDCTTB?= =?iso-8859-1?Q?+CSwNLB+YHsIrvBMArzH3DyI6QtSA9tP5g4NOX7dl9617s+jgXqgFhKlgx?= =?iso-8859-1?Q?FHyXYzDkImmK6iusP2RmwkXBnS1TWmoDDIXYtvzj+ZWuZBFHut+nES8F8j?= =?iso-8859-1?Q?Jdt+K2RdXma9DiOaeC4gUPcaphMCDCbXVuSBuhUjEIntzjFmLp/C2h7f1H?= =?iso-8859-1?Q?H7rPjWHduyXyR5FXxlDkFyUYiqJgiTsfP6aPij0yZYlCInSIj6dQ7EVaPk?= =?iso-8859-1?Q?WZ9rObh5nCki+qUXbs8blNTx3xuGO2XNuVGA7lyAqje9N1tzJnYpiPVs29?= =?iso-8859-1?Q?4WirkTFf05psJIjlnyXcuhqbWsfNixhVN8D4Ll80wogihWkyMkFla8/5fo?= =?iso-8859-1?Q?FY2Io83Fz+pXRPIPhHwf+uWt2+hOmRmmmBbRcQ7/0l2303SywKBGGlonzj?= =?iso-8859-1?Q?IY7OAAdawaiSdJnTc/BEunGqBQ1C0N7FoJ7I8qX83P2scoXT7Hjycp48IS?= =?iso-8859-1?Q?VkGxswuUz6Wc829jKQO8wRBezhrPQQyngN7LqAoSpjyLmQw5+Fp10FMMtH?= =?iso-8859-1?Q?WKooXEjHLU2Z6hvrIlO8KzwIVIkZPi/eotRHaDkuK2h9dNV4S50igZANrF?= =?iso-8859-1?Q?z//R9OPRrCmrTcmhWMUWuDeISkdnehFIf96i4PyI7riHClublOeJDZ1fRl?= =?iso-8859-1?Q?2Oar8NQD0FKbVXMsqpr1QQ32VDJ07SpVJ39S1JVN7GQqy6+Q4jW8DJs0kt?= =?iso-8859-1?Q?25hqLnGCBHEJL+M24YjcZJYgOMph8FA+GtPs0uVn6agGVjDWU16F80yNH0?= =?iso-8859-1?Q?wvaCF80sUNwZ4jS1ZaFHV4VIMDS1utx1Wn/+IYvypJEkLCsEpNCpAAgOnc?= =?iso-8859-1?Q?08SBU+q0/WxEMjwhJ18IoMwG0RuJW2KO01JSkIALie4nO5v+OJvpVEAeSu?= =?iso-8859-1?Q?mmP8JTweM8s607ZBjNPrnz9AEpyR6Daw39IF5xOiJHila1Cat8CT8n4zp2?= =?iso-8859-1?Q?sfsNwGS3T3Oj0hGYzk/gwHpLLa9wDPDeNhSKrGw+ncozOp7vIfSsxa/5xk?= =?iso-8859-1?Q?dreN+EYkzRrM9sgnwALjGUWoommFYjg+YHaojiV+uT1AqnQECgsyTL+LTh?= =?iso-8859-1?Q?5Lcq4v+Ji5JLi56mQmBZbL7jo+bGXcuwp9Jv2tToswW27qLpNl++McszNA?= =?iso-8859-1?Q?kEWBoJexPd5Uku2o0S9kR1o75GII606EH21Djm0hTX53Yto0ChntQGdopz?= =?iso-8859-1?Q?WG5cK0puQI7WuyR8K5U46NmaegNWW5xcZaskm4pJc/gFb7OTpF/nN68w6R?= =?iso-8859-1?Q?hnPjttQgfNMzJIzWcl5GhJ0KWGB9k?= X-Microsoft-Exchange-Diagnostics: 1;AT5PR84MB0307;6:IZBGLRXIFBznUt934OPgSrDBT+D92qX/+bS0I7uFeNeCNP1WHCwvL0mljC8mYeFrXtUIHwQB6iC3JKIwOom5ROem5hZgeQ79A41ZYJjzZcheXpoAEUDD15sMJGyuwjRXycVBzmu214RTr+Jl0UzMhwEmM+ZRn/iVjWLD/ulk95RLhDOMZ/SH6J2+BzZWNGjhzM4YIIOjx622TqRaX9Ijs0O99LZLten+t5yeKQcKZmGxs19rkYmtT7Cv5mmObgzosthfJO18VFRW679GyNeCe+HderGBVv+dJhiVadCyCPA=;5:183JtDrooiSuSqlBUwlAIiV+YKR25KY65JrNrShZH/8Fq9T9G6gvo4SKYqCu7R9Kkfag/s35jWFJ3rfrwsEtyFdXdoYrLYbEqsBpNe/lhcrXliCaKtqwlZ+Nrto3ixu9oYtnybnuqR+BoA6pZkZocQ==;24:ouWugP6KtNNdZMLjSzWcybBz050mWb1PnV+oOj+XrA0wvzJjl5LKyaRxuHXD76mtVwPqNQ6Fx0zL2REeTx3PD3jFDyVK9gIw+bLdH4Ll7Zo=;7:P7XGskvwta88xqHnsiYDt4csmRzx+Exw7mmNplowu4jHn3b+VuSGsAjVhrOvzF+lZVx2bZEFB6pKO8Wdm1tQSwcs2aMwFZn6Wmxt0PLnytKhdGhKyf05w1UI/Lcm3eZBAq//1py8+tPDIjvck4DabHZORdxK9Fl4VaIwy6pFJmr/R7fMfqdcBBwGTNMNU1Xz5YvjhtPBi8gIN0eIow2VuFY/I71t/Dr+XiBsgCZxtnbA+07CifjMGr+jYTu8uMJH SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jun 2016 19:06:55.0438 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AT5PR84MB0307 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/15/2016 05:31 AM, Pan Xinhui wrote: > This patch aims to get rid of endianness in queued_write_unlock(). We > want to set __qrwlock->wmode to NULL, however the address is not > &lock->cnts in big endian machine. That causes queued_write_unlock() > write NULL to the wrong field of __qrwlock. > > Actually qrwlock can have same layout, IOW we can remove the #if > __little_endian in struct __qrwlock. With such modification, we only > need define some _QW* and _QR* with corresponding values in different > endian systems. > > Suggested-by: Will Deacon > Signed-off-by: Pan Xinhui > --- > change from v1: > A typo fix which is really bad... > thanks Will for the carefull review. :) > --- > include/asm-generic/qrwlock.h | 15 +++++++++++---- > kernel/locking/qrwlock.c | 10 ++++------ > 2 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h > index 54a8e65..28fb94a 100644 > --- a/include/asm-generic/qrwlock.h > +++ b/include/asm-generic/qrwlock.h > @@ -27,11 +27,18 @@ > /* > * Writer states& reader shift and bias > */ > -#define _QW_WAITING 1 /* A writer is waiting */ > -#define _QW_LOCKED 0xff /* A writer holds the lock */ > -#define _QW_WMASK 0xff /* Writer mask */ > +#ifdef __LITTLE_ENDIAN > #define _QR_SHIFT 8 /* Reader count shift */ > -#define _QR_BIAS (1U<< _QR_SHIFT) > +#define _QW_SHIFT 0 /* Writer mode shift */ > +#else > +#define _QR_SHIFT 0 /* Reader count shift */ > +#define _QW_SHIFT 24 /* Writer mode shift */ > +#endif > + > +#define _QW_WAITING (1U<< _QW_SHIFT) /* A writer is waiting */ > +#define _QW_LOCKED (0xffU<< _QW_SHIFT) /* A writer holds the lock */ > +#define _QW_WMASK (0xffU<< _QW_SHIFT) /* Writer mask */ > +#define _QR_BIAS (1U<< _QR_SHIFT) > > /* > * External function declarations > diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c > index fec0823..57d66cf 100644 > --- a/kernel/locking/qrwlock.c > +++ b/kernel/locking/qrwlock.c > @@ -30,18 +30,15 @@ struct __qrwlock { > union { > atomic_t cnts; > struct { > -#ifdef __LITTLE_ENDIAN > u8 wmode; /* Writer mode */ > u8 rcnts[3]; /* Reader counts */ > -#else > - u8 rcnts[3]; /* Reader counts */ > - u8 wmode; /* Writer mode */ > -#endif > }; > }; > arch_spinlock_t lock; > }; > > +#define _QW_MODEVAL(v) ((v)>> _QW_SHIFT) I know what you are doing here, but it is a bit hard to understand it just by looking at the name of the macro itself. Maybe some other names like _QW_MASKVAL() or_QW_BYTEVAL(). You may also want to have a line of comment about it. Other than that, I don't see any problem with it. Acked-by: Waiman Long > + > /** > * rspin_until_writer_unlock - inc reader count& spin until writer is gone > * @lock : Pointer to queue rwlock structure > @@ -127,7 +124,8 @@ void queued_write_lock_slowpath(struct qrwlock *lock) > struct __qrwlock *l = (struct __qrwlock *)lock; > > if (!READ_ONCE(l->wmode)&& > - (cmpxchg_relaxed(&l->wmode, 0, _QW_WAITING) == 0)) > + (cmpxchg_relaxed(&l->wmode, 0, > + _QW_MODEVAL(_QW_WAITING)) == 0)) > break; > > cpu_relax_lowlatency();