From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753730AbaFBJbm (ORCPT ); Mon, 2 Jun 2014 05:31:42 -0400 Received: from mail-pb0-f43.google.com ([209.85.160.43]:52921 "EHLO mail-pb0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753237AbaFBJbk (ORCPT ); Mon, 2 Jun 2014 05:31:40 -0400 Date: Mon, 2 Jun 2014 02:30:20 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Peter Zijlstra cc: Hugh Dickins , Dave Jones , Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: sleeping function warning from __put_anon_vma In-Reply-To: <20140601193240.GF16155@laptop.programming.kicks-ass.net> Message-ID: References: <20140530000944.GA29942@redhat.com> <20140601193240.GF16155@laptop.programming.kicks-ass.net> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 1 Jun 2014, Peter Zijlstra wrote: > On Sat, May 31, 2014 at 01:33:13PM -0700, Hugh Dickins wrote: > > [PATCH] mm: fix sleeping function warning from __put_anon_vma > > > > Trinity reports BUG: > > sleeping function called from invalid context at kernel/locking/rwsem.c:47 > > in_atomic(): 0, irqs_disabled(): 0, pid: 5787, name: trinity-c27 > > __might_sleep < down_write < __put_anon_vma < page_get_anon_vma < > > migrate_pages < compact_zone < compact_zone_order < try_to_compact_pages .. > > > > Right, since conversion to mutex then rwsem, we should not put_anon_vma() > > from inside an rcu_read_lock()ed section: fix the two places that did so. > > > > Fixes: 88c22088bf23 ("mm: optimize page_lock_anon_vma() fast-path") > > Reported-by: Dave Jones > > Signed-off-by: Hugh Dickins > > Needs-Ack-from: Peter Zijlstra > > --- > > > > mm/rmap.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > --- 3.15-rc7/mm/rmap.c 2014-04-13 17:24:36.680507189 -0700 > > +++ linux/mm/rmap.c 2014-05-31 12:02:08.496088637 -0700 > > @@ -426,12 +426,14 @@ struct anon_vma *page_get_anon_vma(struc > > * above cannot corrupt). > > */ > > if (!page_mapped(page)) { > > + rcu_read_unlock(); > > put_anon_vma(anon_vma); > > anon_vma = NULL; > > + goto outer; > > } > > out: > > rcu_read_unlock(); > > - > > +outer: > > return anon_vma; > > } > > I think we can do that without the goto if we write something like: > > if (!page_mapped(page)) { > rcu_read_unlock(); > put_anon_vma(anon_vma); > return NULL; > } Yes, that was my preference too. Then I thought we'd be accused of mixing early returns and gotos, so did it the goto way. Since we both prefer the early return, I'll put it back to that. > > > @@ -477,9 +479,10 @@ struct anon_vma *page_lock_anon_vma_read > > } > > > > if (!page_mapped(page)) { > > + rcu_read_unlock(); > > put_anon_vma(anon_vma); > > anon_vma = NULL; > > - goto out; > > + goto outer; > > } > > > > /* we pinned the anon_vma, its safe to sleep */ > > @@ -501,6 +504,7 @@ struct anon_vma *page_lock_anon_vma_read > > > > out: > > rcu_read_unlock(); > > +outer: > > return anon_vma; > > } > > Same here too, I suppose. > > Interesting that we never managed to hit this one; it might also make > sense to put a might_sleep() in anon_vma_free(). Yes, good idea, that's an excellent place for a might_sleep(): done. Though it does change the patch from a "push this out immediately" to a "let's wait a little to check we don't now see lots of new warnings". But that's okay, we've survived three years without it; and I saw no no warnings myself from the new patch, when running under load plus lots of page migration. > > Other than that, I don't see anything really odd, then again, its sunday > evening and my thinking cap isn't exactly on proper. Thank you - shout if any worries arise once the cap is back on. I think it's fine, I just felt more comfortable bringing you in. [PATCH v2] mm: fix sleeping function warning from __put_anon_vma Trinity reports BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:47 in_atomic(): 0, irqs_disabled(): 0, pid: 5787, name: trinity-c27 __might_sleep < down_write < __put_anon_vma < page_get_anon_vma < migrate_pages < compact_zone < compact_zone_order < try_to_compact_pages .. Right, since conversion to mutex then rwsem, we should not put_anon_vma() from inside an rcu_read_lock()ed section: fix the two places that did so. And add might_sleep() to anon_vma_free(), as suggested by Peter Zijlstra. Fixes: 88c22088bf23 ("mm: optimize page_lock_anon_vma() fast-path") Reported-by: Dave Jones Signed-off-by: Hugh Dickins --- v2: use early return instead of goto add might_sleep in anon_vma_free mm/rmap.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) --- 3.15-rc8/mm/rmap.c 2014-04-13 17:24:36.680507189 -0700 +++ linux/mm/rmap.c 2014-06-01 13:08:24.432176737 -0700 @@ -103,6 +103,7 @@ static inline void anon_vma_free(struct * LOCK should suffice since the actual taking of the lock must * happen _before_ what follows. */ + might_sleep(); if (rwsem_is_locked(&anon_vma->root->rwsem)) { anon_vma_lock_write(anon_vma); anon_vma_unlock_write(anon_vma); @@ -426,8 +427,9 @@ struct anon_vma *page_get_anon_vma(struc * above cannot corrupt). */ if (!page_mapped(page)) { + rcu_read_unlock(); put_anon_vma(anon_vma); - anon_vma = NULL; + return NULL; } out: rcu_read_unlock(); @@ -477,9 +479,9 @@ struct anon_vma *page_lock_anon_vma_read } if (!page_mapped(page)) { + rcu_read_unlock(); put_anon_vma(anon_vma); - anon_vma = NULL; - goto out; + return NULL; } /* we pinned the anon_vma, its safe to sleep */