From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754175AbcEOOsL (ORCPT ); Sun, 15 May 2016 10:48:11 -0400 Received: from mail-bl2on0131.outbound.protection.outlook.com ([65.55.169.131]:47261 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750958AbcEOOsJ (ORCPT ); Sun, 15 May 2016 10:48:09 -0400 Authentication-Results: hurleysoftware.com; dkim=none (message not signed) header.d=none;hurleysoftware.com; dmarc=none action=none header.from=hpe.com; Message-ID: <57388C13.20205@hpe.com> Date: Sun, 15 May 2016 10:47:47 -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 Hurley CC: Peter Zijlstra , Ingo Molnar , , Davidlohr Bueso , Jason Low , Dave Chinner , Scott J Norton , Douglas Hatch Subject: Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field References: <1462580424-40333-1-git-send-email-Waiman.Long@hpe.com> <5733AC64.6020306@hurleysoftware.com> <20160513150749.GT3192@twins.programming.kicks-ass.net> <573615AD.60300@hurleysoftware.com> In-Reply-To: <573615AD.60300@hurleysoftware.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [72.71.243.157] X-ClientProxiedBy: BLUPR11CA0069.namprd11.prod.outlook.com (10.141.30.37) To TU4PR84MB0317.NAMPRD84.PROD.OUTLOOK.COM (10.162.186.27) X-MS-Office365-Filtering-Correlation-Id: 1390b58b-fbfc-4e7f-c1d0-08d37ccfecb9 X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;2:/8a3S3HTI3gdgaxrd9Mi57ZXrmpjgbHDsQzdG0ERWpk37kMVxrK85PRiAP5y+NE3DTEZ0NARVS2JpiI4ww/+acLzy+Qj7HBAub5KpAt2wVPMnE4f/Ra8FrDJe0bMHS51cQFC0MR9eOja9YY5AB4y2jrC0drPbzhbuyAv+c3XdTH2a4tAfa7j6Rx2tlahtK9k;3:XAFXGb+1pwXAk75N9vewR0vQEMMfamjyvU2NNghRAtWbdKF8GFOgKU21g+O221vbhGRKoHl8jzl3h4unwwlnfxp/9J7u0Woe1iUI9yVZsBvPPjv4m0syEsebGUGow+nY X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0317; X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;25:sSq3kS8hGZYL+P/zkN9ilwJLYDJc33oK/19nrj+3s5Bhx3/aQRPq57eSTYQAzPSWuwhv9LfQ9HFcTZEAp3JaPONucIdgC8CdbVSlqP/tyNYzfGfpxa2HKLRt3+X6qWqFVlTjdU49gyQcaLJCrDFv7XSUzFpR6GnPmNCSPRpjguV9uryLbEwTSRuDISPyTVr0ns3UVC7/NFh5FUpzMvXo5WBki9DH1JsIJ+9Z+V3WF4Hq1sbSobvd4QIc0TX1L8xDfnTK7XLEfUG/DjZTnB7JlO7tnzzqgIIfY816at9yTduPVcu6c+L1OzJko/txQsUkL+dYQqm1GLEXqC/8LPVP3on8Pt5a+YgtiGfOoSv1jKtlAxsgJ+2vperAB/Cn9jUoFL3+fZYP+yujK5PjucWUqPIefRiTbzbi/VvYpTHLZSRpgQiylfuEEVYr8P2PtwVf8oPYQ4/6HpxCkyYLetok2ozB4jTGF/xA4QWF+fOrwo3XtMB/aZ0CeLJpbrYXILb9AU2w68dkM/mRzfnVZyKkzKhUM6P85mnHHKiSc8xEdacFj++mZ/F6qGS08eSdErKaR+D1mdjSptWnFD88KBtZNlhSoTbbisoB3JZPhjKF3RI6ZFwZV7FdWuANyrkNr0oKgGGZ114wWFgxLnkASUPiuaDiVzm/1pRAasWrV2mFm2sPAawZ9WwTnYrJxCuIniJFyQRFqR/HccfxJ3dnJ0z5MLKjxolb3I/BrbCdKicgJ2o= X-LD-Processed: 105b2061-b669-4b31-92ac-24d304d195dc,ExtAddr X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;20:EKzF3EYWayPq1+tpc2D3v9Dd8jV6lRgA2tkMlDOVhAfz3LpuKz5PsP5p/+gImoK+fXG4W1wqo4VXJwosUQhRNMb2vvTA/XoXzY+D5yPhFGgsP4mkncjWu/3IoSlcbkaD+1N2IrejxedUtA5kzjkBaHG3DgcXqBkZE0LMwkGAVBi2rdY5OvKaA4AIpEwb3zlcOORuvyCV3Qo/6ECIoaJSR4QsZc9lrCGHQuubbZ9HQ9/uKAs1KxUKRHkyR1oJifjSh3Mv0rerrKA8E1dCc7UCzkv7dzYo4eXjWQA121Dgkk8w6afEsjEMB22+53K5/j46GKtElIBs2m7ND0vYbYpcmNuEtT98ZQtwDWpX4UsedEhF/QWuPVWQFcWMB4V7m28uLFB3aURaBKzzmHQcdqG964Dqm4wkpdrDofZC4QhyGtY=;4:Yu1E/NNZAD2kuNSccbJz0PJ0Z/hn5gAUcLgf93N+cw+eO6L2ziUE67tVjaFFKDunWips7+2GUebmN2mMXBWL7OKb1n5UBFDEP0EDMQJ17h37JeGb2bw4q9DaqX2QHRTtL2lxgXUpkPi5QRN58YoJeyRPngeALjBsFntuIpHxtgWRygUOTFUA6mY5r5mqVIorNXH3/i9FAZQBo+AVFLGjssHjzhGhxz9Fm1mxpkrjCAJb/DLZyrj6P5qbI1Q+4BYh2W5BxY+WV+0uybBOksABOHygDyoFHlFSHRtvs7YE3AMgWSIJbshuXnD/Yn0OYVmXHHaCNu2GtiQrTFTwjIKdZuktPiMxmI0xu4v3+r+GNll2kwbKGgv0SIiwgxM+X/ZT 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)(10201501046)(3002001);SRVR:TU4PR84MB0317;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0317; X-Forefront-PRVS: 09435FCA72 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(377454003)(24454002)(65806001)(66066001)(65956001)(36756003)(8676002)(77096005)(47776003)(4001350100001)(117156001)(81166006)(2906002)(42186005)(110136002)(86362001)(93886004)(92566002)(23676002)(189998001)(5008740100001)(5004730100002)(230700001)(50466002)(54356999)(50986999)(76176999)(33656002)(64126003)(83506001)(586003)(6116002)(3846002)(2950100001)(7059030);DIR:OUT;SFP:1102;SCL:1;SRVR:TU4PR84MB0317;H:[192.168.142.131];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtUVTRQUjg0TUIwMzE3OzIzOmxLSU9MNVBnUW9iMEU0SFJTZmJqTzZYaUpP?= =?utf-8?B?b3pzbGNzWlRGN1Nnd2k5RWFNY3dQeXRFOTA2VWpYTXhnZXNZallPRC9sSlBp?= =?utf-8?B?cmVmQStIUTM3ZjBHNGlZV2Ezc2txUWlpM3J6ajZIS0U3TVJEWmZGV09JdnZ2?= =?utf-8?B?NmxOb1FMdWxxd1FjR1R1WlBGTkV5NkNOcVFNYlVmMkJPVnQrZnNEVHlLV0Jv?= =?utf-8?B?L2FyRXZkbWVlRzRpOFUzbnlmcHF1L1dleEFaSGY1eHlDaVVqOXZBbGdUc2px?= =?utf-8?B?aXlqN3VSR3RLd2NhN2ZvbEdNVnYyS2xIdENQU1FDUTBYcWVjaUFnTGVybDZI?= =?utf-8?B?RzNYVklLSzVEays5b0xRdVBPRXoybExNSFFteGcwQ0lQZENGcVc0bXV6V3Yr?= =?utf-8?B?REdFR0JTYlBhNzRuMG9ZUEVmL0hrOWtNcHhlVS9qL3ByOVRPN2lEQnpwdWFq?= =?utf-8?B?M1pvOTVvbmJUNGJNRitVeDltQUJPMUs3ZlZyQ3ZmYlpMU0M0Y3NnSjJBR3NR?= =?utf-8?B?WkpPaHRmLzJqTnhkMkwzMDBWSjB0eEVPMm5CeHkxQWZ5RVRTUUdrRkRMZDY4?= =?utf-8?B?dDczeXRWZEJYZTJhN0pUK0RTMC9STHVQL1NGU05LZjByemxGbGhXemJoUUdM?= =?utf-8?B?eVZGN2dXZW5pazZRb3dCYzQyRjc3bEZTWmFLTzFkNW1qd2xEY0JRMWw2alh2?= =?utf-8?B?UFE5ODk2L0Ywek1zNVZXWjBLZzlQb0FEcEVLTXByNjJIeFdUZHRYU0Qwb0Q0?= =?utf-8?B?VWVNY3NtUTc2SVh1cVVhS2RmcGdmUGlya25TZHBzRVk3ZUhTYnZmYk1uZjFJ?= =?utf-8?B?TnA2dXlpWXFDMzc5d1o5M3NrZnQvM2s0Tk9iZE1EMzNSYTZ3OFlqQ2JNKzFl?= =?utf-8?B?TU9jUFJZK21JMTBzZURweGZ6SndYQkkyQzhGZkUxZWdjTUNNWE9nRi9xbWg3?= =?utf-8?B?aU5wMGVOOWxaOFVCWjhIT1FQS2VJVG0xYUE0RjNWdlJFWTRBY25vNGFlZUZO?= =?utf-8?B?WWR0NE1GNEQyV2lVN21VeHI4Ym1FL3JMQk5ZWEF1ZEhaR1pBdktWeFppS3dn?= =?utf-8?B?ejlyU0NrdWtwbmU0ODRKRkZpalVrbjEzaS9ldmQxQmxsTXhHK3VnZmJqbGJx?= =?utf-8?B?UkQ2K0xwSy9HeFpLWmtOWGZNTWhHRHBKUzlNaGNKVmQwUG8rRXJOa3JrOWho?= =?utf-8?B?Wk5sOHFkNmNVTVo4ejRMd1ZBRnFucGdrckRCTWxDMHVIL3FBdm9LOCtUbVlP?= =?utf-8?B?SEIrNEl5TEVTcUFmQVlJV2ZkMzNpME9wMmE4cGttNGVKYmwyVXYzcjkvVXZ2?= =?utf-8?B?K2M1S1hKeVlDUXgwRG5VYjlvQ3VCVE53ZmRGcC9KNHNQVEZpV2VmemJGMWVD?= =?utf-8?Q?+OheIn1c?= X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0317;5:JAZzG7Ch3jIjQLG+yNX452R5SaJyZIVN4eaW5q7/tkMQO5HI/fvlvL+bCzNxw1ht0uZ8a7O9uJsVbIgpH04jWI/qQKfxszLpz+/x0Z61JZiliZu2J202lu9UgXrUktixCi6rYyBGK1Jof4HDUXDi5w==;24:RciPfq4wKb6jlU/RIKV1kMeSs/4apxnYPQD88IQ8rIH5oVbeIN5zSVcczb4H4Hi+qM902Utx6UlclccpdMPHZsDVz2ut1aTT7D7LF6YFz0o=;7:uDd35glKYoRVhdq3qPbVa1HNHfp6Wb5J4e2ISDuCkxrtonjHZbz0iJMwi5ghb6yxBxaX0WqwJiLLfWESGVvXmx6QMFnoLJy1qJj13PTYlU5YGJXqvoa0zjXWuAzqpC24HZnJhG2SEg7PerwXu8bzu3xE3eeeVlkYSEm89xgdnPh9WmtowMYXYnS232ViK+k3 SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2016 14:48:02.8385 (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 05/13/2016 01:58 PM, Peter Hurley wrote: > On 05/13/2016 08:07 AM, Peter Zijlstra wrote: >> On Wed, May 11, 2016 at 03:04:20PM -0700, Peter Hurley wrote: >>>> + return !rwsem_is_reader_owned(READ_ONCE(sem->owner)); >>> It doesn't make sense to force reload sem->owner here; if sem->owner >>> is not being reloaded then the loop above will execute forever. >>> >>> Arguably, this check should be bumped out to the optimistic spin and >>> reload/check the owner there? >>> >> Note that barrier() and READ_ONCE() have overlapping but not identical >> results and the combined use actually makes sense here. >> >> Yes, a barrier() anywhere in the loop will force a reload of the >> variable, _however_ it doesn't force that reload to not suffer from >> load tearing. >> >> Using volatile also forces a reload, but also ensures the load cannot >> be torn IFF it is of machine word side and naturally aligned. >> >> So while the READ_ONCE() here is pointless for forcing the reload; >> that's already ensured, we still need to make sure the load isn't torn. > If load tearing a naturally aligned pointer is a real code generation > possibility then the rcu list code is broken too (which loads ->next > directly; cf. list_for_each_entry_rcu()& list_for_each_entry_lockless()). > > For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc > that had to do with control dependencies and not load tearing. > > OTOH, this patch might actually produce store-tearing: > > +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) > +{ > + /* > + * We check the owner value first to make sure that we will only > + * do a write to the rwsem cacheline when it is really necessary > + * to minimize cacheline contention. > + */ > + if (sem->owner != RWSEM_READER_OWNED) > + sem->owner = RWSEM_READER_OWNED; > +} > > > Regards, > Peter Hurley While load tearing in the argument to rwsem_is_reader_owned() isn't an issue as the wrong decision won't do any harm. Store tearing as identified above can be a problem. I will fix that even though the the chance of compiling generating store tearing code is really small. Cheers, Longman