From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752268AbcEUQEW (ORCPT ); Sat, 21 May 2016 12:04:22 -0400 Received: from mail-pf0-f173.google.com ([209.85.192.173]:34576 "EHLO mail-pf0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752090AbcEUQET (ORCPT ); Sat, 21 May 2016 12:04:19 -0400 Subject: Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE To: Jason Low , Waiman Long 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> <1463601515.2587.24.camel@j-VirtualBox> Cc: Davidlohr Bueso , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Dave Chinner , "Paul E. McKenney" , Scott J Norton , Douglas Hatch , jason.low2@hp.com From: Peter Hurley Message-ID: <574086FE.6080807@hurleysoftware.com> Date: Sat, 21 May 2016 09:04:14 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <1463601515.2587.24.camel@j-VirtualBox> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/18/2016 12:58 PM, Jason Low wrote: > 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. Which doesn't cost anything either. >> 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. See, I don't understand this line of reasoning at all. I read this as "it's ok to be non-optimal here where were spinning CPU time but not ok to be non-optimal generally elsewhere where it's way less important like at init time". And by the way, it's not just "here" but _everywhere_. What about reading ->on_cpu locklessly? Sure it's a bool, but doesn't the "we need to document lockless access" argument equally apply here? Regards, Peter Hurley