Re: [PATCH 2/2] mm, thp: clear PG_mlocked when last mapping gone

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Tue, Jan 05, 2016 at 10:37:18AM +0100, Vlastimil Babka wrote:
> On 12/29/2015 09:46 PM, Kirill A. Shutemov wrote:
> >I missed clear_page_mlock() in page_remove_anon_compound_rmap().
> >It usually shouldn't cause any problems since we munlock pages
> >explicitly, but in conjunction with missed munlock in __oom_reap_vmas()
> >it causes problems:
> >  http://lkml.kernel.org/r/5661FBB6.6050307@oracle.com
> >
> >Let's put it in place an mirror behaviour for small pages.
> >
> >NOTE: I'm not entirely sure why we ever need clear_page_mlock() in
> >page_remove_rmap() codepath. It looks redundant to me as we munlock
> >pages anyway. But this is out of scope of the patch.
> 
> Git blame actually quickly points to commit e6c509f854550 which explains it
> :)

Okay, it explains situation somewhat.

The thing which still makes me a bit uncomfortable with the situation is
that we remove PG_mlocked only when the last mapping of the page gone.
It's not necessary the mapping which was VM_LOCKED. It means we can rely
on the clear_page_mlock() inside page_remove_rmap() only when remove all
page mappings at once (like in truncate case).

The clear_page_mlock() also helps hide real mlock leak bugs, like fixed by
patch 1/2: we saliently munlock page when last mapping gone, even if the
VMA was never been mlocked in the first place.

That's kinda suboptimal.

> 
> >
> >The patch can be folded into
> >  "thp: allow mlocked THP again"
> >
> >Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 
> Ack.
> 
> >Reported-by: Sasha Levin <sasha.levin@oracle.com>
> >---
> >  mm/rmap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/mm/rmap.c b/mm/rmap.c
> >index 384516fb7495..68af2e32f7ed 100644
> >--- a/mm/rmap.c
> >+++ b/mm/rmap.c
> >@@ -1356,6 +1356,9 @@ static void page_remove_anon_compound_rmap(struct page *page)
> >  		nr = HPAGE_PMD_NR;
> >  	}
> >
> >+	if (unlikely(PageMlocked(page)))
> >+		clear_page_mlock(page);
> >+
> >  	if (nr) {
> >  		__mod_zone_page_state(page_zone(page), NR_ANON_PAGES, -nr);
> >  		deferred_split_huge_page(page);
> >
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>