mm: preemptibility -v2

The powerpc page table freeing relies on the fact that IRQs hold off an RCU grace period, this is currently true for all existing RCU implementations but is not an assumption Paul wants to support.

Therefore, also take the RCU read lock along with disabling IRQs to ensure the RCU grace period does at least cover these lookups.

Requested-by Paul E. McKenney<[email protected]>

Signed-off-by Peter Zijlstra<[email protected]>

Cc Nick Piggin<[email protected]>

Cc Benjamin Herrenschmidt<[email protected]>

The powerpc page table freeing relies on the fact that IRQs hold off an RCU grace period, this is currently true for all existing RCU implementations but is not an assumption Paul wants to support.

Therefore, also take the RCU read lock along with disabling IRQs to ensure the RCU grace period does at least cover these lookups.

Requested-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

---
 arch/powerpc/mm/gup.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/arch/powerpc/mm/gup.c
===================================================================

arch/powerpc/mm/gup.c +3 -0

@@ -142,6 +142,7 @@ int get_user_pages_fast(unsigned long st
 	 * So long as we atomically load page table pointers versus teardown,
 	 * we can follow the address down to the the page and take a ref on it.
 	 */
+	rcu_read_lock();
 	local_irq_disable();
 
 	pgdp = pgd_offset(mm, addr);

@@ -162,6 +163,7 @@ int get_user_pages_fast(unsigned long st
 	} while (pgdp++, addr = next, addr != end);
 
 	local_irq_enable();
+	rcu_read_unlock();
 
 	VM_BUG_ON(nr != (end - start) >> PAGE_SHIFT);
 	return nr;

@@ -171,6 +173,7 @@ int get_user_pages_fast(unsigned long st
 
 slow:
 		local_irq_enable();
+		rcu_read_unlock();
 slow_irqon:
 		pr_devel("  slow path ! nr = %d\n", nr);
 

plain text document attachment (powerpc-gup_fast-rcu.patch) The powerpc page table freeing relies on the fact that IRQs hold off an RCU grace period, this is currently true for all existing RCU implementations but is not an assumption Paul wants to support.

Therefore, also take the RCU read lock along with disabling IRQs to ensure the RCU grace period does at least cover these lookups.

Requested-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

---
 arch/powerpc/mm/gup.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/arch/powerpc/mm/gup.c
===================================================================

arch/powerpc/mm/gup.c +3 -0

@@ -142,6 +142,7 @@ int get_user_pages_fast(unsigned long st
 	 * So long as we atomically load page table pointers versus teardown,
 	 * we can follow the address down to the the page and take a ref on it.
 	 */
+	rcu_read_lock();
 	local_irq_disable();
 
 	pgdp = pgd_offset(mm, addr);

@@ -162,6 +163,7 @@ int get_user_pages_fast(unsigned long st
 	} while (pgdp++, addr = next, addr != end);
 
 	local_irq_enable();
+	rcu_read_unlock();
 
 	VM_BUG_ON(nr != (end - start) >> PAGE_SHIFT);
 	return nr;

@@ -171,6 +173,7 @@ int get_user_pages_fast(unsigned long st
 
 slow:
 		local_irq_enable();
+		rcu_read_unlock();
 slow_irqon:
 		pr_devel("  slow path ! nr = %d\n", nr);
 

Quoted 7 lines · Peter Zijlstra

plain text document attachment (powerpc-gup_fast-rcu.patch) The powerpc page table freeing relies on the fact that IRQs hold off an RCU grace period, this is currently true for all existing RCU implementations but is not an assumption Paul wants to support.

Therefore, also take the RCU read lock along with disabling IRQs to ensure the RCU grace period does at least cover these lookups.

There's a few other places that need a similar fix then. The hash page code for example. All the C cases should end up calling the find_linux_pte() helper afaik, so we should be able to stick the lock in there (and the hugetlbfs variant, find_linux_pte_or_hugepte()).

However, we also have cases of tight asm code walking the page tables, such as the tlb miss handler on embedded processors. I don't see how I could do that there. IE. I only have a handful of registers to play with, no stack, etc...

So we might have to support the interrupt assumption, at least in some form, with those guys...

Cheers,
Ben.

Quoted 30 lines

Requested-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

---
 arch/powerpc/mm/gup.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/arch/powerpc/mm/gup.c
===================================================================

arch/powerpc/mm/gup.c +2 -0

@@ -142,6 +142,7 @@ int get_user_pages_fast(unsigned long st
 	 * So long as we atomically load page table pointers versus teardown,
 	 * we can follow the address down to the the page and take a ref on it.
 	 */
+	rcu_read_lock();
 	local_irq_disable();
 
 	pgdp = pgd_offset(mm, addr);

@@ -162,6 +163,7 @@ int get_user_pages_fast(unsigned long st
 	} while (pgdp++, addr = next, addr != end);
 
 	local_irq_enable();
+	rcu_read_unlock();
 
 	VM_BUG_ON(nr != (end - start) >> PAGE_SHIFT);
 	return nr;

arch/powerpc/mm/gup.c int get_user_pages_fast(unsigned long st · line 179

Quoted 7 lines

arch/powerpc/mm/gup.c +1 -0

@@ -171,6 +173,7 @@ int get_user_pages_fast(unsigned long st
 
 slow:
 		local_irq_enable();
+		rcu_read_unlock();
 slow_irqon:
 		pr_devel("  slow path ! nr = %d\n", nr);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines

So we might have to support the interrupt assumption, at least in some form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Quoted 5 lines · Paul E. McKenney

Quoted 2 lines

So we might have to support the interrupt assumption, at least in some form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Well, call_rcu_sched() then, because the current usage is to use call_rcu() to free the page directories.

Paul, here is a call_rcu_sched() available in kernel/rcutree.c, but am I right in reading that code that that would not be available for preemptible RCU?

Of course, with call_rcu_sched(), the corresponding RCU read-side critical
sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

Can the code in question accommodate these restrictions?

Quoted 6 lines · Paul E. McKenney

Of course, with call_rcu_sched(), the corresponding RCU read-side critical
sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

Quoted 1 line

Can the code in question accommodate these restrictions?

What we protect against is always code that hard-disable IRQs (though there seem to be a bug in the hugepages code there...). Would that work ?

So I believe that in this case call_rcu_sched() is your friend. ;-)

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

Quoted 2 lines

So we might have to support the interrupt assumption, at least in some form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Quoted 6 lines

Well, call_rcu_sched() then, because the current usage is to use call_rcu() to free the page directories.

Paul, here is a call_rcu_sched() available in kernel/rcutree.c, but am I right in reading that code that that would not be available for preemptible RCU?

Both call_rcu_sched() and call_rcu() are always there for you. ;-)

o If CONFIG_TREE_RCU (or CONFIG_TINY_RCU), they both have the same
implementation.

o If CONFIG_TREE_PREEMPT_RCU, call_rcu_sched() is preemptible and
call_rcu() is not.

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

Can the code in question accommodate these restrictions?

Quoted 17 lines · Paul E. McKenney

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

Quoted 2 lines

So we might have to support the interrupt assumption, at least in some form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Quoted 6 lines

Well, call_rcu_sched() then, because the current usage is to use call_rcu() to free the page directories.

Paul, here is a call_rcu_sched() available in kernel/rcutree.c, but am I right in reading that code that that would not be available for preemptible RCU?

Both call_rcu_sched() and call_rcu() are always there for you. ;-)

Quoted 5 lines

o If CONFIG_TREE_RCU (or CONFIG_TINY_RCU), they both have the same
implementation.

o If CONFIG_TREE_PREEMPT_RCU, call_rcu_sched() is preemptible and
call_rcu() is not.

(The reverse I suspect?)

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

Quoted 1 line

Can the code in question accommodate these restrictions?

Yes, that should do just fine I think.

Quoted 4 lines

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

Quoted 6 lines

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

What would rcu_read_lock_preempt() do in a !CONFIG_PREEMPT kernel?

Quoted 11 lines · Paul E. McKenney

Quoted 10 lines

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

Quoted 1 line

What would rcu_read_lock_preempt() do in a !CONFIG_PREEMPT kernel?

Same as for a preempt one, since you'd have to be able to schedule() while holding it to be able to do things like mutex_lock().

Quoted 15 lines · Peter Zijlstra

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

What would rcu_read_lock_preempt() do in a !CONFIG_PREEMPT kernel?

Quoted 2 lines

Same as for a preempt one, since you'd have to be able to schedule() while holding it to be able to do things like mutex_lock().

So what you really want is something like rcu_read_lock_sleep() rather than rcu_read_lock_preempt(), right? The point is that you want to do more than merely preempt, given that it is legal to do general blocking while holding a mutex, correct?

Quoted 19 lines · Paul E. McKenney

Quoted 17 lines · Peter Zijlstra

Quoted 4 lines · Paul E. McKenney

Quoted 4 lines

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

Quoted 8 lines

Quoted 6 lines

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

What would rcu_read_lock_preempt() do in a !CONFIG_PREEMPT kernel?

Same as for a preempt one, since you'd have to be able to schedule() while holding it to be able to do things like mutex_lock().

Quoted 4 lines

So what you really want is something like rcu_read_lock_sleep() rather than rcu_read_lock_preempt(), right? The point is that you want to do more than merely preempt, given that it is legal to do general blocking while holding a mutex, correct?

Right, but CONFIG_TREE_PREEMPT_RCU=y ends up being that. We could change the name to _sleep, but we've been calling it preemptible-rcu for a long while now.

Quoted 25 lines · Peter Zijlstra

Quoted 23 lines · Paul E. McKenney

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

Quoted 10 lines

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

Quoted 4 lines

Quoted 1 line

What would rcu_read_lock_preempt() do in a !CONFIG_PREEMPT kernel?

Same as for a preempt one, since you'd have to be able to schedule() while holding it to be able to do things like mutex_lock().

So what you really want is something like rcu_read_lock_sleep() rather than rcu_read_lock_preempt(), right? The point is that you want to do more than merely preempt, given that it is legal to do general blocking while holding a mutex, correct?

Quoted 3 lines

Right, but CONFIG_TREE_PREEMPT_RCU=y ends up being that. We could change the name to _sleep, but we've been calling it preemptible-rcu for a long while now.

It is actually not permitted to do general blocking in a preemptible RCU read-side critical section. Otherwise, someone is going to block waiting for a network packet that never comes, thus OOMing the system.

Quoted 30 lines · Paul E. McKenney

Quoted 28 lines · Peter Zijlstra

Quoted 16 lines · Paul E. McKenney

Quoted 15 lines · Peter Zijlstra

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines

Quoted 3 lines

Of course, with call_rcu_sched(), the corresponding RCU read-side critical sections are non-preemptible. Therefore, in CONFIG_PREEMPT_RT, these read-side critical sections must use raw spinlocks.

OK, so if we fully remove CONFIG_TREE_PREEMPT_RCU (defaulting to y), rename all the {call_rcu, rcu_read_lock, rcu_read_unlock, synchronize_rcu} functions to {*}_preempt and then add a new CONFIG_PREEMPT_RCU that simply maps {*} to either {*}_sched or {*}_preempt, we've basically got what I've been asking for for a while, no?

What would rcu_read_lock_preempt() do in a !CONFIG_PREEMPT kernel?

Quoted 7 lines

Quoted 2 lines

Same as for a preempt one, since you'd have to be able to schedule() while holding it to be able to do things like mutex_lock().

So what you really want is something like rcu_read_lock_sleep() rather than rcu_read_lock_preempt(), right? The point is that you want to do more than merely preempt, given that it is legal to do general blocking while holding a mutex, correct?

Right, but CONFIG_TREE_PREEMPT_RCU=y ends up being that. We could change the name to _sleep, but we've been calling it preemptible-rcu for a long while now.

Quoted 3 lines

It is actually not permitted to do general blocking in a preemptible RCU read-side critical section. Otherwise, someone is going to block waiting for a network packet that never comes, thus OOMing the system.

Sure, something that guarantees progress seems like a sensible restriction for any lock, and in particular RCU :-)

o mutex_lock(): Critical sections need not guarantee
forward progress, as general blocking is permitted.

So the easy response is "just use SRCU." Of course, SRCU has some disadvantages at the moment:

o The return value from srcu_read_lock() must be passed to
srcu_read_unlock(). I believe that I can fix this.

o There is no call_srcu(). I believe that I can fix this.

o SRCU uses a flat per-CPU counter scheme that is not particularly
scalable. I believe that I can fix this.

o SRCU's current implementation makes it almost impossible to
implement priority boosting. I believe that I can fix this.

o SRCU requires explicit initialization of the underlying
srcu_struct. Unfortunately, I don't see a reasonable way around this. Not yet, anyway.

So, is there anything else that you don't like about SRCU?

Quoted 3 lines · Paul E. McKenney

o mutex_lock(): Critical sections need not guarantee
forward progress, as general blocking is permitted.

Right, I would argue that they should guarantee fwd progress, but due to being able to schedule while holding them, its harder to enforce.

Anything that is waiting for uncertainty should do so without any locks held and simply re-acquire them once such an event does occur.

Quoted 18 lines

So the easy response is "just use SRCU." Of course, SRCU has some disadvantages at the moment:

o The return value from srcu_read_lock() must be passed to
srcu_read_unlock(). I believe that I can fix this.

o There is no call_srcu(). I believe that I can fix this.

o SRCU uses a flat per-CPU counter scheme that is not particularly
scalable. I believe that I can fix this.

o SRCU's current implementation makes it almost impossible to
implement priority boosting. I believe that I can fix this.

o SRCU requires explicit initialization of the underlying
srcu_struct. Unfortunately, I don't see a reasonable way around this. Not yet, anyway.

Quoted 1 line

So, is there anything else that you don't like about SRCU?

No, I quite like SRCU when implemented as preemptible tree RCU, and I don't at all mind that last point, all dynamic things need some sort of init. All locks certainly have.

o mutex_lock(): Critical sections need not guarantee
forward progress, as general blocking is permitted.

Quoted 2 lines · Paul E. McKenney

o mutex_lock(): Critical sections need not guarantee
forward progress, as general blocking is permitted.

This isn't quite right. mutex critical sections must guarantee eventual forward progress against the class of other potential acquirers of the mutex otherwise the system will become either deadlocked or livelocked.

Quoted 7 lines · James Bottomley

Quoted 2 lines · Paul E. McKenney

o mutex_lock(): Critical sections need not guarantee
forward progress, as general blocking is permitted.

This isn't quite right. mutex critical sections must guarantee eventual forward progress against the class of other potential acquirers of the mutex otherwise the system will become either deadlocked or livelocked.

If I understand you correctly, you are saying that it is OK for a given critical section for a given mutex to fail to make forward progress if nothing else happens to acquire that mutex during that time. I would agree, at least I would if you were to further add that the soft-lockup checks permit an additional 120 seconds of failure to make forward progress even if something -is- attempting to acquire that mutex.

By my standards, 120 seconds is a reasonable approximation to infinity, hence my statement above.

So, would you agree with the following as a more precise statement?

o mutex_lock(): Critical sections need not guarantee
forward progress unless some other task is waiting on the mutex in question, in which case critical sections should complete in 120 seconds.

Quoted 1 line

So we might have to support the interrupt assumption, at least in

some

Quoted 1 line

form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Quoted 6 lines · Paul E. McKenney

Quoted 1 line

So we might have to support the interrupt assumption, at least in

some

Quoted 1 line

form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Ok, so I'm a bit of a RCU newbie as you may know :-) Right now, we use neither, we use call_rcu and we free the pages from the callback.

Quoted 8 lines · Benjamin Herrenschmidt

Quoted 6 lines · Paul E. McKenney

Quoted 1 line

So we might have to support the interrupt assumption, at least in

some

Quoted 1 line

form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Quoted 2 lines

Ok, so I'm a bit of a RCU newbie as you may know :-) Right now, we use neither, we use call_rcu and we free the pages from the callback.

BTW. you currently have an interesting page table freeing path where you usually free by RCU, but (occasionally) free by IPI. This means you need to disable both RCU and interrupts to walk page tables.

If you change it to always use RCU, then you wouldn't need to disable interrupts. Whether this actually matters anywhere in your mm code, I don't know (it's probably not terribly important for gup_fast). But rcu disable is always preferable for latency and performance.

--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 12 lines · Nick Piggin

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 6 lines · Paul E. McKenney

Quoted 1 line

So we might have to support the interrupt assumption, at least in

some

Quoted 1 line

form, with those guys...

One way to make the interrupt assumption official is to use synchronize_sched() rather than synchronize_rcu().

Ok, so I'm a bit of a RCU newbie as you may know :-) Right now, we use neither, we use call_rcu and we free the pages from the callback.

Quoted 3 lines

BTW. you currently have an interesting page table freeing path where you usually free by RCU, but (occasionally) free by IPI. This means you need to disable both RCU and interrupts to walk page tables.

Well, the point is we use interrupts to synchronize. The fact that RCU used to do the job was an added benefit. I may need to switch to rcu _sched variants tho to keep that. The IPI case is a slow path in case we are out of memory and cannot allocate our page of RCU batch.

Quoted 4 lines

If you change it to always use RCU, then you wouldn't need to disable interrupts. Whether this actually matters anywhere in your mm code, I don't know (it's probably not terribly important for gup_fast). But rcu disable is always preferable for latency and performance.

Well, the main case is the hash miss and that always runs with IRQs off.

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Signed-off-by Peter Zijlstra<[email protected]>

Cc Hugh Dickins<[email protected]>

Cc Linus Torvalds<[email protected]>

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock.

Most (all?) current users end up
calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Signed-off-by Peter Zijlstra <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 mm/rmap.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6/mm/rmap.c
===================================================================

mm/rmap.c +1 -0

@@ -294,6 +294,7 @@ struct anon_vma *page_lock_anon_vma(stru
 	unsigned long anon_mapping;
 
 	rcu_read_lock();
+again:
 	anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
 	if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		goto out;

@@ -302,6 +303,12 @@ struct anon_vma *page_lock_anon_vma(stru
 
 	anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);

Quoted 2 lines · On Thu, 08 Apr 2010 21:17:39 +0200 Peter Zijlstra <[email protected]>…

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock.

Well. The comment there clearly implies (or states) that RCU protection is used to "guard against races". If that's inaccurate or incomplete, can we please get it fixed?

The whole function makes be a bit queasy.

- Fails to explain why it pulls all these party tricks to read page->mapping a single time. What code path are we defending against here?

- Then checks page_mapped() without having any apparent defence against page_mapped() becoming untrue one nanosecond later.

- Checks page_mapped() inside the rcu_read_locked() section for inscrutable reasons.

Quoted 5 lines

- Then checks page_mapped() without having any apparent defence against page_mapped() becoming untrue one nanosecond later.

- Checks page_mapped() inside the rcu_read_locked() section for inscrutable reasons.

Right, I think the page_mapped() stuff is just an early bail out.

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Signed-off-by Peter Zijlstra <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 mm/rmap.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6/mm/rmap.c
===================================================================

mm/rmap.c +1 -0

@@ -294,6 +294,7 @@ struct anon_vma *page_lock_anon_vma(stru
 	unsigned long anon_mapping;
 
 	rcu_read_lock();
+again:
 	anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
 	if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		goto out;

mm/rmap.c +4 -0

+		spin_unlock(&anon_vma->lock);
+		goto again;
+	}
+
 	return anon_vma;
 out:
 	rcu_read_unlock();

Quoted 11 lines · Peter Zijlstra

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Quoted 5 lines

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Hm, looks like a bugfix? How was this supposed to be safe?

Quoted 22 lines

Signed-off-by Peter Zijlstra <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 mm/rmap.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6/mm/rmap.c
===================================================================

mm/rmap.c +1 -0

@@ -294,6 +294,7 @@ struct anon_vma *page_lock_anon_vma(stru
 	unsigned long anon_mapping;
 
 	rcu_read_lock();
+again:
 	anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
 	if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		goto out;

@@ -302,6 +303,12 @@ struct anon_vma *page_lock_anon_vma(stru
 

mm/rmap.c struct anon_vma *page_lock_anon_vma(stru · line 307

Quoted 4 lines

mm/rmap.c +2 -0

 	anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
 	spin_lock(&anon_vma->lock);
+
+	if (page_rmapping(page) != anon_vma) {

very unlikely()?

Quoted 18 lines · On Fri, 9 Apr 2010 13:16:41 +1000 Nick Piggin <[email protected]> wrote:

Quoted 16 lines · Peter Zijlstra

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Quoted 2 lines

Hm, looks like a bugfix? How was this supposed to be safe?

IIUC.

Before Rik's change to anon_vma, once page->mapping is set as anon_vma | 0x1, it's not modified until the page is freed.
After the patch, do_wp_page() overwrite page->mapping when it reuse existing page.

==
static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
unsigned long address, pte_t *page_table, pmd_t *pmd, spinlock_t *ptl, pte_t orig_pte)
{
....

        if (PageAnon(old_page) && !PageKsm(old_page)) {
                if (!trylock_page(old_page)) {
                        page_cache_get(old_page);
....
                reuse = reuse_swap_page(old_page);
                if (reuse)
                        /*
                         * The page is all ours.  Move it to our anon_vma so
                         * the rmap code will not search our parent or siblings.
                         * Protected against the rmap code by the page lock.
                         */
                        page_move_anon_rmap(old_page, vma, address); ----(*)
}

===
(*) is new.

Then, this new check makes sense in the current kernel.

Quoted 21 lines

Quoted 18 lines · On Fri, 9 Apr 2010 13:16:41 +1000 Nick Piggin <[email protected]> wrote:

Quoted 16 lines · Peter Zijlstra

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Quoted 8 lines

Quoted 2 lines

Hm, looks like a bugfix? How was this supposed to be safe?

IIUC.

Before Rik's change to anon_vma, once page->mapping is set as anon_vma | 0x1, it's not modified until the page is freed.
After the patch, do_wp_page() overwrite page->mapping when it reuse existing page.

Why?
IIUC. page->mapping dereference in page_lock_anon_vma() makes four story.

1. the anon_vma is valid

	-> do page_referenced_one(). 
2. the anon_vma is invalid and freed to buddy
	-> bail out by page_mapped(), no touch anon_vma
3. the anon_vma is kfreed, and not reused
	-> bail out by page_mapped()
4. the anon_vma is kfreed, but reused as another anon_vma
	-> bail out by page_check_address()

Now we have to consider 5th story.

5. the anon_vma is exchanged another anon_vma by do_wp_page.

	-> bail out by above bailing out stuff.

I agree peter's patch makes sense. but I don't think Rik's patch change locking rule.

Quoted 42 lines · On Fri, 9 Apr 2010 15:34:33 +0900 (JST) KOSAKI Motohiro…

Quoted 21 lines

Quoted 18 lines · On Fri, 9 Apr 2010 13:16:41 +1000 Nick Piggin <[email protected]> wrote:

Quoted 16 lines · Peter Zijlstra

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Quoted 8 lines

Quoted 2 lines

Hm, looks like a bugfix? How was this supposed to be safe?

IIUC.

Before Rik's change to anon_vma, once page->mapping is set as anon_vma | 0x1, it's not modified until the page is freed.
After the patch, do_wp_page() overwrite page->mapping when it reuse existing page.

Why?
IIUC. page->mapping dereference in page_lock_anon_vma() makes four story.

1. the anon_vma is valid

	-> do page_referenced_one(). 
2. the anon_vma is invalid and freed to buddy
	-> bail out by page_mapped(), no touch anon_vma
3. the anon_vma is kfreed, and not reused
	-> bail out by page_mapped()
4. the anon_vma is kfreed, but reused as another anon_vma
	-> bail out by page_check_address()

Quoted 9 lines

Now we have to consider 5th story.

5. the anon_vma is exchanged another anon_vma by do_wp_page.

	-> bail out by above bailing out stuff.

I agree peter's patch makes sense. but I don't think Rik's patch change locking rule.

Hmm, I think following.

Assume a page is ANON and SwapCache, and it has only one reference. Consider it's read-only mapped and cause do_wp_page(). page_mapcount(page) == 1 here.

CPU0 CPU1

1. do_wp_page()
2. .....
3. replace anon_vma. anon_vma = lock_page_anon_vma()

So, lock_page_anon_vma() may have lock on wrong anon_vma, here.(mapcount=1)

4. modify pte to writable. do something...

After lock, in CPU1, a pte of estimated address by vma_address(vma, page) containes pfn of the page and page_check_address() will success.

I'm not sure how this is dangerouns.
But it's possible that CPU1 cannot notice there was anon_vma replacement. And modifies pte withoug holding anon vma's lock which the code believes it's holded.

Quoted 71 lines

Quoted 42 lines · On Fri, 9 Apr 2010 15:34:33 +0900 (JST) KOSAKI Motohiro…

Quoted 21 lines

Quoted 18 lines · On Fri, 9 Apr 2010 13:16:41 +1000 Nick Piggin <[email protected]> wrote:

Quoted 16 lines · Peter Zijlstra

There is nothing preventing the anon_vma from being detached while we are spinning to acquire the lock. Most (all?) current users end up calling something like vma_address(page, vma) on it, which has a fairly good chance of weeding out wonky vmas.

However suppose the anon_vma got freed and re-used while we were waiting to acquire the lock, and the new anon_vma fits with the page->index (because that is the only thing vma_address() uses to determine if the page fits in a particular vma, we could end up traversing faulty anon_vma chains.

Close this hole for good by re-validating that page->mapping still holds the very same anon_vma pointer after we acquire the lock, if not be utterly paranoid and retry the whole operation (which will very likely bail, because it's unlikely the page got attached to a different anon_vma in the meantime).

Quoted 8 lines

Quoted 2 lines

Hm, looks like a bugfix? How was this supposed to be safe?

IIUC.

Before Rik's change to anon_vma, once page->mapping is set as anon_vma | 0x1, it's not modified until the page is freed.
After the patch, do_wp_page() overwrite page->mapping when it reuse existing page.

Why?
IIUC. page->mapping dereference in page_lock_anon_vma() makes four story.

1. the anon_vma is valid

	-> do page_referenced_one(). 
2. the anon_vma is invalid and freed to buddy
	-> bail out by page_mapped(), no touch anon_vma
3. the anon_vma is kfreed, and not reused
	-> bail out by page_mapped()
4. the anon_vma is kfreed, but reused as another anon_vma
	-> bail out by page_check_address()

Quoted 9 lines

Now we have to consider 5th story.

5. the anon_vma is exchanged another anon_vma by do_wp_page.

	-> bail out by above bailing out stuff.

I agree peter's patch makes sense. but I don't think Rik's patch change locking rule.

Hmm, I think following.

Assume a page is ANON and SwapCache, and it has only one reference. Consider it's read-only mapped and cause do_wp_page(). page_mapcount(page) == 1 here.

CPU0 CPU1

1. do_wp_page()
2. .....
3. replace anon_vma. anon_vma = lock_page_anon_vma()

So, lock_page_anon_vma() may have lock on wrong anon_vma, here.(mapcount=1)

4. modify pte to writable. do something...

Quoted 7 lines

After lock, in CPU1, a pte of estimated address by vma_address(vma, page) containes pfn of the page and page_check_address() will success.

I'm not sure how this is dangerouns.
But it's possible that CPU1 cannot notice there was anon_vma replacement. And modifies pte withoug holding anon vma's lock which the code believes it's holded.

Hehe, page_referenced() already can take unstable VM_LOCKED value. So, In worst case we make false positive pageout, but it's not disaster. I think. Anyway "use after free" don't happen by this blutal code.

However, I think you pointed one good thing. before Rik patch, we don't have page->mapping reassignment. then, we didn't need rcu_dereference(). but now it can happen. so, I think rcu_dereference() is better.

Perhaps, I'm missing something.

mm/rmap.c +1 -1

index 8b088f0..b4a0b5b 100644
@@ -295,7 +295,7 @@ struct anon_vma *page_lock_anon_vma(struct page *page)
 	unsigned long anon_mapping;
 
 	rcu_read_lock();
-	anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
+	anon_mapping = (unsigned long) rcu_dereference(page->mapping);
 	if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		goto out;
 	if (!page_mapped(page))

Quoted 29 lines · On Fri, 9 Apr 2010 16:29:59 +0900 (JST) KOSAKI Motohiro…

Quoted 16 lines

Hmm, I think following.

Assume a page is ANON and SwapCache, and it has only one reference. Consider it's read-only mapped and cause do_wp_page(). page_mapcount(page) == 1 here.

CPU0 CPU1

1. do_wp_page()
2. .....
3. replace anon_vma. anon_vma = lock_page_anon_vma()

So, lock_page_anon_vma() may have lock on wrong anon_vma, here.(mapcount=1)

4. modify pte to writable. do something...

Quoted 7 lines

After lock, in CPU1, a pte of estimated address by vma_address(vma, page) containes pfn of the page and page_check_address() will success.

I'm not sure how this is dangerouns.
But it's possible that CPU1 cannot notice there was anon_vma replacement. And modifies pte withoug holding anon vma's lock which the code believes it's holded.

Hehe, page_referenced() already can take unstable VM_LOCKED value. So, In worst case we make false positive pageout, but it's not disaster. I think. Anyway "use after free" don't happen by this blutal code.

Quoted 6 lines

However, I think you pointed one good thing. before Rik patch, we don't have page->mapping reassignment. then, we didn't need rcu_dereference(). but now it can happen. so, I think rcu_dereference() is better.

Perhaps, I'm missing something.

Hmm. I wonder we can check "whether we lock valid anon_vma or not" only under pte_lock or lock_page().
==

	anon_vma = page_anon_vma();
	lock(anon_vma->lock);
	....
	page_check_address(page)
		....
		pte_lock();
		if (page_anon_vma(page) == anon_vma)
			# anon_vma replacement happens!
	unlock(anon_vma->lock);

==
So, rather than page_lock_anon_vma(), page_check_address() may have to check anon_vma replacement....But I cannot think of dangerous case which can cause panic for now. I may miss something...

Quoted 51 lines · On Fri, 9 Apr 2010 16:57:03 +0900 KAMEZAWA Hiroyuki…

Quoted 29 lines · On Fri, 9 Apr 2010 16:29:59 +0900 (JST) KOSAKI Motohiro…

Quoted 16 lines

Hmm, I think following.

Assume a page is ANON and SwapCache, and it has only one reference. Consider it's read-only mapped and cause do_wp_page(). page_mapcount(page) == 1 here.

CPU0 CPU1

1. do_wp_page()
2. .....
3. replace anon_vma. anon_vma = lock_page_anon_vma()

So, lock_page_anon_vma() may have lock on wrong anon_vma, here.(mapcount=1)

4. modify pte to writable. do something...

Quoted 7 lines

After lock, in CPU1, a pte of estimated address by vma_address(vma, page) containes pfn of the page and page_check_address() will success.

I'm not sure how this is dangerouns.
But it's possible that CPU1 cannot notice there was anon_vma replacement. And modifies pte withoug holding anon vma's lock which the code believes it's holded.

Hehe, page_referenced() already can take unstable VM_LOCKED value. So, In worst case we make false positive pageout, but it's not disaster. I think. Anyway "use after free" don't happen by this blutal code.

Quoted 6 lines

However, I think you pointed one good thing. before Rik patch, we don't have page->mapping reassignment. then, we didn't need rcu_dereference(). but now it can happen. so, I think rcu_dereference() is better.

Perhaps, I'm missing something.

Hmm. I wonder we can check "whether we lock valid anon_vma or not" only under pte_lock or lock_page().
==

	anon_vma = page_anon_vma();
	lock(anon_vma->lock);
	....
	page_check_address(page)
		....
		pte_lock();
		if (page_anon_vma(page) == anon_vma)
			# anon_vma replacement happens!
	unlock(anon_vma->lock);

Quoted 5 lines

==
So, rather than page_lock_anon_vma(), page_check_address() may have to check anon_vma replacement....But I cannot think of dangerous case which can cause panic for now. I may miss something...

Ah...anon_vma replacemet occurs under lock_page() and pte_lock. Almost all callers of page_lock_anon_vma() holds lock_page(). So, I think this anon_vma replacement is not very serious.

Quoted 19 lines

Hmm, I think following.

Assume a page is ANON and SwapCache, and it has only one reference. Consider it's read-only mapped and cause do_wp_page(). page_mapcount(page) == 1 here.

    CPU0                          CPU1

1. do_wp_page()
2. .....
3. replace anon_vma.     anon_vma = lock_page_anon_vma()

So, lock_page_anon_vma() may have lock on wrong anon_vma, here.(mapcount=1)

4. modify pte to writable.        do something...

After lock, in CPU1, a pte of estimated address by vma_address(vma, page) containes pfn of the page and page_check_address() will success.

Quoted 4 lines

I'm not sure how this is dangerouns.
But it's possible that CPU1 cannot notice there was anon_vma replacement. And modifies pte withoug holding anon vma's lock which the code believes it's holded.

Hehe, page_referenced() already can take unstable VM_LOCKED value. So, In worst case we make false positive pageout, but it's not disaster.

OFF-TOPIC:

I think you pointed out good thing, too. :)

You mean although application call mlock of any vma, few pages on the vma can be swapout by race between mlock and reclaim?

Although it's not disaster, apparently it breaks API.
Man page
" mlock() and munlock()
mlock() locks pages in the address range starting at addr and
continuing for len bytes. All pages that contain a part of the specified address range are guaranteed to be resident in RAM when the call returns successfully; the pages are guaranteed to stay in RAM until later unlocked."

Do you have a plan to solve such problem?

And how about adding simple comment about that race in page_referenced_one? Could you send the patch?

Hi Minchan,

Quoted 16 lines

OFF-TOPIC:

I think you pointed out good thing, too. :)

You mean although application call mlock of any vma, few pages on the vma can be swapout by race between mlock and reclaim?

Although it's not disaster, apparently it breaks API.
Man page
" mlock() and munlock()
mlock() locks pages in the address range starting at addr and
continuing for len bytes. All pages that contain a part of the specified address range are guaranteed to be resident in RAM when the call returns successfully; the pages are guaranteed to stay in RAM until later unlocked."

Quoted 4 lines

Do you have a plan to solve such problem?

And how about adding simple comment about that race in page_referenced_one? Could you send the patch?

I'm surprising this mail. you were pushing much patch in this area. I believed you know all stuff ;)

My answer is, it don't need to fix, because it's not bug. The point is that this one is race issue. not "pageout after mlock" issue. If pageout and mlock occur at the exactly same time, the human can't observe which event occur in first. it's not API violation.

mm/rmap.c +1 -1

index 8b088f0..b4a0b5b 100644
@@ -295,7 +295,7 @@ struct anon_vma *page_lock_anon_vma(struct page
 *page)
        unsigned long anon_mapping;
 
        rcu_read_lock();
-       anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
+       anon_mapping = (unsigned long) rcu_dereference(page->mapping);
        if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
                goto out;

        if (!page_mapped(page)) 

Quoted 15 lines · KOSAKI Motohiro

mm/rmap.c +1 -1

index 8b088f0..b4a0b5b 100644
@@ -295,7 +295,7 @@ struct anon_vma *page_lock_anon_vma(struct page
 *page)
        unsigned long anon_mapping;
 
        rcu_read_lock();
-       anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
+       anon_mapping = (unsigned long) rcu_dereference(page->mapping);
        if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
                goto out;

Quoted 1 line

        if (!page_mapped(page)) 

Yes, I think this is indeed required.

I'll do a new version of the patch that includes the comment updates requested by Andrew.

Quoted 20 lines · On Fri, 09 Apr 2010 10:44:53 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 15 lines · KOSAKI Motohiro

mm/rmap.c +1 -1

index 8b088f0..b4a0b5b 100644
@@ -295,7 +295,7 @@ struct anon_vma *page_lock_anon_vma(struct page
 *page)
        unsigned long anon_mapping;
 
        rcu_read_lock();
-       anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
+       anon_mapping = (unsigned long) rcu_dereference(page->mapping);
        if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
                goto out;

Quoted 1 line

        if (!page_mapped(page)) 

Yes, I think this is indeed required.

Quoted 2 lines

I'll do a new version of the patch that includes the comment updates requested by Andrew.

Either this didn't happen or I lost the patch.

I parked mm-revalidate-anon_vma-in-page_lock_anon_vma.patch for now. Hopefully everything still works without it..

Index: linux-2.6/mm/rmap.c
===================================================================

mm/rmap.c +1 -0

@@ -294,6 +294,7 @@ struct anon_vma *page_lock_anon_vma(stru
 	unsigned long anon_mapping;
 
 	rcu_read_lock();
+again:
 	anon_mapping = (unsigned long) ACCESS_ONCE(page->mapping);
 	if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		goto out;

@@ -302,6 +303,12 @@ struct anon_vma *page_lock_anon_vma(stru
 
 	anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);

We still have a stray quicklist header included even though we axed quicklist usage quite a while back.

Signed-off-by Peter Zijlstra<[email protected]>

We need an anon_vma refcount for preemptible anon_vma->lock as well as memory compaction, so move it out into generic code.

Signed-off-by Peter Zijlstra<[email protected]>

Hi,

mm/rmap.c again: · line 316

Quoted 13 lines · Peter Zijlstra

mm/rmap.c +3 -2

@@ -302,23 +307,33 @@ again:
 		goto out;
 
 	anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
-	spin_lock(&anon_vma->lock);
+	if (!atomic_inc_not_zero(&anon_vma->ref))
+		anon_vma = NULL;
 
 	if (page_rmapping(page) != anon_vma) {
-		spin_unlock(&anon_vma->lock);
+		anon_vma_put(anon_vma);
 		goto again;
 	}

AFAICS anon_vma_put might be called with anon_vma == NULL here which will oops on the ref count. Not sure if

     page_rmapping(page) == anon_vma == NULL 

is possible, too.

include/asm-generic/tlb.h line 43

Quoted 13 lines · Peter Zijlstra

include/asm-generic/tlb.h +6 -1

@@ -39,30 +33,48 @@
 struct mmu_gather {
 	struct mm_struct	*mm;
 	unsigned int		nr;	/* set to ~0U means fast mode */
+	unsigned int		max;	/* nr < max */
 	unsigned int		need_flush;/* Really unmapped some ptes? */
 	unsigned int		fullmm; /* non-zero means full mm flush */
-	struct page *		pages[FREE_PTE_NR];
+#ifdef HAVE_ARCH_MMU_GATHER
+	struct arch_mmu_gather	arch;
+#endif
+	struct page		**pages;
+	struct page		*local[8];

Have you done some profiling on this? What I would like to see, if it's not too much complexity, is to have a small set of pages to handle common size frees, and then use them up first by default before attempting to allocate more.

Also, it would be cool to be able to chain allocations to avoid TLB flushes even on big frees (overridable by arch of course, in case they're doing some non-preeemptible work or you wish to break up lock hold times). But that might be just getting over engineered.

Have you done some profiling on this? What I would like to see, if it's not too much complexity, is to have a small set of pages to handle common size frees, and then use them up first by default before attempting to allocate more.

Also, it would be cool to be able to chain allocations to avoid TLB flushes even on big frees (overridable by arch of course, in case they're doing some non-preeemptible work or you wish to break up lock hold times). But that might be just getting over engineered.

Quoted 9 lines · Nick Piggin

Have you done some profiling on this? What I would like to see, if it's not too much complexity, is to have a small set of pages to handle common size frees, and then use them up first by default before attempting to allocate more.

Also, it would be cool to be able to chain allocations to avoid TLB flushes even on big frees (overridable by arch of course, in case they're doing some non-preeemptible work or you wish to break up lock hold times). But that might be just getting over engineered.

Fix up powerpc to the new mmu_gather stuffs.

PPC has an extra batching queue to RCU free the actual pagetable allocations, use the ARCH extentions for that for now.

For the ppc64_tlb_batch, which tracks the vaddrs to unhash from the hardware hash-table, keep using per-cpu arrays but flush on context switch and use a TIF bit to track the laxy_mmu state.

Quoted 5 lines · Peter Zijlstra

Fix up powerpc to the new mmu_gather stuffs.

PPC has an extra batching queue to RCU free the actual pagetable allocations, use the ARCH extentions for that for now.

Quoted 3 lines

For the ppc64_tlb_batch, which tracks the vaddrs to unhash from the hardware hash-table, keep using per-cpu arrays but flush on context switch and use a TIF bit to track the laxy_mmu state.

Hm. Pity powerpc can't just use tlb flush gathering for this batching, (which is what it was designed for). Then it could avoid these tricks. What's preventing this? Adding a tlb gather for COW case in copy_page_range?

Quoted 10 lines · Nick Piggin

Quoted 8 lines · Peter Zijlstra

Fix up powerpc to the new mmu_gather stuffs.

PPC has an extra batching queue to RCU free the actual pagetable allocations, use the ARCH extentions for that for now.

For the ppc64_tlb_batch, which tracks the vaddrs to unhash from the hardware hash-table, keep using per-cpu arrays but flush on context switch and use a TIF bit to track the laxy_mmu state.

Quoted 4 lines

Hm. Pity powerpc can't just use tlb flush gathering for this batching, (which is what it was designed for). Then it could avoid these tricks. What's preventing this? Adding a tlb gather for COW case in copy_page_range?

I'm not quite sure what about that, didn't fully investigate it, just wanted to get something working for now.

Of of the things is that both power and sparc need more than the struct page we normally gather.

I did think of making the mmu_gather have something like

struct mmu_page {
struct page *page;
#ifdef HAVE_ARCH_TLB_VADDR
unsigned long vaddr;
#endif
};

struct mmu_gather {
...
unsigned int nr;
struct mmu_page *pages;
};

and doing that vaddr collection right along with it in the same batch.

I think that that would work, Ben, Dave?

Quoted 6 lines

struct mmu_page {
struct page *page;
#ifdef HAVE_ARCH_TLB_VADDR
unsigned long vaddr;
#endif
};

Well you could also have a per-arch struct for this, which they can fill in their own info with (I think powerpc takes the pte as well)

and doing that vaddr collection right along with it in the same batch.

I think that that would work, Ben, Dave?

Quoted 6 lines

PPC has an extra batching queue to RCU free the actual pagetable allocations, use the ARCH extentions for that for now.

For the ppc64_tlb_batch, which tracks the vaddrs to unhash from the hardware hash-table, keep using per-cpu arrays but flush on context switch and use a TIF bit to track the laxy_mmu state.

Hm. Pity powerpc can't just use tlb flush gathering for this batching, (which is what it was designed for). Then it could avoid these tricks. What's preventing this? Adding a tlb gather for COW case in copy_page_range?

arch/powerpc/include/asm/tlbflush.h +1 -3

-	if (batch->index)
-		__flush_tlb_pending(batch);
-	batch->active = 0;
+	put_cpu_var(ppc64_tlb_batch);
 }
 
 #define arch_flush_lazy_mmu_mode()      do {} while (0)

Index: linux-2.6/arch/powerpc/kernel/process.c
===================================================================

arch/powerpc/kernel/process.c +3 -0

@@ -389,6 +389,9 @@ struct task_struct *__switch_to(struct t
 	struct thread_struct *new_thread, *old_thread;
 	unsigned long flags;
 	struct task_struct *last;
+#ifdef CONFIG_PPC64
+	struct ppc64_tlb_batch *batch;
+#endif
 
 #ifdef CONFIG_SMP
 	/* avoid complexity of lazy save/restore of fpu

@@ -479,6 +482,14 @@ struct task_struct *__switch_to(struct t
 		old_thread->accum_tb += (current_tb - start_tb);
 		new_thread->start_tb = current_tb;

arch/powerpc/kernel/process.c +7 -0

 	local_irq_save(flags);

@@ -495,6 +506,13 @@ struct task_struct *__switch_to(struct t
 	hard_irq_disable();
 	last = _switch(old_thread, new_thread);
 
+#ifdef CONFIG_PPC64
+	if (test_and_clear_ti_thread_flag(task_thread_info(new), TIF_LAZY_MMU)) {
+		batch = &__get_cpu_var(ppc64_tlb_batch);
+		batch->active = 1;
+	}
+#endif
+
 	local_irq_restore(flags);
 
 	return last;

Index: linux-2.6/arch/powerpc/mm/pgtable.c
===================================================================

arch/powerpc/mm/pgtable.c +0 -5

@@ -33,8 +33,6 @@
 
 #include "mmu_decl.h"
 
-DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
-
 #ifdef CONFIG_SMP
 
 /*

@@ -43,7 +41,6 @@ DEFINE_PER_CPU(struct mmu_gather, mmu_ga
  * freeing a page table page that is being walked without locks
  */
 
-static DEFINE_PER_CPU(struct pte_freelist_batch *, pte_freelist_cur);
 static unsigned long pte_freelist_forced_free;
 
 struct pte_freelist_batch

@@ -98,12 +95,30 @@ static void pte_free_submit(struct pte_f
 
 void pgtable_free_tlb(struct mmu_gather *tlb, void *table, unsigned shift)
 {
-	/* This is safe since tlb_gather_mmu has disabled preemption */
-	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);

arch/powerpc/mm/pgtable.c +3 -3

+	if (atomic_read(&tlb->mm->mm_users) < 2) {
 		pgtable_free(table, shift);
 		return;
 	}

@@ -125,10 +140,9 @@ void pgtable_free_tlb(struct mmu_gather 
 	}
 }
 
-void pte_free_finish(void)
+void pte_free_finish(struct mmu_gather *tlb)
 {
-	/* This is safe since tlb_gather_mmu has disabled preemption */
-	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);
+	struct pte_freelist_batch **batchp = &tlb->arch.batch;
 
 	if (*batchp == NULL)
 		return;

Index: linux-2.6/arch/powerpc/mm/tlb_hash64.c
===================================================================

arch/powerpc/mm/tlb_hash64.c +7 -5

@@ -38,13 +38,11 @@ DEFINE_PER_CPU(struct ppc64_tlb_batch, p
  * neesd to be flushed. This function will either perform the flush
  * immediately or will batch it up if the current CPU has an active
  * batch on it.
- *
- * Must be called from within some kind of spinlock/non-preempt region...
  */
 void hpte_need_flush(struct mm_struct *mm, unsigned long addr,
 		     pte_t *ptep, unsigned long pte, int huge)
 {
-	struct ppc64_tlb_batch *batch = &__get_cpu_var(ppc64_tlb_batch);
+	struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);
 	unsigned long vsid, vaddr;
 	unsigned int psize;
 	int ssize;

@@ -99,6 +97,7 @@ void hpte_need_flush(struct mm_struct *m
 	 */
 	if (!batch->active) {
 		flush_hash_page(vaddr, rpte, psize, ssize, 0);
+		put_cpu_var(ppc64_tlb_batch);
 		return;
 	}
 

@@ -127,6 +126,7 @@ void hpte_need_flush(struct mm_struct *m
 	batch->index = ++i;
 	if (i >= PPC64_TLB_BATCH_NR)
 		__flush_tlb_pending(batch);
+	put_cpu_var(ppc64_tlb_batch);
 }
 
 /*

@@ -155,7 +155,7 @@ void __flush_tlb_pending(struct ppc64_tl
 
 void tlb_flush(struct mmu_gather *tlb)
 {
-	struct ppc64_tlb_batch *tlbbatch = &__get_cpu_var(ppc64_tlb_batch);
+	struct ppc64_tlb_batch *tlbbatch = &get_cpu_var(ppc64_tlb_batch);
 
 	/* If there's a TLB batch pending, then we must flush it because the
 	 * pages are going to be freed and we really don't want to have a CPU

@@ -164,8 +164,10 @@ void tlb_flush(struct mmu_gather *tlb)
 	if (tlbbatch->index)
 		__flush_tlb_pending(tlbbatch);
 
+	put_cpu_var(ppc64_tlb_batch);
+
 	/* Push out batch of freed page tables */
-	pte_free_finish();
+	pte_free_finish(tlb);
 }
 
 /**

Index: linux-2.6/arch/powerpc/include/asm/thread_info.h
===================================================================

arch/powerpc/include/asm/thread_info.h +2 -0

@@ -111,6 +111,7 @@ static inline struct thread_info *curren
 #define TIF_NOTIFY_RESUME	13	/* callback before returning to user */
 #define TIF_FREEZE		14	/* Freezing for suspend */
 #define TIF_RUNLATCH		15	/* Is the runlatch enabled? */
+#define TIF_LAZY_MMU		16	/* tlb_batch is active */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)

@@ -128,6 +129,7 @@ static inline struct thread_info *curren
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_RUNLATCH		(1<<TIF_RUNLATCH)
+#define _TIF_LAZY_MMU		(1<<TIF_LAZY_MMU)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
 
 #define _TIF_USER_WORK_MASK	(_TIF_SIGPENDING | _TIF_NEED_RESCHED | \

Index: linux-2.6/arch/powerpc/include/asm/pgalloc.h
===================================================================

arch/powerpc/include/asm/pgalloc.h +2 -2

@@ -32,13 +32,13 @@ static inline void pte_free(struct mm_st
 
 #ifdef CONFIG_SMP
 extern void pgtable_free_tlb(struct mmu_gather *tlb, void *table, unsigned shift);
-extern void pte_free_finish(void);
+extern void pte_free_finish(struct mmu_gather *tlb);
 #else /* CONFIG_SMP */
 static inline void pgtable_free_tlb(struct mmu_gather *tlb, void *table, unsigned shift)
 {
 	pgtable_free(table, shift);
 }
-static inline void pte_free_finish(void) { }
+static inline void pte_free_finish(struct mmu_gather *tlb) { }
 #endif /* !CONFIG_SMP */
 
 static inline void __pte_free_tlb(struct mmu_gather *tlb, struct page *ptepage,

Index: linux-2.6/arch/powerpc/mm/tlb_hash32.c
===================================================================

arch/powerpc/mm/tlb_hash32.c +1 -1

@@ -73,7 +73,7 @@ void tlb_flush(struct mmu_gather *tlb)
 	}
 
 	/* Push out batch of freed page tables */
-	pte_free_finish();
+	pte_free_finish(tlb);
 }
 
 /*

Index: linux-2.6/arch/powerpc/mm/tlb_nohash.c
===================================================================

arch/powerpc/mm/tlb_nohash.c +1 -1

@@ -298,7 +298,7 @@ void tlb_flush(struct mmu_gather *tlb)
 	flush_tlb_mm(tlb->mm);
 
 	/* Push out batch of freed page tables */
-	pte_free_finish();
+	pte_free_finish(tlb);
 }
 
 /*

arch/powerpc/include/asm/tlbflush.h extern void hpte_need_flush(struct mm_st · line 126

Quoted 10 lines

static inline void arch_leave_lazy_mmu_mode(void) {

arch/powerpc/include/asm/tlbflush.h +7 -1

-     struct ppc64_tlb_batch *batch = &__get_cpu_var(ppc64_tlb_batch);
+     struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);
+
+     if (batch->active) {
+             if (batch->index)
+                     __flush_tlb_pending(batch);
+             batch->active = 0;
+     }

Can index be > 0 if active == 0 ? I though not, which means you don't need to add a test on active, do you ?

I'm also pondering whether we should just stick something in the task/thread struct and avoid that whole per-cpu manipulation including the stuff in process.c in fact.

Heh, maybe it's time to introduce thread vars ? :-)

arch/powerpc/mm/pgtable.c static void pte_free_submit(struct pte_f · line 105

Quoted 4 lines

arch/powerpc/mm/pgtable.c +4 -0

+      * A comment here about on why we have RCU freed page tables might be
+      * interesting, also explaining why we don't need any sort of grace
+      * period for mm_users == 1, and have some home brewn smp_call_func()
+      * for single frees.

iirc, we are synchronizing with CPUs walking page tables in their hash or TLB miss code, which is lockless. The mm_users test is a -little- bit dubious indeed. It may have to be mm_users < 2 && mm == current->active_mm, ie, we know for sure nobody else is currently walking those page tables ...

Tho even than is fishy nowadays. We -can- walk page tables on behave of another process. In fact, we do it in the Cell SPU code for faulting page table entries as a result of SPEs taking faults for example. So I'm starting to suspect that this mm_users optimisation is bogus.

We -do- want to optimize out the case where there is no user left though, ie, the MM is dead. IE. The typical exit case.

arch/powerpc/mm/pgtable.c static void pte_free_submit(struct pte_f · line 115

Quoted 4 lines

arch/powerpc/mm/pgtable.c +4 -0

+      * If there are, but we fail to batch, we need to IPI (all?) CPUs so as
+      * to serialize against the IRQ disable. In case we do batch, the RCU
+      * grace period is at least long enough to cover IRQ disabled sections
+      * (XXX assumption, not strictly true).

Yeah well ... I'm not that familiar with RCU anymore. Back when I wrote that, iirc, I would more or less be safe against a CPU that doesn't schedule, but things may have well changed.

We are trying to be safe against another CPU walking page tables in the asm lockless hash miss or TLB miss code. Note that sparc64 has a similar issue. This is highly optimized asm code that -cannot- call into things like rcu_read_lock().