x86/mm: Implement lockless pgd_alloc()/pgd_free()

arch/x86/mm/init_64.c +3 -4

@@ -169,29 +169,40 @@ void sync_global_pgds(unsigned long start, unsigned long end, int removed)
 
 	for (address = start; address <= end; address += PGDIR_SIZE) {
 		const pgd_t *pgd_ref = pgd_offset_k(address);
-		struct page *page;
+		struct task_struct *g, *p;
 
 		/*
-		 * When it is called after memory hot remove, pgd_none()
-		 * returns true. In this case (removed == 1), we must clear
-		 * the PGD entries in the local PGD level page.
+		 * When this function is called after memory hot remove,
+		 * pgd_none() already returns true, but only the reference

arch/x86/mm/init_64.c +2 -0

+		for_each_process_thread(g, p) {
+			struct mm_struct *mm;
 			pgd_t *pgd;
 			spinlock_t *pgt_lock;

Quoted 14 lines · Ingo Molnar

arch/x86/mm/init_64.c +3 -4

@@ -169,29 +169,40 @@ void sync_global_pgds(unsigned long start, unsigned long end, int removed)
 
 	for (address = start; address <= end; address += PGDIR_SIZE) {
 		const pgd_t *pgd_ref = pgd_offset_k(address);
-		struct page *page;
+		struct task_struct *g, *p;
 
 		/*
-		 * When it is called after memory hot remove, pgd_none()
-		 * returns true. In this case (removed == 1), we must clear
-		 * the PGD entries in the local PGD level page.
+		 * When this function is called after memory hot remove,
+		 * pgd_none() already returns true, but only the reference

arch/x86/mm/init_64.c void sync_global_pgds(unsigned long start, unsigned long end, int removed) · line 184

Quoted 10 lines

arch/x86/mm/init_64.c +4 -2

+		 * kernel PGD has been cleared, not the process PGDs.
+		 *
+		 * So clear the affected entries in every process PGD as well:
 		 */
 		if (pgd_none(*pgd_ref) && !removed)
 			continue;
 
-		spin_lock(&pgd_lock);
-		list_for_each_entry(page, &pgd_list, lru) {
+		spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */

                                         ^^^^^^^^^^^^^^^^^^^^^^^

Hmm, but it doesn't if PREEMPT_RCU? No, no, I do not pretend I understand how it actually works ;) But, say, rcu_check_callbacks() can be called from irq and since spin_lock() doesn't increment current->rcu_read_lock_nesting this can lead to rcu_preempt_qs()?

Quoted 5 lines

arch/x86/mm/init_64.c +2 -0

+		for_each_process_thread(g, p) {
+			struct mm_struct *mm;
 			pgd_t *pgd;
 			spinlock_t *pgt_lock;
 

arch/x86/mm/init_64.c void sync_global_pgds(unsigned long start, unsigned long end, int removed) · line 196

Quoted 9 lines

arch/x86/mm/init_64.c +6 -3

-			pgd = (pgd_t *)page_address(page) + pgd_index(address);
-			/* the pgt_lock only for Xen */
-			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
+			task_lock(p);
+			mm = p->mm;
+			if (!mm) {
+				task_unlock(p);
+				continue;
+			}

Again, you can simplify this code and avoid for_each_process_thread() if you use for_each_process() + find_lock_task_mm().

Quoted 7 lines · Oleg Nesterov

arch/x86/mm/init_64.c void sync_global_pgds(unsigned long start, unsigned long end, int removed) · line 184

Quoted 1 line

arch/x86/mm/init_64.c +1 -0

+		spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */

                                         ^^^^^^^^^^^^^^^^^^^^^^^

Hmm, but it doesn't if PREEMPT_RCU? No, no, I do not pretend I understand how it actually works ;) But, say, rcu_check_callbacks() can be called from irq and since spin_lock() doesn't increment current->rcu_read_lock_nesting this can lead to rcu_preempt_qs()?

No, RCU grace periods are still defined by 'heavy' context boundaries such as context switches, entering idle or user-space mode.

PREEMPT_RCU is like traditional RCU, except that blocking is allowed within the RCU read critical section - that is why it uses a separate nesting counter (current->rcu_read_lock_nesting), not the preempt count.

But if a piece of kernel code is non-preemptible, such as a spinlocked region or an irqs-off region, then those are still natural RCU read lock regions, regardless of the RCU model, and need no additional RCU locking.

Quoted 11 lines · Ingo Molnar

Quoted 7 lines · Oleg Nesterov

Review

Quoted 1 line

(quoted diff) +1 -0

+		spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */

                                         ^^^^^^^^^^^^^^^^^^^^^^^

Hmm, but it doesn't if PREEMPT_RCU? No, no, I do not pretend I understand how it actually works ;) But, say, rcu_check_callbacks() can be called from irq and since spin_lock() doesn't increment current->rcu_read_lock_nesting this can lead to rcu_preempt_qs()?

Quoted 6 lines

No, RCU grace periods are still defined by 'heavy' context boundaries such as context switches, entering idle or user-space mode.

PREEMPT_RCU is like traditional RCU, except that blocking is allowed within the RCU read critical section - that is why it uses a separate nesting counter (current->rcu_read_lock_nesting), not the preempt count.

Yes.

Quoted 3 lines

But if a piece of kernel code is non-preemptible, such as a spinlocked region or an irqs-off region, then those are still natural RCU read lock regions, regardless of the RCU model, and need no additional RCU locking.

I do not think so. Yes I understand that rcu_preempt_qs() itself doesn't finish the gp, but if there are no other rcu-read-lock holders then it seems synchronize_rcu() on another CPU can return _before_ spin_unlock(), this CPU no longer needs rcu_preempt_note_context_switch().

OK, I can be easily wrong, I do not really understand the implementation of PREEMPT_RCU. Perhaps preempt_disable() can actually act as rcu_read_lock() with the _current_ implementation. Still this doesn't look right even if happens to work, and Documentation/RCU/checklist.txt says:

11. Note that synchronize_rcu() -only- guarantees to wait until
all currently executing rcu_read_lock()-protected RCU read-side critical sections complete. It does -not- necessarily guarantee that all currently running interrupts, NMIs, preempt_disable() code, or idle loops will complete. Therefore, if your read-side critical sections are protected by something other than rcu_read_lock(), do -not- use synchronize_rcu().

Quoted 36 lines · Oleg Nesterov

Quoted 11 lines · Ingo Molnar

Quoted 7 lines · Oleg Nesterov

Review

Quoted 1 line

(quoted diff) +1 -0

+		spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */

                                         ^^^^^^^^^^^^^^^^^^^^^^^

Hmm, but it doesn't if PREEMPT_RCU? No, no, I do not pretend I understand how it actually works ;) But, say, rcu_check_callbacks() can be called from irq and since spin_lock() doesn't increment current->rcu_read_lock_nesting this can lead to rcu_preempt_qs()?

Quoted 6 lines

No, RCU grace periods are still defined by 'heavy' context boundaries such as context switches, entering idle or user-space mode.

PREEMPT_RCU is like traditional RCU, except that blocking is allowed within the RCU read critical section - that is why it uses a separate nesting counter (current->rcu_read_lock_nesting), not the preempt count.

Yes.

Quoted 3 lines

But if a piece of kernel code is non-preemptible, such as a spinlocked region or an irqs-off region, then those are still natural RCU read lock regions, regardless of the RCU model, and need no additional RCU locking.

I do not think so. Yes I understand that rcu_preempt_qs() itself doesn't finish the gp, but if there are no other rcu-read-lock holders then it seems synchronize_rcu() on another CPU can return _before_ spin_unlock(), this CPU no longer needs rcu_preempt_note_context_switch().

OK, I can be easily wrong, I do not really understand the implementation of PREEMPT_RCU. Perhaps preempt_disable() can actually act as rcu_read_lock() with the _current_ implementation. Still this doesn't look right even if happens to work, and Documentation/RCU/checklist.txt says:

Quoted 7 lines

11. Note that synchronize_rcu() -only- guarantees to wait until
all currently executing rcu_read_lock()-protected RCU read-side critical sections complete. It does -not- necessarily guarantee that all currently running interrupts, NMIs, preempt_disable() code, or idle loops will complete. Therefore, if your read-side critical sections are protected by something other than rcu_read_lock(), do -not- use synchronize_rcu().

I've even checked this ;) I applied the stupid patch below and then

	$ taskset 2 perl -e 'syscall 157, 666, 5000' &
	[1] 565

	$ taskset 1 perl -e 'syscall 157, 777'

	$
	[1]+  Done                    taskset 2 perl -e 'syscall 157, 666, 5000'

	$ dmesg -c
	SPIN start
	SYNC start
	SYNC done!
	SPIN done!

Oleg.

kernel/sys.c +9 -0

@@ -2049,6 +2049,9 @@ static int prctl_get_tid_address(struct task_struct *me, int __user **tid_addr)
 }
 #endif
 
+#include <linux/delay.h>
+
+
 SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		unsigned long, arg4, unsigned long, arg5)
 {

@@ -2062,6 +2065,19 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 
 	error = 0;
 	switch (option) {
+	case 666:
+		preempt_disable();
+		pr_crit("SPIN start\n");
+		while (arg2--)
+			mdelay(1);
+		pr_crit("SPIN done!\n");

Quoted 53 lines · Oleg Nesterov

Quoted 36 lines · Oleg Nesterov

Quoted 11 lines · Ingo Molnar

Quoted 7 lines · Oleg Nesterov

Review

Quoted 1 line

(quoted diff) +1 -0

+		spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */

                                         ^^^^^^^^^^^^^^^^^^^^^^^

Hmm, but it doesn't if PREEMPT_RCU? No, no, I do not pretend I understand how it actually works ;) But, say, rcu_check_callbacks() can be called from irq and since spin_lock() doesn't increment current->rcu_read_lock_nesting this can lead to rcu_preempt_qs()?

Quoted 6 lines

No, RCU grace periods are still defined by 'heavy' context boundaries such as context switches, entering idle or user-space mode.

PREEMPT_RCU is like traditional RCU, except that blocking is allowed within the RCU read critical section - that is why it uses a separate nesting counter (current->rcu_read_lock_nesting), not the preempt count.

Yes.

Quoted 3 lines

But if a piece of kernel code is non-preemptible, such as a spinlocked region or an irqs-off region, then those are still natural RCU read lock regions, regardless of the RCU model, and need no additional RCU locking.

I do not think so. Yes I understand that rcu_preempt_qs() itself doesn't finish the gp, but if there are no other rcu-read-lock holders then it seems synchronize_rcu() on another CPU can return _before_ spin_unlock(), this CPU no longer needs rcu_preempt_note_context_switch().

OK, I can be easily wrong, I do not really understand the implementation of PREEMPT_RCU. Perhaps preempt_disable() can actually act as rcu_read_lock() with the _current_ implementation. Still this doesn't look right even if happens to work, and Documentation/RCU/checklist.txt says:

Quoted 7 lines

11. Note that synchronize_rcu() -only- guarantees to wait until
all currently executing rcu_read_lock()-protected RCU read-side critical sections complete. It does -not- necessarily guarantee that all currently running interrupts, NMIs, preempt_disable() code, or idle loops will complete. Therefore, if your read-side critical sections are protected by something other than rcu_read_lock(), do -not- use synchronize_rcu().

I've even checked this ;) I applied the stupid patch below and then

	$ taskset 2 perl -e 'syscall 157, 666, 5000' &
	[1] 565

	$ taskset 1 perl -e 'syscall 157, 777'

Quoted 8 lines

	$
	[1]+  Done                    taskset 2 perl -e 'syscall 157, 666, 5000'

	$ dmesg -c
	SPIN start
	SYNC start
	SYNC done!
	SPIN done!

Please accept my apologies for my late entry to this thread. Youngest kid graduated from university this weekend, so my attention has been elsewhere.

If you were to disable interrupts instead of preemption, I would expect that the preemptible-RCU grace period would be blocked -- though I am not particularly comfortable with people relying on disabled interrupts blocking a preemptible-RCU grace period.

Here is what can happen if you try to block a preemptible-RCU grace period by disabling preemption, assuming that there are at least two online CPUs in the system:

1. CPU 0 does spin_lock(), which disables preemption.

2. CPU 1 starts a grace period.

3. CPU 0 takes a scheduling-clock interrupt. It raises softirq,
and the RCU_SOFTIRQ handler notes that there is a new grace period and sets state so that a subsequent quiescent state on this CPU will be noted.

4. CPU 0 takes another scheduling-clock interrupt, which checks
current->rcu_read_lock_nesting, and notes that there is no preemptible-RCU read-side critical section in progress. It again raises softirq, and the RCU_SOFTIRQ handler reports the quiescent state to core RCU.

5. Once each of the other CPUs report a quiescent state, the
grace period can end, despite CPU 0 having preemption disabled the whole time.

So Oleg's test is correct, disabling preemption is not sufficient to block a preemptible-RCU grace period.

The usual suggestion would be to add rcu_read_lock() just after the lock is acquired and rcu_read_unlock() just before each release of that same lock.

arch/x86/xen/mmu.c +15 -7

@@ -967,19 +979,32 @@ static void xen_pgd_unpin(struct mm_struct *mm)
  */
 void xen_mm_unpin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (PageSavePinned(page)) {
-			BUG_ON(!PagePinned(page));
-			__xen_pgd_unpin(&init_mm, (pgd_t *)page_address(page));
-			ClearPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (PageSavePinned(page)) {
+				BUG_ON(!PagePinned(page));

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

(As I don't use Xen this is was only build tested.)

Cc Andrew Morton <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Waiman Long <[email protected]>

Cc [email protected]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/xen/mmu.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

arch/x86/xen/mmu.c +35 -13

index dd151b2045b0..70a3df5b0b54 100644
@@ -853,15 +853,27 @@ static void xen_pgd_pin(struct mm_struct *mm)
  */
 void xen_mm_pin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (!PagePinned(page)) {
-			__xen_pgd_pin(&init_mm, (pgd_t *)page_address(page));
-			SetPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (!PagePinned(page)) {
+				__xen_pgd_pin(&init_mm, pgd);
+				SetPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 
 	spin_unlock(&pgd_lock);

@@ -967,19 +979,32 @@ static void xen_pgd_unpin(struct mm_struct *mm)
  */
 void xen_mm_unpin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (PageSavePinned(page)) {
-			BUG_ON(!PagePinned(page));
-			__xen_pgd_unpin(&init_mm, (pgd_t *)page_address(page));
-			ClearPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (PageSavePinned(page)) {
+				BUG_ON(!PagePinned(page));
+				__xen_pgd_unpin(&init_mm, pgd);
+				ClearPageSavePinned(page);

Quoted 8 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

Quoted 2 lines

(As I don't use Xen this is was only build tested.)

In which case it seems extra important to copy the appropriate maintainers, which I've done here.

Ian.

Quoted 91 lines

Cc Andrew Morton <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Waiman Long <[email protected]>

Cc [email protected]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/xen/mmu.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

arch/x86/xen/mmu.c +37 -13

index dd151b2045b0..70a3df5b0b54 100644
@@ -853,15 +853,27 @@ static void xen_pgd_pin(struct mm_struct *mm)
  */
 void xen_mm_pin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (!PagePinned(page)) {
-			__xen_pgd_pin(&init_mm, (pgd_t *)page_address(page));
-			SetPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (!PagePinned(page)) {
+				__xen_pgd_pin(&init_mm, pgd);
+				SetPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 
 	spin_unlock(&pgd_lock);

@@ -967,19 +979,32 @@ static void xen_pgd_unpin(struct mm_struct *mm)
  */
 void xen_mm_unpin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (PageSavePinned(page)) {
-			BUG_ON(!PagePinned(page));
-			__xen_pgd_unpin(&init_mm, (pgd_t *)page_address(page));
-			ClearPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (PageSavePinned(page)) {
+				BUG_ON(!PagePinned(page));
+				__xen_pgd_unpin(&init_mm, pgd);
+				ClearPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 

arch/x86/xen/mmu.c static void xen_pgd_unpin(struct mm_struct *mm) · line 1010

Quoted 5 lines

arch/x86/xen/mmu.c +1 -0

 	spin_unlock(&pgd_lock);
+	rcu_read_unlock();
 }
 
 static void xen_activate_mm(struct mm_struct *prev, struct mm_struct *next)

Quoted 8 lines · Ian Campbell

Quoted 6 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

Quoted 2 lines

Quoted 2 lines

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

It is not equivalent because pgd_alloc() now populates entries in pgds that are not visible to xen_mm_pin_all() (note how the original code adds the pgd to the pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly allocated page tables won't be correctly converted on suspend/resume and the new process will die after resume.

Quoted 12 lines · David Vrabel

Quoted 10 lines · Ian Campbell

Quoted 8 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

Quoted 5 lines

It is not equivalent because pgd_alloc() now populates entries in pgds that are not visible to xen_mm_pin_all() (note how the original code adds the pgd to the pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly allocated page tables won't be correctly converted on suspend/resume and the new process will die after resume.

So how should the Xen logic be fixed for the new scheme? I can't say I can see through the paravirt complexity here.

Quoted 17 lines · Ingo Molnar

Quoted 15 lines · David Vrabel

Quoted 9 lines · Ian Campbell

Quoted 8 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

It is not equivalent because pgd_alloc() now populates entries in pgds that are not visible to xen_mm_pin_all() (note how the original code adds the pgd to the pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly allocated page tables won't be correctly converted on suspend/resume and the new process will die after resume.

Quoted 2 lines

So how should the Xen logic be fixed for the new scheme? I can't say I can see through the paravirt complexity here.

Actually, since we freeze_processes() before trying to pin page tables, I think it should be ok as-is.

I'll put the patch through some tests.

Quoted 28 lines · David Vrabel

Quoted 27 lines · Ingo Molnar

Quoted 13 lines · David Vrabel

Quoted 7 lines · Ian Campbell

Quoted 6 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance
suspend/restore. It's a stop-all method that needs to iterate through all
allocated pgds in the system to fix them up for Xen's use.

Quoted 5 lines

Quoted 5 lines

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all
tasks in the system. This is an equivalent method.

Quoted 9 lines

It is not equivalent because pgd_alloc() now populates entries in pgds that are
not visible to xen_mm_pin_all() (note how the original code adds the pgd to the
pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly
allocated page tables won't be correctly converted on suspend/resume and the new
process will die after resume.

So how should the Xen logic be fixed for the new scheme? I can't say I can see
through the paravirt complexity here.

Quoted 4 lines

Actually, since we freeze_processes() before trying to pin page tables, I think it should be ok as-is.

I'll put the patch through some tests.

Actually, I just ran this through a couple of boot/suspend/resume tests and didn't see any issues (with the one fix I mentioned to Ingo earlier). On unstable Xen only.

Quoted 28 lines · David Vrabel

Quoted 27 lines · Ingo Molnar

Quoted 13 lines · David Vrabel

Quoted 7 lines · Ian Campbell

Quoted 6 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance
suspend/restore. It's a stop-all method that needs to iterate through all
allocated pgds in the system to fix them up for Xen's use.

Quoted 5 lines

Quoted 5 lines

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all
tasks in the system. This is an equivalent method.

Quoted 9 lines

It is not equivalent because pgd_alloc() now populates entries in pgds that are
not visible to xen_mm_pin_all() (note how the original code adds the pgd to the
pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly
allocated page tables won't be correctly converted on suspend/resume and the new
process will die after resume.

So how should the Xen logic be fixed for the new scheme? I can't say I can see
through the paravirt complexity here.

Quoted 4 lines

Actually, since we freeze_processes() before trying to pin page tables, I think it should be ok as-is.

I'll put the patch through some tests.

Actually, I just ran this through a couple of boot/suspend/resume tests and didn't see any issues (with the one fix I mentioned to Ingo earlier). On unstable Xen only.

Quoted 17 lines · Ingo Molnar

Quoted 15 lines · David Vrabel

Quoted 9 lines · Ian Campbell

Quoted 8 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

It is not equivalent because pgd_alloc() now populates entries in pgds that are not visible to xen_mm_pin_all() (note how the original code adds the pgd to the pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly allocated page tables won't be correctly converted on suspend/resume and the new process will die after resume.

Quoted 2 lines

So how should the Xen logic be fixed for the new scheme? I can't say I can see through the paravirt complexity here.

Actually, since we freeze_processes() before trying to pin page tables, I think it should be ok as-is.

I'll put the patch through some tests.

Quoted 12 lines · David Vrabel

Quoted 10 lines · Ian Campbell

Quoted 8 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

Quoted 5 lines

It is not equivalent because pgd_alloc() now populates entries in pgds that are not visible to xen_mm_pin_all() (note how the original code adds the pgd to the pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly allocated page tables won't be correctly converted on suspend/resume and the new process will die after resume.

So how should the Xen logic be fixed for the new scheme? I can't say I can see through the paravirt complexity here.

Quoted 8 lines · Ian Campbell

Quoted 6 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

Quoted 2 lines

Quoted 2 lines

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

It is not equivalent because pgd_alloc() now populates entries in pgds that are not visible to xen_mm_pin_all() (note how the original code adds the pgd to the pgd_list in pgd_ctor() before calling pgd_prepopulate_pmd()). These newly allocated page tables won't be correctly converted on suspend/resume and the new process will die after resume.

Quoted 8 lines · Ingo Molnar

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

Quoted 2 lines

(As I don't use Xen this is was only build tested.)

In which case it seems extra important to copy the appropriate maintainers, which I've done here.

Ian.

Quoted 96 lines

Cc Andrew Morton <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Waiman Long <[email protected]>

Cc [email protected]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/xen/mmu.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

arch/x86/xen/mmu.c +38 -13

index dd151b2045b0..70a3df5b0b54 100644
@@ -853,15 +853,27 @@ static void xen_pgd_pin(struct mm_struct *mm)
  */
 void xen_mm_pin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (!PagePinned(page)) {
-			__xen_pgd_pin(&init_mm, (pgd_t *)page_address(page));
-			SetPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (!PagePinned(page)) {
+				__xen_pgd_pin(&init_mm, pgd);
+				SetPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 
 	spin_unlock(&pgd_lock);

@@ -967,19 +979,32 @@ static void xen_pgd_unpin(struct mm_struct *mm)
  */
 void xen_mm_unpin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (PageSavePinned(page)) {
-			BUG_ON(!PagePinned(page));
-			__xen_pgd_unpin(&init_mm, (pgd_t *)page_address(page));
-			ClearPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (PageSavePinned(page)) {
+				BUG_ON(!PagePinned(page));
+				__xen_pgd_unpin(&init_mm, pgd);
+				ClearPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 
 	spin_unlock(&pgd_lock);
+	rcu_read_unlock();
 }
 
 static void xen_activate_mm(struct mm_struct *prev, struct mm_struct *next)

xen_mm_pin_all()/unpin_all() are used to implement full guest instance suspend/restore. It's a stop-all method that needs to iterate through all allocated pgds in the system to fix them up for Xen's use.

This code uses pgd_list, probably because it was an easy interface.

But we want to remove the pgd_list, so convert the code over to walk all tasks in the system. This is an equivalent method.

(As I don't use Xen this is was only build tested.)

Cc Andrew Morton <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Waiman Long <[email protected]>

Cc [email protected]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/xen/mmu.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

arch/x86/xen/mmu.c +38 -13

index dd151b2045b0..70a3df5b0b54 100644
@@ -853,15 +853,27 @@ static void xen_pgd_pin(struct mm_struct *mm)
  */
 void xen_mm_pin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (!PagePinned(page)) {
-			__xen_pgd_pin(&init_mm, (pgd_t *)page_address(page));
-			SetPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (!PagePinned(page)) {
+				__xen_pgd_pin(&init_mm, pgd);
+				SetPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 
 	spin_unlock(&pgd_lock);

@@ -967,19 +979,32 @@ static void xen_pgd_unpin(struct mm_struct *mm)
  */
 void xen_mm_unpin_all(void)
 {
-	struct page *page;
+	struct task_struct *g, *p;
 
-	spin_lock(&pgd_lock);
+	spin_lock(&pgd_lock); /* Implies rcu_read_lock() for the task list iteration: */
 
-	list_for_each_entry(page, &pgd_list, lru) {
-		if (PageSavePinned(page)) {
-			BUG_ON(!PagePinned(page));
-			__xen_pgd_unpin(&init_mm, (pgd_t *)page_address(page));
-			ClearPageSavePinned(page);
+	for_each_process_thread(g, p) {
+		struct mm_struct *mm;
+		struct page *page;
+		pgd_t *pgd;
+
+		task_lock(p);
+		mm = p->mm;
+		if (mm) {
+			pgd = mm->pgd;
+			page = virt_to_page(pgd);
+
+			if (PageSavePinned(page)) {
+				BUG_ON(!PagePinned(page));
+				__xen_pgd_unpin(&init_mm, pgd);
+				ClearPageSavePinned(page);
+			}
 		}
+		task_unlock(p);
 	}
 
 	spin_unlock(&pgd_lock);
+	rcu_read_unlock();
 }
 
 static void xen_activate_mm(struct mm_struct *prev, struct mm_struct *next)

I didn't read v2 yet, but I'd like to ask a question.

Why do we need vmalloc_sync_all()?

It has a single caller, register_die_notifier() which calls it without any explanation. IMO, this needs a comment at least.

I am not sure I understand the changelog in 101f12af correctly, but at first glance vmalloc_sync_all() is no longer needed at least on x86, do_page_fault() no longer does notify_die(DIE_PAGE_FAULT). And btw DIE_PAGE_FAULT has no users. DIE_MNI too...

Perhaps we can simply kill it on x86?

Quoted 4 lines · Oleg Nesterov

I didn't read v2 yet, but I'd like to ask a question.

Why do we need vmalloc_sync_all()?

Quoted 2 lines

It has a single caller, register_die_notifier() which calls it without any explanation. IMO, this needs a comment at least.

Yes, it's used to work around crashes in modular callbacks: if the callbacks happens to be called from within the page fault path, before the vmalloc page fault handler runs, then we have a catch-22 problem.

It's rare but not entirely impossible.

Quoted 6 lines

I am not sure I understand the changelog in 101f12af correctly, but at first glance vmalloc_sync_all() is no longer needed at least on x86, do_page_fault() no longer does notify_die(DIE_PAGE_FAULT). And btw DIE_PAGE_FAULT has no users. DIE_MNI too...

Perhaps we can simply kill it on x86?

So in theory we could still have it run from DIE_OOPS, and that could turn a survivable kernel crash into a non-survivable one.

But again, the kernel no longer does this? do_page_fault() does vmalloc_fault() without notify_die(). If it fails, I do not see how/why a modular DIE_OOPS handler could try to resolve this problem and trigger another fault.

Quoted 4 lines · Oleg Nesterov

But again, the kernel no longer does this? do_page_fault() does vmalloc_fault() without notify_die(). If it fails, I do not see how/why a modular DIE_OOPS handler could try to resolve this problem and trigger another fault.

The same problem can happen from NMI handlers or machine check handlers. It's not necessarily tied to page faults only.

Quoted 9 lines · Andi Kleen

Quoted 5 lines · Oleg Nesterov

But again, the kernel no longer does this? do_page_fault() does vmalloc_fault() without notify_die(). If it fails, I do not see how/why a modular DIE_OOPS handler could try to resolve this problem and trigger another fault.

The same problem can happen from NMI handlers or machine check handlers. It's not necessarily tied to page faults only.

AIUI, the point of the one and only vmalloc_sync_all call is to prevent infinitely recursive faults when we call a notify_die callback. The only thing that it could realistically protect is module text or static non-per-cpu module data, since that's the only thing that's reliably already in the init pgd. I'm with Oleg: I don't see how that can happen, since do_page_fault fixes up vmalloc faults before it calls notify_die.

Quoted 11 lines · Andy Lutomirski

Quoted 9 lines · Andi Kleen

Quoted 5 lines · Oleg Nesterov

But again, the kernel no longer does this? do_page_fault() does vmalloc_fault() without notify_die(). If it fails, I do not see how/why a modular DIE_OOPS handler could try to resolve this problem and trigger another fault.

The same problem can happen from NMI handlers or machine check handlers. It's not necessarily tied to page faults only.

Quoted 6 lines

AIUI, the point of the one and only vmalloc_sync_all call is to prevent infinitely recursive faults when we call a notify_die callback. The only thing that it could realistically protect is module text or static non-per-cpu module data, since that's the only thing that's reliably already in the init pgd. I'm with Oleg: I don't see how that can happen, since do_page_fault fixes up vmalloc faults before it calls notify_die.

Yes, but what I meant is that it can happen if due to an unrelated kernel bug and unlucky timing we have installed this new handler just when that other unrelated kernel bug triggers: say a #GPF crash in kernel code.

In any case it should all be mooted with the removal of lazy PGD instantiation.