From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752137AbdAaOX0 (ORCPT ); Tue, 31 Jan 2017 09:23:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49694 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752340AbdAaOXJ (ORCPT ); Tue, 31 Jan 2017 09:23:09 -0500 Subject: Re: Q: lockdep_assert_held_read() after downgrade_write() To: Peter Zijlstra , Jens Axboe References: <18295.1485811542@jrobl> <86195df9-2a43-2a0f-38ac-68792edc41c0@fb.com> <20170131103620.GM6515@twins.programming.kicks-ass.net> <20170131112526.GH6536@twins.programming.kicks-ass.net> Cc: "J. R. Okajima" , linux-kernel@vger.kernel.org, darrick.wong@oracle.com, david@fromorbit.com, dave@stgolabs.net From: Waiman Long Organization: Red Hat Message-ID: <9e2f505e-3d1f-3573-661a-b2ad185566b1@redhat.com> Date: Tue, 31 Jan 2017 09:23:08 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170131112526.GH6536@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 31 Jan 2017 14:23:09 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/31/2017 06:25 AM, Peter Zijlstra wrote: > On Tue, Jan 31, 2017 at 11:36:20AM +0100, Peter Zijlstra wrote: >> On Mon, Jan 30, 2017 at 02:30:45PM -0700, Jens Axboe wrote: >>> I don't think you understand how it works. downgrade_write() turns a write >>> lock into read held. To make that last sequence valid, you'd need: >> Correct, and I'm surprised that didn't explode in different ways. >> >>> down_write(&rw); >>> downgrade_write(&rw); >>> lockdep_assert_held_read(&rw) >>> up_read(&rw); >>> >>> or just not drop up_write() from the last section. >> Right, but also, there seems to be a missing lockdep annotation to make >> that work. That is, downgrade_write() doesn't have a lockdep annotation, >> so it (lockdep) will still think its a write lock. >> >> >> Let me try and fix both issues. > Something like so I suppose,... completely untested. > > There could be a good reason for the current lockdep behaviour, but I > cannot remember. > > --- > diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c > index 45ba475d4be3..dfa9e40f83d5 100644 > --- a/kernel/locking/rwsem.c > +++ b/kernel/locking/rwsem.c > @@ -123,10 +123,9 @@ EXPORT_SYMBOL(up_write); > */ > void downgrade_write(struct rw_semaphore *sem) > { > - /* > - * lockdep: a downgraded write will live on as a write > - * dependency. > - */ > + rwsem_release(&sem->dep_map, 1, _RET_IP_); > + rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_); > + > rwsem_set_reader_owned(sem); > __downgrade_write(sem); > } > diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h > index a699f4048ba1..3bd584c81b0b 100644 > --- a/kernel/locking/rwsem.h > +++ b/kernel/locking/rwsem.h > @@ -40,8 +40,10 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) > * do a write to the rwsem cacheline when it is really necessary > * to minimize cacheline contention. > */ > - if (sem->owner != RWSEM_READER_OWNED) > + if (sem->owner != RWSEM_READER_OWNED) { > + WARN_ON_ONCE(sem->owner != current); > WRITE_ONCE(sem->owner, RWSEM_READER_OWNED); > + } > } > > static inline bool rwsem_owner_is_writer(struct task_struct *owner) I don't think you can do a WARN_ON_ONCE() check for sem->owner != current here. If the rwsem starts from an unlock state, sem->owner will be NULL and an incorrect warning message will be printed. Cheers, Longman