From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751278AbcGNC1r (ORCPT ); Wed, 13 Jul 2016 22:27:47 -0400 Received: from mail-by2nam03on0124.outbound.protection.outlook.com ([104.47.42.124]:54637 "EHLO NAM03-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750865AbcGNC1h (ORCPT ); Wed, 13 Jul 2016 22:27:37 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=waiman.long@hpe.com; Message-ID: <5786F0E6.4020800@hpe.com> Date: Wed, 13 Jul 2016 21:54:46 -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: Peter Zijlstra CC: Pan Xinhui , , , , Subject: Re: [PATCH v3] locking/qrwlock: Let qrwlock has same layout regardless of the endian References: <1466403652-2931-1-git-send-email-xinhui.pan@linux.vnet.ibm.com> <20160713195423.GD30921@twins.programming.kicks-ass.net> In-Reply-To: <20160713195423.GD30921@twins.programming.kicks-ass.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [72.71.243.165] X-ClientProxiedBy: SN1PR15CA0021.namprd15.prod.outlook.com (10.163.200.31) To TU4PR84MB0318.NAMPRD84.PROD.OUTLOOK.COM (10.162.186.28) X-MS-Office365-Filtering-Correlation-Id: 7a38aa2e-62fd-417e-b768-08d3ab89dc3a X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0318;2:23NeWYwVHlbcAYILmWlPG6ok0Ri0tZxtt7B3I60NO+JsiAkxqW8qsqTAe+eBjxB2Ei1ukdTFYYuVAHl42QVCx25CmzTW4AOInG6zcKdU5LQFcp2z4zQ7tCmH9hrkfJxppfv7Ba/DJndGTxjg3lFu7SiGYyiFxmps6l5FX+l/5WGPNiPFO3az9Rldwiakknsc;3:iytIc5IZIG18TaM0/XrtPyZH2r1bqOpj4GjcDP8h7EBtNOrBKwG73adskKP4ILJT0iqc0FmJMMFh1s+nJXDfiPIBAHzvmGdsBSRApJKrCu3L3vARoHts7ovYaOeFt9NY;25:gOF0vsC9TYiU8PafOOY6iAynICMt2cQbTpFcXV6qloFrrgwn9pNHn4AVQGo29UCpdAxQA6CSxhfb1Kt2V/y/4py5jXXTXhKnwkDRpX/VFxrQOyOW04zlBUmWTqJp5cBG4k4wAVsi1KHcrSr48KiiZLg7vyn5Ma025cm4nGVMzUPm9zeKI+iS2ZURQIfKu2ve3jgQFZ7hwBi5NE1KS/0s6PIwuWcbvISosnwRIQmuqahsbm0UT0tpJyPnvsi90u1FjsRIBzn3QxSnTeQ5WCsfG8aol7tddqrsiBtgTYsS0Pyb7p9SD/tsCvl8wF/tBmP/dZxH+eTODkKDsxGbRGakklL5R82Jko/YNfBWpgrWe7fX0f2UYvLrHoWwNlYwx+kh44ylMSX5Kg7GrFQ3fEzf2D8fcKO71CoADeKA4jRDxo8=;31:VyvYRSCUs9Ai38w70LTEVHUyNDJn1l2hS+nzHCZUVqybnPCPbFLPp7QHAhRGmivsjhir8aHB+X4PfqZ169WdPG5A6vS4yQmm7RBMOkplOpJzt4Oizq5olDOYs9dLstNHXeCmW3VEwL4YUdwzLQyfmgdXSKqWTloLrBrGr5Lda/jvRllBNUsEP1GZq8VXpN+2E8QG0LuRF/EhnSmezwMpXw== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0318; X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0318;20:sHJFAqNxd2leymMhL1izN1b8EfHfUZN4kYGgEKgsO3zdSfpqzIRwStOSeCKx4Jn7PJOXaeeWRkJAfc+Qrj13pa+8DfERXRctv9Dv71prLk1fgTx/Lh+HQZd02RgqYuOSvJ7rEZsrcfwbI7+fbSVRNcOCUEvKsTKK4Ub8d2BSs7U7FOqkZl9VG8NiNnDk+QkQ77tJ/9hJZTLbim8Mp/d6yQdlF6dAthza4896te0ROgttw5n3MQ/CKjQW/aRtWtH9wWVwT7c+pbY9qQ/D1l4JWiHfimdKg6n78UrWazMR+PN3oUKrF5LX6/KSCOd1jwePpFjfzPbdjKJCIp7d9nj2EPK9mrMf88Lhu1a45UP/AAMlwZ0gy3gEef9mVv0qrfszbdUYTDzKre40eRMjr/QrV6WH+u7rn9uKtPRFeMyJaZFtId529c5lPxdzodl3LzrY1onaj2htqtN+0xcerwWMUqQjOKjQJZtOCenDlNZkDaSEl0XTXtDjGyBitj0iTx2d 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)(8121501046)(5005006)(3002001)(10201501046)(6055026);SRVR:TU4PR84MB0318;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0318; X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0318;4:KjwDjlP3LDTErzmr4YrIy2OUhHIm8EMv1mXwVfjxUOtq78HNAeYtWHti+sff2GayliQrrdqXoj20ellIMQjNkbqVwowSeF6LpahJF8rUTNZM3qGC7wOcmM/UD5EjscOUxLrYnSA+HuYQI1xW/9rRJcR7p/ApIMFSf+Rm0zETrLRcjQfYTmdBcZMo7M2nK+FkLyu5Fk7mXg21JjUXs5dXvUfHtR1rzQ8tzKqTi4e65S7Nupd++1aCrz+t9ek+sWqitmdGeT8mLGZYJN8kinxldg7Bn3TSnSbpV0af69hXRL29rThwfO8+iNWkf/YR+cTdpy8dJUjRr8xKq8GahW1BO0/Yrzeh6nW5xWrgV0GJ2VJ9FORjbtnhCaYkdzcUE2rCXZUw4udtf5/IMsWcoRxBg/4YCdoXXHqUfS30R7sgPYvpcnWH1v+sJUmacgHod9nkf+9lGvDGYmMzJ9ReBb76ntbJ8xyTX/mEZF9f5aI+mmKdbgHZIKHmc6hMZTdbwgEx X-Forefront-PRVS: 00032065B2 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(7916002)(377454003)(189002)(24454002)(199003)(65816999)(2906002)(105586002)(106356001)(97736004)(87266999)(54356999)(189998001)(4001350100001)(50986999)(6116002)(7846002)(110136002)(7736002)(76176999)(2950100001)(3846002)(586003)(101416001)(4326007)(305945005)(77096005)(92566002)(19580395003)(19580405001)(8676002)(47776003)(230700001)(33656002)(42186005)(66066001)(65806001)(65956001)(36756003)(23756003)(64126003)(50466002)(83506001)(81166006)(81156014)(68736007)(117156001)(86362001)(80316001)(59896002)(26953001);DIR:OUT;SFP:1102;SCL:1;SRVR:TU4PR84MB0318;H:[192.168.142.166];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;TU4PR84MB0318;23:1xRh8cALHJqZuoDlcvLgxMatvNkHDjXDSOnwetB?= =?iso-8859-1?Q?h8H/j8s4BPzjinhNPM2vk6g+c68ilJBcOiKS+YbVm9Lc2ikr3Jz+VUiZX/?= =?iso-8859-1?Q?FVSCAN2s5YE2TSRIfvpcsVn5pM/lo8guUD/HgdpKwszB8CUJSojfzTl7f5?= =?iso-8859-1?Q?pfgrYE0lBOLlqVka5FkaZS1XDdT4yJNTt77wXyI4xovFgx1bkSbMaR0fm0?= =?iso-8859-1?Q?dcAPksoKVzmr83yX3dx3pndetN5yxUkriCcvq9hgaGlqnG6Sb0t60Ug4Re?= =?iso-8859-1?Q?bjZMfPU9iDbWYZapskxYb9eSoDaWMTUzMEEtStEERF5x4quJ/0CNu26d+R?= =?iso-8859-1?Q?Xq30cVu0iHlE9WV/rMjJgWpUQXIIhV0V0bgBAe+AgyP8H7IYqMVehTkwNE?= =?iso-8859-1?Q?YhmePxgZky0eGEyNTOcNB7nGkCsp85qzPrsFbyzurirwRVRA6eWkfst7wX?= =?iso-8859-1?Q?DRIBMdFaYMQk/rCsqKaG0mP4aEKa2BvB+PdoXptYMaEe2dGx4VLDlZuEgZ?= =?iso-8859-1?Q?EYC7fl0KoJEnBp7Sorbn/CehcLwULxiZL+D1NHo3b1X2pi7FY5JaCpRZXz?= =?iso-8859-1?Q?no5JlALz520yz70jdSLDrBm2XcBy5ZCzJ0nNcgO48+c9+BWlafEANyL2xV?= =?iso-8859-1?Q?Q3wmaB24dqCgj/NUR2tvz7DIM9E+aGj9CwrwPlsy2jzkbcJPLM1HDO4lXE?= =?iso-8859-1?Q?bw6+5Z3mkpRUdQlKNC5IRfMMKsswHUlfk5F7T34CXPzBmToAUNN4Nf36m8?= =?iso-8859-1?Q?BKLSNZlefDHqdjcKjsP7avXTkyHJ0MCy+tUS0GmXg4zxej4suPi1qWEZZD?= =?iso-8859-1?Q?amLkRC6o1SEaYMaQzm8FN1/Kc+hAq9L7TQk0wU6XNEpvi4qBJMjW4u2U1M?= =?iso-8859-1?Q?rio37WM+3xC27wQrioAfq3aZ8Di5KxfOgd9o3DeCKYLbH0xmL5JiMRZj3T?= =?iso-8859-1?Q?6lQBGcp0Src++OWBrzQl2laCgSLAa6GtKvYkHri0GJkTZYhrsVkv2Vt0f1?= =?iso-8859-1?Q?sYWLZ7TQ5fk3XKL4IiBwJgXup7mVGIsvJhJSEaN5DD5ZK058Gd+5y9WJYt?= =?iso-8859-1?Q?v3/hwztc6/vaQx2pI0QYHZEqOODc6WiC9UbHqLThKZwjp60T0MuMvUlpga?= =?iso-8859-1?Q?VZx1D5fF+bhXlVzj/teKI9wwPDgUVDcTLOw9VXEZhPVTb9eK4r08CCetlG?= =?iso-8859-1?Q?3lHKi7LgC1b44WpE8LRx5wNI3OdkbYfWKwcI1VU70hM/9YyEa3nrmEzB56?= =?iso-8859-1?Q?pHFe5UngJrifxIGdE9FqzgveSDGuhEuh6JoXM4g0pUUhOtocx0eKiQCU5T?= =?iso-8859-1?Q?CoKWuWs/lzASAFzbzmWmO+8ewKuvZY33iWeh2RK7tfdtf8q69Syr9rhPbr?= =?iso-8859-1?Q?QooAGAJfeFXSyPzykhEcaHJKJlWVo?= X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0318;6:3yFypS9zXoaV48XORrq3c8h11HVvHqeMSTVaIYuzjHUGhGkFkikUyY9CTEzCW3XDaBE7lwZIZ5B6ctP35HjY8SOTtv9uOnr3KYvQhlL2L2oygc08B54I7yA9bBBeviPFEEPacDwpuMBz0yUwgELEk/s2j/O+eEZX0tSRs8cqSdqxzzDpMfpVX9OU27/hyn1Ilm5jC6nWG4a4Luxp3Ni83GIUemZRPzn9UQZ4l/JeosLA5nBTTkI+xcpFouUqWB1DQns0JdZ8usb+w/zfxAluilY7X1dvu355H46GaAW6yw5Q7Y0HXXDV65dyEKZiUzA/KtSjeAY8z161uK9uPKJ9WA==;5:t3dFbC4eVCjH0fxytejqwhBRKwczAPiagZ0wt0XOQeJ8mzbElQ0LlE+f5Tgfiol/royGlwmErDk2cqXEZUxJAkGsxiuXs5acJi01noNauGVUDMrB2UCejIAjdrBd5S1GOAeDysn5F74vrBsdW+Z5Hg==;24:keOuwUE2fwDOd15Icr9U9S7Ciu6+mU/dZ1lZfcnT37Gftk45osN1SoL2XHc7fQ79Rohh2F+70eZZlVvpaM3l7FQL9BKlO7SeA2/h+DHJdkI=;7:45jVIQZYjGlA7pitySF/4r+Nk4w08Sjg//jyXHoiWHO1PcVXAT7e4QUG46j2Du52FRzm+VnJaHKKmKS4hmWK5VznZcHYSHLndd2YPyf6uoMoFOlDNgTkziK+5p9LW+MvRCYA06oL0x7PT7NeFkxi64603qpVf17bfsXGIMwc2gUkh+CGHy2w+9Y1ynPNiDudnyRZUNMZaWA037nnxYNHFIf77XdNjU6tfbBG3vO8wWa6D2FUviYnncl0RobcxXX/ SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2016 01:54:57.4007 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: TU4PR84MB0318 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/13/2016 03:54 PM, Peter Zijlstra wrote: > On Mon, Jun 20, 2016 at 02:20:52PM +0800, 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 >> Acked-by: Waiman Long >> --- > Urgh, I hate this stuff :/ > > OK, so I poked at this a bit and I ended up with the below; but now > qrwlock and qspinlock are inconsistent; although I suspect qspinlock is > similarly busted wrt endian muck. Qspinlock is a bit different that the generic unlock code is endian-independent. The x86 architectural unlock code, however, is little-endian specific. This is not a problem. PPC will certainly needs its own unlock code for better performance. With this patch, there is indeed inconsistency on how qspinlock and qrwlock handle their internal data. I guess we could make qrwlock more like qspinlock if you want consistency, but I am fine with either ways. > Not sure what to do.. > > --- a/include/asm-generic/qrwlock.h > +++ b/include/asm-generic/qrwlock.h > @@ -25,13 +25,31 @@ > #include > > /* > - * Writer states& reader shift and bias > + * Writer states& reader shift and bias. > + * > + * | +0 | +1 | +2 | +3 | > + * ----+----+----+----+----+ > + * LE | 12 | 34 | 56 | 78 | 0x12345678 > + * ----+----+----+----+----+ > + * BE | 78 | 56 | 34 | 12 | 0x12345678 > + * ----+----+----+----+----+ > + * | wr | rd | > + * +----+----+----+----+ > + * > */ > -#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 (0x01U<< _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 (0x01U<< _QR_SHIFT) > I like the comment at the top (after you fixed the typo :-) ). > /* > * External function declarations > --- a/kernel/locking/qrwlock.c > +++ b/kernel/locking/qrwlock.c > @@ -22,26 +22,6 @@ > #include > #include > > -/* > - * This internal data structure is used for optimizing access to some of > - * the subfields within the atomic_t cnts. > - */ > -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; > -}; > - > /** > * rspin_until_writer_unlock - inc reader count& spin until writer is gone > * @lock : Pointer to queue rwlock structure > @@ -124,10 +104,10 @@ void queued_write_lock_slowpath(struct q > * or wait for a previous writer to go away. > */ > for (;;) { > - struct __qrwlock *l = (struct __qrwlock *)lock; > + u8 *wr = (u8 *)lock; > > - if (!READ_ONCE(l->wmode)&& > - (cmpxchg_relaxed(&l->wmode, 0, _QW_WAITING) == 0)) > + if (!READ_ONCE(*wr)&& > + (cmpxchg_relaxed(wr, 0, _QW_WAITING>> _QW_SHIFT) == 0)) > break; > > cpu_relax_lowlatency(); Cheers, Longman