From: Hugh Dickins <hughd@google.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Hugh Dickins <hughd@google.com>, Dave Jones <davej@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: sleeping function warning from __put_anon_vma
Date: Mon, 2 Jun 2014 02:30:20 -0700 (PDT) [thread overview]
Message-ID: <alpine.LSU.2.11.1406020216240.1177@eggly.anvils> (raw)
In-Reply-To: <20140601193240.GF16155@laptop.programming.kicks-ass.net>
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 <davej@redhat.com>
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Needs-Ack-from: Peter Zijlstra <peterz@infradead.org>
> > ---
> >
> > 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 <davej@redhat.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
---
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 */
prev parent reply other threads:[~2014-06-02 9:31 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-30 0:09 sleeping function warning from __put_anon_vma Dave Jones
2014-05-31 20:33 ` Hugh Dickins
2014-05-31 21:04 ` Vlastimil Babka
2014-05-31 21:16 ` Hugh Dickins
2014-06-01 19:32 ` Peter Zijlstra
2014-06-02 9:30 ` Hugh Dickins [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LSU.2.11.1406020216240.1177@eggly.anvils \
--to=hughd@google.com \
--cc=akpm@linux-foundation.org \
--cc=davej@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).