Re: [PATCH 11/24] huge tmpfs: shrinker to migrate and free underused holes

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

On Sun, Mar 22, 2015 at 09:40:02PM -0700, Hugh Dickins wrote:
> (I think Kirill has a problem of that kind in his page_remove_rmap scan).
> 
> It will be interesting to see what Kirill does to maintain the stats
> for huge pagecache: but he will have no difficulty in finding fields
> to store counts, because he's got lots of spare fields in those 511
> tail pages - that's a useful benefit of the compound page, but does
> prevent the tails from being used in ordinary ways.  (I did try using
> team_head[1].team_usage for more, but atomicity needs prevented it.)

The patch below should address the race you pointed, if I've got all
right. Not hugely happy with the change though.

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 435c90f59227..a3e6b35520f8 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -423,8 +423,17 @@ static inline void page_mapcount_reset(struct page *page)
 
 static inline int page_mapcount(struct page *page)
 {
+	int ret;
 	VM_BUG_ON_PAGE(PageSlab(page), page);
-	return atomic_read(&page->_mapcount) + compound_mapcount(page) + 1;
+	ret = atomic_read(&page->_mapcount) + 1;
+	if (compound_mapcount(page)) {
+		/*
+		 * positive compound_mapcount() offsets ->_mapcount by one --
+		 * substract here.
+		*/
+	       ret += compound_mapcount(page) - 1;
+	}
+	return ret;
 }
 
 static inline int page_count(struct page *page)
diff --git a/mm/rmap.c b/mm/rmap.c
index fc6eee4ed476..f4ab976276e7 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1066,9 +1066,17 @@ void do_page_add_anon_rmap(struct page *page,
 		 * disabled.
 		 */
 		if (compound) {
+			int i;
 			VM_BUG_ON_PAGE(!PageTransHuge(page), page);
 			__inc_zone_page_state(page,
 					      NR_ANON_TRANSPARENT_HUGEPAGES);
+			/*
+			 * While compound_mapcount() is positive we keep *one*
+			 * mapcount reference in all subpages. It's required
+			 * for atomic removal from rmap.
+			 */
+			for (i = 0; i < nr; i++)
+				atomic_set(&page[i]._mapcount, 0);
 		}
 		__mod_zone_page_state(page_zone(page), NR_ANON_PAGES, nr);
 	}
@@ -1103,10 +1111,19 @@ void page_add_new_anon_rmap(struct page *page,
 	VM_BUG_ON_VMA(address < vma->vm_start || address >= vma->vm_end, vma);
 	SetPageSwapBacked(page);
 	if (compound) {
+		int i;
+
 		VM_BUG_ON_PAGE(!PageTransHuge(page), page);
 		/* increment count (starts at -1) */
 		atomic_set(compound_mapcount_ptr(page), 0);
 		__inc_zone_page_state(page, NR_ANON_TRANSPARENT_HUGEPAGES);
+		/*
+		 * While compound_mapcount() is positive we keep *one* mapcount
+		 * reference in all subpages. It's required for atomic removal
+		 * from rmap.
+		 */
+		for (i = 0; i < nr; i++)
+			atomic_set(&page[i]._mapcount, 0);
 	} else {
 		/* Anon THP always mapped first with PMD */
 		VM_BUG_ON_PAGE(PageTransCompound(page), page);
@@ -1174,9 +1191,6 @@ out:
  */
 void page_remove_rmap(struct page *page, bool compound)
 {
-	int nr = compound ? hpage_nr_pages(page) : 1;
-	bool partial_thp_unmap;
-
 	if (!PageAnon(page)) {
 		VM_BUG_ON_PAGE(compound && !PageHuge(page), page);
 		page_remove_file_rmap(page);
@@ -1184,10 +1198,20 @@ void page_remove_rmap(struct page *page, bool compound)
 	}
 
 	/* page still mapped by someone else? */
-	if (!atomic_add_negative(-1, compound ?
-			       compound_mapcount_ptr(page) :
-			       &page->_mapcount))
+	if (compound) {
+		int i;
+
+		VM_BUG_ON_PAGE(!PageTransHuge(page), page);
+		if (!atomic_add_negative(-1, compound_mapcount_ptr(page)))
+			return;
+		__dec_zone_page_state(page, NR_ANON_TRANSPARENT_HUGEPAGES);
+		for (i = 0; i < hpage_nr_pages(page); i++)
+			page_remove_rmap(page + i, false);
 		return;
+	} else {
+		if (!atomic_add_negative(-1, &page->_mapcount))
+			return;
+	}
 
 	/* Hugepages are not counted in NR_ANON_PAGES for now. */
 	if (unlikely(PageHuge(page)))
@@ -1198,26 +1222,12 @@ void page_remove_rmap(struct page *page, bool compound)
 	 * these counters are not modified in interrupt context, and
 	 * pte lock(a spinlock) is held, which implies preemption disabled.
 	 */
-	if (compound) {
-		int i;
-		VM_BUG_ON_PAGE(!PageTransHuge(page), page);
-		__dec_zone_page_state(page, NR_ANON_TRANSPARENT_HUGEPAGES);
-		/* The page can be mapped with ptes */
-		for (i = 0; i < hpage_nr_pages(page); i++)
-			if (page_mapcount(page + i))
-				nr--;
-		partial_thp_unmap = nr != hpage_nr_pages(page);
-	} else if (PageTransCompound(page)) {
-		partial_thp_unmap = !compound_mapcount(page);
-	} else
-		partial_thp_unmap = false;
-
-	__mod_zone_page_state(page_zone(page), NR_ANON_PAGES, -nr);
+	__dec_zone_page_state(page, NR_ANON_PAGES);
 
 	if (unlikely(PageMlocked(page)))
 		clear_page_mlock(page);
 
-	if (partial_thp_unmap)
+	if (PageTransCompound(page))
 		deferred_split_huge_page(compound_head(page));
 
 	/*
-- 
 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>