linux-kernel - Re: [PATCH] thp: tail page refcounting fix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 24 Aug 2011 02:27:17 +0200
From:	Andrea Arcangeli <aarcange@...hat.com>
To:	Michel Lespinasse <walken@...gle.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Hugh Dickins <hughd@...gle.com>,
	Minchan Kim <minchan.kim@...il.com>,
	Johannes Weiner <jweiner@...hat.com>,
	Rik van Riel <riel@...hat.com>, Mel Gorman <mgorman@...e.de>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Shaohua Li <shaohua.li@...el.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] thp: tail page refcounting fix

On Wed, Aug 24, 2011 at 02:09:14AM +0200, Andrea Arcangeli wrote:
> That's an optimization I can look into agreed. I guess I just added
> one line and not even think too much at optimizing this,
> split_huge_page isn't in a fast path.

So this would more or less be the optimization (untested):

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1169,8 +1169,8 @@ static void __split_huge_page_refcount(s
 		atomic_sub(page_mapcount(page_tail), &page->_count);
 		BUG_ON(atomic_read(&page->_count) <= 0);
 		BUG_ON(atomic_read(&page_tail->_count) != 0);
-		atomic_add(page_mapcount(page) + 1, &page_tail->_count);
-		atomic_add(page_mapcount(page_tail), &page_tail->_count);
+		atomic_add(page_mapcount(page) + page_mapcount(page_tail) + 1,
+			   &page_tail->_count);

 		/* after clearing PageTail the gup refcount can be released */
 		smp_mb();

This might also be possible but I'm scared by it because the value
would be set by the C language without locked op, and I wonder then
what happens with get_page_unless_zero runs. Now we relay on atomic
(without lock prefix) writes from C for all pagetable updates
already. So I guess this might actually work ok too in
practice. get_page_unless_zero not incrementing anything sounds
unlikely, and it's hard to see how it could increment zero or a random
value if the "long" write is done in a single asm insn (like we relay
in other places). But still the above is obviously safe, the below far
less obvious and generally one always is forced to use locked ops on
any region of memory concurrently modified to have a deterministic
result. And there's nothing anywhere doing atomic_set on page->_count
except at boot where there are no races before the pages are visible
to the buddy allocator. So for now I'll stick to the above version
unless somebody can guarantee the safety of the below (which I can't).
Comments welcome..

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1169,8 +1169,8 @@ static void __split_huge_page_refcount(s
 		atomic_sub(page_mapcount(page_tail), &page->_count);
 		BUG_ON(atomic_read(&page->_count) <= 0);
 		BUG_ON(atomic_read(&page_tail->_count) != 0);
-		atomic_add(page_mapcount(page) + 1, &page_tail->_count);
-		atomic_add(page_mapcount(page_tail), &page_tail->_count);
+		atomic_set(&page_tail->_count,
+			   page_mapcount(page) + page_mapcount(page_tail) + 1);

 		/* after clearing PageTail the gup refcount can be released */
 		smp_mb();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/