Messages in this thread Patch in this message | | | Date | Mon, 2 Jun 2014 02:30:20 -0700 (PDT) | From | Hugh Dickins <> | Subject | Re: sleeping function warning from __put_anon_vma |
| |
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 */
| |