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@xxxxxxxxxx>
> > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > Needs-Ack-from: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxx>
Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
---
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 */
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>