From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753524AbcERT7p (ORCPT ); Wed, 18 May 2016 15:59:45 -0400 Received: from g1t6223.austin.hp.com ([15.73.96.124]:43214 "EHLO g1t6223.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751917AbcERT7o (ORCPT ); Wed, 18 May 2016 15:59:44 -0400 X-Greylist: delayed 155621 seconds by postgrey-1.27 at vger.kernel.org; Wed, 18 May 2016 15:59:44 EDT Message-ID: <1463601515.2587.24.camel@j-VirtualBox> Subject: Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE From: Jason Low To: Waiman Long Cc: Davidlohr Bueso , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Dave Chinner , Peter Hurley , "Paul E. McKenney" , Scott J Norton , Douglas Hatch , jason.low2@hp.com Date: Wed, 18 May 2016 12:58:35 -0700 In-Reply-To: <573CB496.4010707@hpe.com> References: <1463534783-38814-1-git-send-email-Waiman.Long@hpe.com> <1463534783-38814-3-git-send-email-Waiman.Long@hpe.com> <20160518140436.GA6273@linux-uzut.site> <1463592095.3369.10.camel@j-VirtualBox> <573CB496.4010707@hpe.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote: > On 05/18/2016 01:21 PM, Jason Low wrote: > > On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote: > >> On Tue, 17 May 2016, Waiman Long wrote: > >> > >>> Without using WRITE_ONCE(), the compiler can potentially break a > >>> write into multiple smaller ones (store tearing). So a read from the > >>> same data by another task concurrently may return a partial result. > >>> This can result in a kernel crash if the data is a memory address > >>> that is being dereferenced. > >>> > >>> This patch changes all write to rwsem->owner to use WRITE_ONCE() > >>> to make sure that store tearing will not happen. READ_ONCE() may > >>> not be needed for rwsem->owner as long as the value is only used for > >>> comparison and not dereferencing. > > It might be okay to leave out READ_ONCE() for reading rwsem->owner, but > > couldn't we include it to at least document that we're performing a > > "special" lockless read? > > > > Using READ_ONCE() does have a bit of cost as it limits compiler > optimization. If we changes all access to rwsem->owner to READ_ONCE() > and WRITE_ONCE(), we may as well change its type to volatile and be done > with. Right, although there are still places like the init function where WRITE_ONCE isn't necessary. > I am not against doing that, but it feels a bit over-reach for me. > On the other hand, we may define a do-nothing macro that designates the > owner as a special variable for documentation purpose, but don't need > protection at that particular call site. It should be fine to use the standard READ_ONCE here, even if it's just for documentation, as it's probably not going to cost anything in practice. It would be better to avoid adding any special macros for this which may just add more complexity.