KSM fixes

Hello,

With a stress test leveraging Hugh's allksm old patch, I found a random memory corruption bug (modify and user after free). It's firing oopses and generic instability so it's not just theoretical, but it can only be reproduced in practice with frequent exits and KSM at 100% CPU load. Note that it's common to give an entire core to KSM when the system is low on memory in production, http://www.ovirt.org/MoM is likely to do that as well. The allksm hack (not in this patchset) only facilitates at creating a workload that exit frequently to increase the frequency of the race window, without having to patch and rebuild binaries to call the MADV_MERGEABLE by hand.

I also did some other orthogonal optimization and cleanup.

I'm sending those upstream standalone, separately from some more complex larger and orthogonal pending changes I'm currently working on. Those will take more time to review and this fix is higher priority.

I haven't added -stable but at least 1/6 is definitely a candidate for stable, IMHO 2/6 would be good idea too. If this passes review and the fix is confirmed, I can resubmit at least 1/6 to stable. Comments?

Thanks,
Andrea

Andrea Arcangeli (6):
ksm: fix rmap_item->anon_vma memory corruption and vma user after free
ksm: add cond_resched() to the rmap_walks
ksm: don't fail stable tree lookups if walking over stale stable_nodes
ksm: use the helper method to do the hlist_empty check
ksm: use find_mergeable_vma in try_to_merge_with_ksm_page
ksm: unstable_tree_search_insert error checking cleanup

 mm/ksm.c  | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 mm/rmap.c |  4 +++
 2 files changed, 88 insertions(+), 13 deletions(-)

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

The ksm_test_exit() run after down_read(&mm->mmap_sem) assumed it could serialize against ksm_exit() and prevent exit_mmap() to run until the up_read(&mm->mmap_sem). That is true when the rmap_item->mm is the one associated with the ksm_scan.mm_slot, as ksm_exit() would take the !easy_to_free_path.

The problem is that when merging the current rmap_item (the one whose ->mm pointer is always associated ksm_scan.mm_slot) with a tree_rmap_item in the unstable tree, the unstable tree tree_rmap_item->mm can be any random mm. The locking technique described above is a noop if the rmap_item->mm is not the one associated with the ksm_scan.mm_slot. In turn the tree_rmap_item when converted to a stable tree rmap_item and added to the stable_node->hlist, can have a &rmap_item->anon_vma that points to already freed memory. The find_vma and other vma operations to reach the anon_vma also run on potentially already freed memory. The get_anon_vma atomic_inc itself could corrupt memory randomly in already re-used memory.

The result are oopses like below:

