On Fri, Aug 05, 2011 at 06:21:51PM +0200, Andrea Arcangeli wrote:

On Fri, Aug 05, 2011 at 04:25:16PM +0100, Mel Gorman wrote:

quoted

quoted

+int move_huge_pmd(struct vm_area_struct *vma, struct vm_area_struct *new_vma,
+		  unsigned long old_addr,
+		  unsigned long new_addr, unsigned long old_end,
+		  pmd_t *old_pmd, pmd_t *new_pmd)
+{
+	int ret = 0;
+	pmd_t pmd;
+
+	struct mm_struct *mm = vma->vm_mm;
+
+	if ((old_addr & ~HPAGE_PMD_MASK) ||
+	    (new_addr & ~HPAGE_PMD_MASK) ||

How could these conditions ever be true? We are here because it was
pmd_trans_huge. There should be no way this can be aligned. If this
is paranoia, make it a BUG_ON.

It actually happens. old_addr/new_addr aren't aligned to hpage
beforehand, and they just from the mremap syscall
parameters. Returning 0 is needed here.

Ah yes, of course. Just because the PMD is huge does not mean you
are calling mremap on an aligned address.

quoted

quoted

+	    (old_addr + HPAGE_PMD_SIZE) > old_end ||

Again, is this possible? The old addr was already huge.

This can happen too, old_end is also passed as parameter from syscall
and it's not mangled to fit an hpage, just calculated as old_addr+len
(len passed as parameter). The length of the destination vma shouldn't
be necessary to check, there's no new_end in the first place as
parameter of move_page_tables so the caller must ensure there's enough
space to copy in the destination. And if the old_end-old_addr <=
HPAGE_PMD_SIZE and the old_addr and new_addr are aligned, we're sure
the hugepmd is safe to create on the destination new_addr (if it's
aligned).

So I think all checks are needed here.

Yes, blindingly obvious now of course :/

In theory the caller could
check this stuff before calling move_huge_pmd but I try to be as less
invasive as possible in the common code that may be built with
TRANSPARENT_HUGEPAGE=n (these checks would still be computed in the
pmd_trans_huge(*old_pmd) case only but it's kind of nicer to hide them
in huge_memory.c).

What you have at the moment is indeed nicer.

quoted

quoted

+	/*
+	 * The destination pmd shouldn't be established, free_pgtables()
+	 * should have release it.
+	 */
+	if (!pmd_none(*new_pmd)) {
+		WARN_ON(1);
+		VM_BUG_ON(pmd_trans_huge(*new_pmd));
+		goto out;
+	}
+

Agreed that this should never happen. The mmap_sem is held for writing
and we are remapping to what should be empty space. It should not be
possible for a huge PMD to be established underneath us.

Yes. The code will work fine also if the new_pmd points to a pte that
wasn't freed by free_pgtables

As we hold mmap_sem for write, I would expect any munmap to have
completed by the time this is reached and we're hardly calling mremap
during exit.

but I think it's kind of safer to have a
WARN_ON because if that really ever happens we would prefer to
release the pte here and proceed mapping the hugepmd instead of
splitting the source hugepmd.

It's healthy paranoia.

If there's a hugepmd mapped instead it means the memory wasn't
munmapped. I could have run a mem compare of the pte against zero page
too but I didn't.. kind of overkill. But checking that there is no
hugepmd is fast.

I think the check is paranoid but there is no harm in that.

quoted

quoted

+	spin_lock(&mm->page_table_lock);
+	if (likely(pmd_trans_huge(*old_pmd))) {
+		if (pmd_trans_splitting(*old_pmd)) {
+			spin_unlock(&mm->page_table_lock);
+			wait_split_huge_page(vma->anon_vma, old_pmd);
+			ret = -1;
+		} else {
+			pmd = pmdp_get_and_clear(mm, old_addr, old_pmd);
+			VM_BUG_ON(!pmd_none(*new_pmd));
+			set_pmd_at(mm, new_addr, new_pmd, pmd);
+			spin_unlock(&mm->page_table_lock);
+			ret = 1;
+		}
+	} else
+		spin_unlock(&mm->page_table_lock);
+

The meaning of the return values of -1, 0, 1 with the caller doing

if (err)
...
else if (!err)
	...

is tricky to work out. split_huge_page only needs to be called if
returning 0. Would it be possible to have the split_huge_page called in
this function? The end of the function would then look like

return ret;

out_split:
split_huge_page_pmd()
return ret;

with either success or failure being returned instead of a tristate
which is easier to understand.

So basically always return 0 regardless if it was a
pmd_trans_splitting or if we splitted it ourself. And only return 1 in
case the move_huge_pmd was successful.

Exactly.

I'm afraid we've other trestates returned for the other mm
methods... if we cleanup this one later we may also cleanup the
others.

There are other tristates but lets not make life hard. When I've needed
tristates in recent patches, I've used enums with symbolic names instead
of -1, 0, 1 but that would be overkill here.

I'll try to clean up this one for now ok.

quoted

quoted

diff --git a/mm/mremap.c b/mm/mremap.c
--- a/mm/mremap.c
+++ b/mm/mremap.c

@@ -41,8 +41,7 @@ static pmd_t *get_old_pmd(struct mm_stru
 		return NULL;
 
 	pmd = pmd_offset(pud, addr);
-	split_huge_page_pmd(mm, pmd);
-	if (pmd_none_or_clear_bad(pmd))
+	if (pmd_none(*pmd))
 		return NULL;
 

Ok, this is changing to pmd_none because it could be a huge PMD and
pmd_none_or_clear_bad triggers on a huge PMD. Right?

Right. It'd bug on with a hugepmd. Maybe we could someday change
pmd_none_or_clear_bad not to choke on the PSE bit, but it was kind of
safer to keep assuming the PSE bit was "bad" in case we forgotten some
split_huge_page somewhere, better to bug in that case than to
ignore.

Agreed.

But as time passes and THP is rock solid, I've to admit it's
becoming more a lack of reliability to consider the PSE bit "bad" and
to remove some pmd_none_clear_bad than an increase of reliability to
validate the THP case. Maybe we should change that. It's a common
problem not specific to mremap.

It's something worth doing in a few releases time. It would be nice
to have the bad PMD checks back at some point.

quoted

quoted

@@ -65,8 +64,6 @@ static pmd_t *alloc_new_pmd(struct mm_st
 		return NULL;
 
 	VM_BUG_ON(pmd_trans_huge(*pmd));
-	if (pmd_none(*pmd) && __pte_alloc(mm, vma, pmd, addr))
-		return NULL;
 
 	return pmd;
 }

@@ -80,11 +77,7 @@ static void move_ptes(struct vm_area_str
 	struct mm_struct *mm = vma->vm_mm;
 	pte_t *old_pte, *new_pte, pte;
 	spinlock_t *old_ptl, *new_ptl;
-	unsigned long old_start;
 
-	old_start = old_addr;
-	mmu_notifier_invalidate_range_start(vma->vm_mm,
-					    old_start, old_end);

The MMU notifier is now being called for a larger range. Previously it
would usually be ranges of 64 pages and now it looks like it happens
once for the entire range being remapped. This is not mentioned in
the leader. What are the consequences of having a large gap between
invalidate_start and invalidate_end? Would it be a big deal to call
the MMU notifier within move_huge_pmd()?

Well it should improve performance. The only downside is that it will
stall secondary page faults for a longer time, but because the mremap
can now run faster with fewer IPIs, I think overall it should improve
performance.

That's a tough call. Increased jitter within KVM might be unwelcome
but ordinarily the only people that might care about latencies are
real time people and I dont think they would care about the KVM case.

Also it's probably not too common to do much mremap on
apps with a secondary MMU attached so in this place the mmu notifier
is more about correctness I think and correctness remains. Userland
can always do mremap in smaller chunks if it needs and then it'll
really run faster with no downside. After all no app is supposed to
touch the source addresses while they're being migrated as it could
SEGFAULT at any time. So a very long mremap basically makes a mapping
"not available" for a longer time, just now it'll be shorter because
mremap will run faster. The mapping being available for the secondary
mmu was incidental implementation detail, now it's not available to
the secondary mmu during the move, like it is not available to
userland. Trapping sigsegv to modify the mapping while it's under
mremap I doubt anybody is depending on.

I agree with you that overall it's probably for the best but splitting
it out as a separate patch and cc'ing the KVM people would do no harm.
Is there any impact for things like xpmem that might be using MMU
notifiers in some creative manner?

quoted

If it's safe to use larger ranges, it would be preferable to see it
in a separate patch or at the very least explained in the changelog.

I can split it off to a separate patch. I knew I should have done that
in the first place and rightfully I got caught :).

:)

quoted

quoted

 	if (vma->vm_file) {
 		/*
 		 * Subtle point from Rajesh Venkatasubramanian: before

@@ -111,7 +104,7 @@ static void move_ptes(struct vm_area_str
 				   new_pte++, new_addr += PAGE_SIZE) {
 		if (pte_none(*old_pte))
 			continue;
-		pte = ptep_clear_flush(vma, old_addr, old_pte);
+		pte = ptep_get_and_clear(mm, old_addr, old_pte);

This looks like an unrelated optimisation. You hint at this in the
patch subject but it needs a separate patch or a better explanation in
the leader. If I'm reading this right, it looks like you are deferring
a TLB flush on a single page and calling one call later at the end of
move_page_tables. At a glance, that seems ok and would reduce IPIs
but I'm not thinking about it properly because I'm trying to think
about THP shenanigans :)

That's exactly what it does. Once I split it off you can concentrate
on the two parts separately. This is also the parts that requires
moving the mmu notifier outside along with the tlb flush outside.

The THP shenanigans don't require moving the mmu notifier outside.

Right. I did notice that it would be less churn if the optimisation
went in first but that was about it.

The one IPI per page is a major bottleneck for java, lack of hugepmd
migrate also major bottleneck, here we get both combined so we get 1
IPI for a ton of THP. The benchmark I run was single threaded on a 12
core system (and single threaded if scheduler is doing good won't
require any IPI), you can only imagine the boost it gets on heavily
multithreaded apps that requires flooding IPI on large SMP (I didn't
measure that as I was already happy with what I got single threaded :).

Yeah, I imagine it should also be a boost during GC if the JVM is
using mremap to migrate full pages from an old heap to a new one.

I don't feel very strongly about it. The change looks reasonable
and with a fresh head with the patch split out, it probably is more
readable.

quoted

quoted

@@ -150,6 +145,23 @@ unsigned long move_page_tables(struct vm
 		new_pmd = alloc_new_pmd(vma->vm_mm, vma, new_addr);
 		if (!new_pmd)
 			break;
+		if (pmd_trans_huge(*old_pmd)) {
+			int err = 0;
+			if (extent == HPAGE_PMD_SIZE)
+				err = move_huge_pmd(vma, new_vma, old_addr,
+						    new_addr, old_end,
+						    old_pmd, new_pmd);
+			if (err > 0) {
+				need_flush = true;
+				continue;
+			} else if (!err)
+				split_huge_page_pmd(vma->vm_mm, old_pmd);
+			VM_BUG_ON(pmd_trans_huge(*old_pmd));

This tristate is hard to parse but I mentioned this already.

Yep I'll try to make it 0/1.

quoted

Functionally, I can't see a major problem with the patch. The
minor problems are that I'd like to see that tristate replaced for
readability, the optimisation better explained or in a separate patch
and an explanation why the larger ranges for mmu_notifiers is not
a problem.

Thanks for the review, very helpful as usual, I'll try to submit a 3
patches version with the cleanups you suggested soon enough. Ideally
I'd like to replace the -1 with the above change that also should
guard against TASK_SIZE ending less than one pmd away from the end of
the address space if you like it.

Sounds good. Will review again when they come around.

-- 
Mel Gorman
SUSE Labs

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>