linux-kernel - Re: [RFC] mm: Don't use radix tree writeback tags for pages in swap cache

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] [day] [month] [year] [list]
Message-ID: <87shtudu3g.fsf@yhuang-mobile.sh.intel.com>
Date:   Wed, 24 Aug 2016 09:12:19 -0700
From:   "Huang\, Ying" <ying.huang@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, <tim.c.chen@...el.com>,
        <andi.kleen@...el.com>, <aaron.lu@...el.com>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, Hugh Dickins <hughd@...gle.com>,
        Shaohua Li <shli@...nel.org>, Minchan Kim <minchan@...nel.org>,
        Rik van Riel <riel@...hat.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Tejun Heo <tj@...nel.org>,
        Wu Fengguang <fengguang.wu@...el.com>,
        "Huang\, Ying" <ying.huang@...el.com>
Subject: Re: [RFC] mm: Don't use radix tree writeback tags for pages in swap cache

"Huang, Ying" <ying.huang@...el.com> writes:

> Hi, Dave,
>
> Dave Hansen <dave.hansen@...el.com> writes:
>
>> On 08/09/2016 09:17 AM, Huang, Ying wrote:
>>> File pages uses a set of radix tags (DIRTY, TOWRITE, WRITEBACK) to
>>> accelerate finding the pages with the specific tag in the the radix tree
>>> during writing back an inode.  But for anonymous pages in swap cache,
>>> there are no inode based writeback.  So there is no need to find the
>>> pages with some writeback tags in the radix tree.  It is no necessary to
>>> touch radix tree writeback tags for pages in swap cache.
>>
>> Seems simple enough.  Do we do any of this unnecessary work for the
>> other radix tree tags?  If so, maybe we should just fix this once and
>> for all.  Could we, for instance, WARN_ONCE() in radix_tree_tag_set() if
>> it sees a swap mapping get handed in there?
>
> Good idea!  I will do that and try to catch other places if any.

I tested all (18) anonymous pages related test cases in vm-scalability
with a debug patch to WARN_ONCE for all swap mapping tag operations.
There are no other tag operations for swap mapping caught.  Below is the
patch I used for debugging.

Best Regards,
Huang, Ying

----------------------------------------->
    dbg: find all tag operations for swap cache

diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
index 4c45105..9a239ec 100644
--- a/include/linux/radix-tree.h
+++ b/include/linux/radix-tree.h
@@ -106,16 +106,24 @@ struct radix_tree_node {
 
 /* root tags are stored in gfp_mask, shifted by __GFP_BITS_SHIFT */
 struct radix_tree_root {
+	bool			swap;
 	gfp_t			gfp_mask;
 	struct radix_tree_node	__rcu *rnode;
 };
 
 #define RADIX_TREE_INIT(mask)	{					\
+	.swap = false,							\
 	.gfp_mask = (mask),						\
 	.rnode = NULL,							\
 }
 
-#define RADIX_TREE(name, mask) \
+#define RADIX_TREE_INIT_SWAP(mask)	{				\
+	.swap = true,							\
+	.gfp_mask = (mask),						\
+	.rnode = NULL,							\
+}
+
+#define RADIX_TREE(name, mask)					\
 	struct radix_tree_root name = RADIX_TREE_INIT(mask)
 
 #define INIT_RADIX_TREE(root, mask)					\
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 1b7bf73..51677bf 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -765,6 +765,8 @@ void *radix_tree_tag_set(struct radix_tree_root *root,
 	struct radix_tree_node *node, *parent;
 	unsigned long maxindex;
 
+	WARN_ON_ONCE(root->swap);
+
 	radix_tree_load_root(root, &node, &maxindex);
 	BUG_ON(index > maxindex);
 
@@ -828,6 +830,8 @@ void *radix_tree_tag_clear(struct radix_tree_root *root,
 	unsigned long maxindex;
 	int uninitialized_var(offset);
 
+	WARN_ON_ONCE(root->swap);
+
 	radix_tree_load_root(root, &node, &maxindex);
 	if (index > maxindex)
 		return NULL;
@@ -867,6 +871,8 @@ int radix_tree_tag_get(struct radix_tree_root *root,
 	struct radix_tree_node *node, *parent;
 	unsigned long maxindex;
 
+	WARN_ON_ONCE(root->swap);
+
 	if (!root_tag_get(root, tag))
 		return 0;
 
@@ -1050,6 +1056,8 @@ unsigned long radix_tree_range_tag_if_tagged(struct radix_tree_root *root,
 	unsigned long tagged = 0;
 	unsigned long index = *first_indexp;
 
+	WARN_ON_ONCE(root->swap);
+
 	radix_tree_load_root(root, &child, &maxindex);
 	last_index = min(last_index, maxindex);
 	if (index > last_index)
@@ -1240,6 +1248,8 @@ radix_tree_gang_lookup_tag(struct radix_tree_root *root, void **results,
 	void **slot;
 	unsigned int ret = 0;
 
+	WARN_ON_ONCE(root->swap);
+
 	if (unlikely(!max_items))
 		return 0;
 
@@ -1281,6 +1291,8 @@ radix_tree_gang_lookup_tag_slot(struct radix_tree_root *root, void ***results,
 	void **slot;
 	unsigned int ret = 0;
 
+	WARN_ON_ONCE(root->swap);
+
 	if (unlikely(!max_items))
 		return 0;
 
@@ -1590,6 +1602,8 @@ struct radix_tree_node *radix_tree_replace_clear_tags(
 	struct radix_tree_node *node;
 	void **slot;
 
+	WARN_ON_ONCE(root->swap);
+
 	__radix_tree_lookup(root, index, &node, &slot);
 
 	if (node) {
diff --git a/mm/swap_state.c b/mm/swap_state.c
index c8310a3..0059653 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -34,7 +34,7 @@ static const struct address_space_operations swap_aops = {
 
 struct address_space swapper_spaces[MAX_SWAPFILES] = {
 	[0 ... MAX_SWAPFILES - 1] = {
-		.page_tree	= RADIX_TREE_INIT(GFP_ATOMIC|__GFP_NOWARN),
+		.page_tree	= RADIX_TREE_INIT_SWAP(GFP_ATOMIC|__GFP_NOWARN),
 		.i_mmap_writable = ATOMIC_INIT(0),
 		.a_ops		= &swap_aops,
 	}