general protection fault: 0000 [#1] SMP
last sysfs file: /sys/kernel/mm/ksm/sleep_millisecs
CPU 14
Modules linked in: netconsole nfs nfs_acl auth_rpcgss fscache lockd sunrpc msr binfmt_misc sr_mod

Pid: 904, comm: ksmd Not tainted 2.6.32 #21 Supermicro X8DTN/X8DTN

RIP: 0010:[<ffffffff81138300>]  [<ffffffff81138300>] drop_anon_vma+0x0/0xe0
RSP: 0000:ffff88023b94bd28  EFLAGS: 00010206
RAX: 0000000000000000 RBX: ffff880231e64d50 RCX: 0000000000000017
RDX: ffff88023b94bfd8 RSI: 0000000000000216 RDI: 80000002192c6067
RBP: ffff88023b94bd40 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000001 R12: ffff880217fa4198
R13: ffff880217fa4198 R14: 000000000021916f R15: ffff880217fa419b
FS:  0000000000000000(0000) GS:ffff880030600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000d116a0 CR3: 000000023aa35000 CR4: 00000000000007e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process ksmd (pid: 904, threadinfo ffff88023b948000, task ffff88023b946500)
Stack:
 ffffffff8114d049 ffffea000757d048 ffffea0000000000 ffff88023b94bda0
<d> ffffffff8114d153 ffff88023b946500 ffff88023afaab40 ffffffff8114e85d
<d> 0100000000000038 ffff88023b94bdb0 ffff880231d33560 ffff880217fa4198
Call Trace:
 [<ffffffff8114d049>] ? remove_node_from_stable_tree+0x29/0x80
 [<ffffffff8114d153>] get_ksm_page+0xb3/0x1e0
 [<ffffffff8114e85d>] ? ksm_scan_thread+0x60d/0x1130
 [<ffffffff8114d499>] remove_rmap_item_from_tree+0x99/0x130
 [<ffffffff8114ed19>] ksm_scan_thread+0xac9/0x1130
 [<ffffffff81095ce0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff8114e250>] ? ksm_scan_thread+0x0/0x1130
 [<ffffffff8109508b>] kthread+0x8b/0xb0
 [<ffffffff8100c0ea>] child_rip+0xa/0x20
 [<ffffffff8100b910>] ? restore_args+0x0/0x30
 [<ffffffff81095000>] ? kthread+0x0/0xb0
 [<ffffffff8100c0e0>] ? child_rip+0x0/0x20
Code: 01 75 10 e8 d3 f7 ff ff 5d c3 90 0f 0b eb fe 0f 1f 40 00 e8 73 f6 ff ff 5d c3 e8 4c 77 01 00 5d c3 66 2e 0f 1f 84 00 00 00 00 00 <8b> 47 48 85 c0 0f 8e c5 00 00 00 55 48 89 e5 41 56 41 55 41 54
RIP  [<ffffffff81138300>] drop_anon_vma+0x0/0xe0
 RSP <ffff88023b94bd28>
---[ end trace b1f69fd4c12ce1ce ]---

rmap_item->anon_vma was set to the RDI value 0x80000002192c6067.

   0xffffffff81138300 <+0>:     mov    0x48(%rdi),%eax
   0xffffffff81138303 <+3>:     test   %eax,%eax
   0xffffffff81138305 <+5>:     jle    0xffffffff811383d0       <drop_anon_vma+208>
   0xffffffff8113830b <+11>:    push   %rbp

Other oopses are more random and harder to debug side effects of memory corruption. In this case the anon_vma was a dangling pointer because when try_to_merge_with_ksm_page did rmap_item->anon_vma = vma->anon_vma, the vma already was already freed and reused memory. At other times the oopses materialize with an vma->anon_vma pointer that looks legit but it points to an already freed and reused anon_vma.

---
 mm/ksm.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

index 7ee101e..8fc6793 100644
@@ -350,6 +350,24 @@ static inline bool ksm_test_exit(struct mm_struct *mm)
 }
 
 /*
+ * If the mm isn't the one associated with the current
+ * ksm_scan.mm_slot ksm_exit() will not down_write();up_write() and in
+ * turn the ksm_test_exit() check run inside a mm->mmap_sem critical
+ * section, will not prevent exit_mmap() to run from under us. In
+ * turn, in those cases where we could work with an "mm" that isn't
+ * guaranteed to be associated with the current ksm_scan.mm_slot,
+ * ksm_get_mm() is needed instead of the ksm_test_exit() run inside
+ * the mmap_sem. Return true if the mm_users was incremented or false
+ * if it we failed at taking the mm because it was freed from under
+ * us. If it returns 1, the caller must take care of calling mmput()
+ * after it finishes using the mm.
+ */
+static __always_inline bool ksm_get_mm(struct mm_struct *mm)
+{
+	return likely(atomic_inc_not_zero(&mm->mm_users));
+}
+
+/*
  * We use break_ksm to break COW on a ksm page: it's a stripped down
  *
  *	if (get_user_pages(current, mm, addr, 1, 1, 1, &page, NULL) == 1)

@@ -412,8 +430,6 @@ static struct vm_area_struct *find_mergeable_vma(struct mm_struct *mm,
 		unsigned long addr)
 {
 	struct vm_area_struct *vma;
-	if (ksm_test_exit(mm))
-		return NULL;
 	vma = find_vma(mm, addr);
 	if (!vma || vma->vm_start > addr)
 		return NULL;

@@ -434,11 +450,21 @@ static void break_cow(struct rmap_item *rmap_item)
 	 */
 	put_anon_vma(rmap_item->anon_vma);
 
