Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756608AbdLPLgv (ORCPT ); Sat, 16 Dec 2017 06:36:51 -0500 Received: from mx2.suse.de ([195.135.220.15]:44516 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752535AbdLPLgs (ORCPT ); Sat, 16 Dec 2017 06:36:48 -0500 Date: Sat, 16 Dec 2017 12:36:45 +0100 From: Michal Hocko To: Tetsuo Handa Cc: Andrew Morton , David Rientjes , Andrea Arcangeli , Benjamin Herrenschmidt , Paul Mackerras , Oded Gabbay , Alex Deucher , Christian =?iso-8859-1?Q?K=F6nig?= , David Airlie , Joerg Roedel , Doug Ledford , Jani Nikula , Mike Marciniszyn , Sean Hefty , Dimitri Sivanich , Boris Ostrovsky , =?iso-8859-1?B?Suly9G1l?= Glisse , Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [patch v2 1/2] mm, mmu_notifier: annotate mmu notifiers with blockable invalidate callbacks Message-ID: <20171216113645.GG16951@dhcp22.suse.cz> References: <20171215150429.f68862867392337f35a49848@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2164 Lines: 52 On Sat 16-12-17 16:14:07, Tetsuo Handa wrote: > On 2017/12/16 8:04, Andrew Morton wrote: > >> The implementation is steered toward an expensive slowpath, such as after > >> the oom reaper has grabbed mm->mmap_sem of a still alive oom victim. > > > > some tweakage, please review. > > > > From: Andrew Morton > > Subject: mm-mmu_notifier-annotate-mmu-notifiers-with-blockable-invalidate-callbacks-fix > > > > make mm_has_blockable_invalidate_notifiers() return bool, use rwsem_is_locked() > > > > > @@ -240,13 +240,13 @@ EXPORT_SYMBOL_GPL(__mmu_notifier_invalid > > * Must be called while holding mm->mmap_sem for either read or write. > > * The result is guaranteed to be valid until mm->mmap_sem is dropped. > > */ > > -int mm_has_blockable_invalidate_notifiers(struct mm_struct *mm) > > +bool mm_has_blockable_invalidate_notifiers(struct mm_struct *mm) > > { > > struct mmu_notifier *mn; > > int id; > > - int ret = 0; > > + bool ret = false; > > > > - WARN_ON_ONCE(down_write_trylock(&mm->mmap_sem)); > > + WARN_ON_ONCE(!rwsem_is_locked(&mm->mmap_sem)); > > > > if (!mm_has_notifiers(mm)) > > return ret; > > rwsem_is_locked() test isn't equivalent with __mutex_owner() == current test, is it? > If rwsem_is_locked() returns true because somebody else has locked it, there is > no guarantee that current thread has locked it before calling this function. > > down_write_trylock() test isn't equivalent with __mutex_owner() == current test, is it? > What if somebody else held it for read or write (the worst case is registration path), > down_write_trylock() will return false even if current thread has not locked it for > read or write. > > I think this WARN_ON_ONCE() can not detect incorrect call to this function. Yes it cannot catch _all_ cases. This is an inherent problem of rwsem_is_locked because semaphores do not really have the owner concept. The core idea behind this, I guess, is to catch obviously incorrect usage and as such it gives us a reasonabe coverage. I could live without the annotation but rwsem_is_locked looks better than down_write_trylock to me. -- Michal Hocko SUSE Labs