+	/*
+	 * The "mm" of the unstable tree rmap_item isn't necessairly
+	 * associated with the current ksm_scan.mm_slot, it could be
+	 * any random mm. So we need ksm_get_mm here to prevent the
+	 * exit_mmap to run from under us in mmput().
+	 */
+	if (!ksm_get_mm(mm))
+		return;
+
 	down_read(&mm->mmap_sem);
 	vma = find_mergeable_vma(mm, addr);
 	if (vma)
 		break_ksm(vma, addr);
 	up_read(&mm->mmap_sem);
+	mmput(mm);
 }
 
 static struct page *page_trans_compound_anon(struct page *page)

@@ -462,6 +488,15 @@ static struct page *get_mergeable_page(struct rmap_item *rmap_item)
 	struct vm_area_struct *vma;
 	struct page *page;
 
+	/*
+	 * The "mm" of the unstable tree rmap_item isn't necessairly
+	 * associated with the current ksm_scan.mm_slot, it could be
+	 * any random mm. So we need ksm_get_mm here to prevent the
+	 * exit_mmap to run from under us in mmput().
+	 */
+	if (!ksm_get_mm(mm))
+		return NULL;
+
 	down_read(&mm->mmap_sem);
 	vma = find_mergeable_vma(mm, addr);
 	if (!vma)

@@ -478,6 +513,7 @@ static struct page *get_mergeable_page(struct rmap_item *rmap_item)
 out:		page = NULL;
 	}
 	up_read(&mm->mmap_sem);
+	mmput(mm);
 	return page;
 }
 

@@ -1086,9 +1122,19 @@ static int try_to_merge_with_ksm_page(struct rmap_item *rmap_item,
 	struct vm_area_struct *vma;
 	int err = -EFAULT;
 
+	/*
+	 * The "mm" of the unstable tree rmap_item isn't necessairly
+	 * associated with the current ksm_scan.mm_slot, it could be
+	 * any random mm. So we need ksm_get_mm() here to prevent the
+	 * exit_mmap to run from under us in mmput(). Otherwise
+	 * rmap_item->anon_vma could point to an anon_vma that has
+	 * already been freed (i.e. get_anon_vma() below would run too
+	 * late).
+	 */
+	if (!ksm_get_mm(mm))
+		return err;
+
 	down_read(&mm->mmap_sem);
-	if (ksm_test_exit(mm))
-		goto out;
 	vma = find_vma(mm, rmap_item->address);
 	if (!vma || vma->vm_start > rmap_item->address)
 		goto out;

@@ -1105,6 +1151,7 @@ static int try_to_merge_with_ksm_page(struct rmap_item *rmap_item,
 	get_anon_vma(vma->anon_vma);
 out:
 	up_read(&mm->mmap_sem);
+	mmput(mm);
 	return err;
 }
 

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

I was convinced for an hour, though puzzled how this had survived six years without being noticed: I'd certainly found the need for the special ksm_exit()/ksm_test_exit() stuff when testing originally, that wasn't hard, and why would this race be so very much harder?

Now, after looking again at ksm_exit(), I just don't see the point you're making. As I read it (and I certainly accept that I may be wrong on all this), it will do the down_write,up_write on any mm that is registered with it, and that has a chain of rmap_items - the easy_to_free route is only for those that have no rmap_items (and are not being worked on at present); and those that have no rmap_items, have no rmap_items in the unstable or the stable tree.

Please explain what I'm missing before I look again harder. One nit below. It looked very reasonable and nicely implemented to me, but I didn't complete checking it before I lost sight of what it's fixing. (And incrementing mm_users always makes me worry a bit about what happens under OOM, so I prefer not to do it.)

I've seen no problems at all in running rc5-mm1 and rc6-mm1 (with "allksm" of course) on this series.

Hugh

Time for an update, maybe :-? Though, in seriousness, I don't think we have fixed anything of this nature in KSM since then. Have probably fixed some anon_vma lifetime issues, though.

Hello,

I was traveling last few days but I could leave the testcase running to reproduce again by rolling back only this patch and I failed...

I now assume that it was another buggy patch that caused a corruption consistent with the ksm_exit not taking the mmap_sem for writing.

The patch that may have caused this (not part of this patchset) tried to synchronously drop the stable nodes, in order to remove the migrate_nodes loop in scan_get_next_rmap_item.

So the safety comes from relying on various implicit memory barriers that are taken as we change mm_slot during the scan, so that we know the mm_slot->rmap_list fields of the mm_slots not under scan, are stable and never zero if we run into a rmap_item that belongs to them.

Well the atomic_inc_not_zero is simpler and OOM shouldn't be a practical problem because this would do mmput immediately after (it's not holding it for long like the scan could do). However it adds an atomic op that the current logic doesn't require, and I wouldn't like to run an atomic op if there's no need.

So for the time being I agree that 1/6 is a noop and should be dropped. This only applies to 1/6.

Thanks and sorry for the confusion about the mm_slot->rmap_list.

Andrea

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

While at it add it to the file and anon walks too.

---
 mm/ksm.c  | 2 ++
 mm/rmap.c | 4 ++++
 2 files changed, 6 insertions(+)

index 8fc6793..39ef485 100644
@@ -1961,9 +1961,11 @@ again:
 		struct anon_vma_chain *vmac;
 		struct vm_area_struct *vma;
 
+		cond_resched();
 		anon_vma_lock_read(anon_vma);
 		anon_vma_interval_tree_foreach(vmac, &anon_vma->rb_root,
 					       0, ULONG_MAX) {
+			cond_resched();
 			vma = vmac->vma;
 			if (rmap_item->address < vma->vm_start ||
 			    rmap_item->address >= vma->vm_end)

index f5b5c1f..b949778 100644
@@ -1607,6 +1607,8 @@ static int rmap_walk_anon(struct page *page, struct rmap_walk_control *rwc)
 		struct vm_area_struct *vma = avc->vma;
 		unsigned long address = vma_address(page, vma);
 
+		cond_resched();
+
 		if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
 			continue;
 

@@ -1656,6 +1658,8 @@ static int rmap_walk_file(struct page *page, struct rmap_walk_control *rwc)
 	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
 		unsigned long address = vma_address(page, vma);
 
+		cond_resched();
+
 		if (rwc->invalid_vma && rwc->invalid_vma(vma, rwc->arg))
 			continue;
 

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

Subject really should be "mm: add cond_resched() to the rmap walks", then body "Add cond_resched() to the ksm and anon and file rmap walks."

but I think we need a blessing from Davidlohr too, if not more.

Perhaps I'm lost in the context, but by the changelog alone I cannot see the reasoning for the patch. Are latencies really that high? Maybe, at least the changelog needs some love.

Thanks,
Davidlohr

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

Yes they can be that high. The rmap walk must reach every possible mapping of the page, so if a page is heavily shared (no matter if it's KSM, anon, pagecache) there will be tons of entries to walk through. All optimizations with prio_tree, anon_vma chains, interval tree, helps to find the right virtual mapping faster, but if there are lots of virtual mappings, all mapping must still be walked through.

The biggest cost is for the IPIs and the IPIs can be optimized in a variety of ways, but at least for KSM if each virtual mapping ends up in a different mm and each mm runs in a different CPU and if there are tons of CPUs, it's actually impossible to reduce the number of IPIs during KSM page migration.

Regardless of the IPIs, it's generally safer to keep these cond_resched() in all cases, as even if we massively reduce the number of IPIs, the number of entries to walk IPI-less may still be large and no entry can be possibly skipped in the page migration case. Plus we leverage having made those locks sleepable.

Dropping 1/6 triggers a reject in a later patch, so I'll have to resubmit. So while at it, I'll add more commentary to the commit.

Thanks,
Andrea

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

The stable_nodes can become stale at any time if the underlying pages gets freed. The stable_node gets collected and removed from the stable rbtree if that is detected during the rbtree tree lookups.

Don't fail the lookup if running into stale stable_nodes, just restart the lookup after collecting the stale entries. Otherwise the CPU spent in the preparation stage is wasted and the lookup must be repeated at the next loop potentially failing a second time in a second stale entry.

This also will contribute to pruning the stable tree and releasing the stable_node memory more efficiently.

---
 mm/ksm.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

index 39ef485..929b5c2 100644
@@ -1225,7 +1225,18 @@ again:
 		stable_node = rb_entry(*new, struct stable_node, node);
 		tree_page = get_ksm_page(stable_node, false);
 		if (!tree_page)
-			return NULL;
+			/*
+			 * If we walked over a stale stable_node,
+			 * get_ksm_page() will call rb_erase() and it
+			 * may rebalance the tree from under us. So
+			 * restart the search from scratch. Returning
+			 * NULL would be safe too, but we'd generate
+			 * false negative insertions just because some
+			 * stable_node was stale which would waste CPU
+			 * by doing the preparation work twice at the
+			 * next KSM pass.
+			 */
+			goto again;
 
 		ret = memcmp_pages(page, tree_page);
 		put_page(tree_page);

@@ -1301,12 +1312,14 @@ static struct stable_node *stable_tree_insert(struct page *kpage)
 	unsigned long kpfn;
 	struct rb_root *root;
 	struct rb_node **new;
-	struct rb_node *parent = NULL;
+	struct rb_node *parent;
 	struct stable_node *stable_node;
 
 	kpfn = page_to_pfn(kpage);
 	nid = get_kpfn_nid(kpfn);
 	root = root_stable_tree + nid;
+again:
+	parent = NULL;
 	new = &root->rb_node;
 
 	while (*new) {

@@ -1317,7 +1330,18 @@ static struct stable_node *stable_tree_insert(struct page *kpage)
 		stable_node = rb_entry(*new, struct stable_node, node);
 		tree_page = get_ksm_page(stable_node, false);
 		if (!tree_page)
-			return NULL;
+			/*
+			 * If we walked over a stale stable_node,
+			 * get_ksm_page() will call rb_erase() and it
+			 * may rebalance the tree from under us. So
+			 * restart the search from scratch. Returning
+			 * NULL would be safe too, but we'd generate
+			 * false negative insertions just because some
+			 * stable_node was stale which would waste CPU
+			 * by doing the preparation work twice at the
+			 * next KSM pass.
+			 */
+			goto again;
 
 		ret = memcmp_pages(kpage, tree_page);
 		put_page(tree_page);

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

I'll say

as a gesture of goodwill, but in honesty I'm sitting on the fence, and couldn't decide. I think I've gone back and forth on this in my own mind in the past, worried that we might get stuck a long time going back round to "again". In the past I've felt that to give up with NULL is consistent with KSM's willingness to give way to any obstruction; but if you're finding "goto again" a better strategy, sure, go ahead. And at least there's a cond_resched() just above the diff context shown.

A dittoed nit below...

If a couple of large process exists and create lots of stale stable_nodes, we'll miss the opportunity to merge lots of unstable tree nodes for potentially many passes. So KSM gets an artificial latency in merging new unstable tree nodes. At the same time if we don't prune aggressively, we delay the freeing of the stale stable_nodes. Keeping stale stable_nodes around wastes memory and it can't provide any benefit.

Ideally we should be doing a full rbtree walk to do a perfect pruning after each pass to be sure some stale stable nodes don't stay around forever, but walking the whole rtbree takes more memory and it'd use more CPU. So considering we're not perfect at pruning the tree after each pass, we can at least be more aggressive at pruning the tree during the lookup.

Ok, I'll add braces, I don't mind either ways.

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

This just uses the helper function to cleanup the assumption on the hlist_node internals.

---
 mm/ksm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 929b5c2..241588e 100644
@@ -661,7 +661,7 @@ static void remove_rmap_item_from_tree(struct rmap_item *rmap_item)
 		unlock_page(page);
 		put_page(page);
 
-		if (stable_node->hlist.first)
+		if (!hlist_empty(&stable_node->hlist))
 			ksm_pages_sharing--;
 		else
 			ksm_pages_shared--;

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

Doing the VM_MERGEABLE check after the page == kpage check won't provide any meaningful benefit. The !vma->anon_vma check of find_mergeable_vma is the only superfluous bit in using find_mergeable_vma because the !PageAnon check of try_to_merge_one_page() implicitly checks for that, but it still looks cleaner to share the same find_mergeable_vma().

---
 mm/ksm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 241588e..10618a3 100644
@@ -1057,8 +1057,6 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 	if (page == kpage)			/* ksm page forked */
 		return 0;
 
-	if (!(vma->vm_flags & VM_MERGEABLE))
-		goto out;
 	if (PageTransCompound(page) && page_trans_compound_anon_split(page))
 		goto out;
 	BUG_ON(PageTransCompound(page));

@@ -1135,8 +1133,8 @@ static int try_to_merge_with_ksm_page(struct rmap_item *rmap_item,
 		return err;
 
 	down_read(&mm->mmap_sem);
-	vma = find_vma(mm, rmap_item->address);
-	if (!vma || vma->vm_start > rmap_item->address)
+	vma = find_mergeable_vma(mm, rmap_item->address);
+	if (!vma)
 		goto out;
 
 	err = try_to_merge_one_page(vma, page, kpage);

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

This looks like a nice little cleanup; I'm not 100% sure of it, forked pages always awkward here; but you're clearly more in touch with this now than I am, and I've seen no problem from it, so let's go with this.

get_mergeable_page() can only return NULL (in case of errors) or the pinned mergeable page. It can't return an error different than NULL. This makes it more readable and less confusion in addition to optimizing the check.

---
 mm/ksm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 10618a3..dcefc37 100644
@@ -1409,7 +1409,7 @@ struct rmap_item *unstable_tree_search_insert(struct rmap_item *rmap_item,
 		cond_resched();
 		tree_rmap_item = rb_entry(*new, struct rmap_item, node);
 		tree_page = get_mergeable_page(tree_rmap_item);
-		if (IS_ERR_OR_NULL(tree_page))
+		if (!tree_page)
 			return NULL;
 
 		/*

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

I share your sentiment, prefer to avoid an unnecessary IS_ERR_OR_NULL. And you may be right that it's unnecessary; but that's far from clear to me, and you haven't changed the IS_ERR_OR_NULL after follow_page() in get_mergeable_page() where it originates, so I wonder if you just got confused on this.

Even if you have established that there's currently no way that follow_page(vma, addr, FOLL_GET) could return an -errno on a vma validated by find_mergeable_vma(), I think we'd still be better off to allow for some future -errno there; but I'd be happy for you to keep the change below, but also adjust get_mergeable_page() to convert an -errno immediately to NULL after follow_page().

So, I think this is gently Nacked in its present form, but a replacement eagerly Acked.

Hugh

The "out:" label is followed by page = NULL, so if follow_page returns an error, get_mergeable_page still cannot return an error.

If this wasn't the case, get_mergeable_page would return an uninitialized pointer if find_mergeable_vma would return NULL.

I guess the IS_ERR_OR_NULL that I removed, was the direct result of overlooking the location of the "out:".

If there was a return after the "out:" the readability would have been improved, but I haven't touched the code around the "out:". That's the way it was before. Now I'll add a return in this same patch after the "out:" before resubmitting without 1/6.

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

Ah, yes, I missed that, thanks.

I can't very well claim now that it was readable before, can I?-) We'll see; I wouldn't mind if you leave this patch as is.

Hugh

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