Skip to contentSkip to conversation

vm_unmap_aliases and Xen

PATCH Applied Jeremy Fitzhardinge 3 participants 9 messages Oct 23, 2008 – Nov 6, 2008

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

I've been having a few problems with Xen, I suspect as a result of the lazy unmapping in vmalloc.c.

One immediate one is that vm_unmap_aliases() will oops if you call it before vmalloc_init() is called, which can happen in the Xen case. RFC patch below.

But the bigger problem I'm seeing is that despite calling vm_unmap_aliases() at the pertinent places, I'm still seeing errors resulting from stray aliases. Is it possible that vm_unmap_aliases() could be missing some, or not completely synchronous?

Subject: vmap: cope with vm_unmap_aliases before vmalloc_init()

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

mm/vmalloc.c +0 -0
diff -r 42c8b29f7ccf mm/vmalloc.c
@@ -591,6 +591,8 @@
 

#define VMAP_BLOCK_SIZE (VMAP_BBMAP_BITS * PAGE_SIZE)

+static bool vmap_initialized = false;
+

struct vmap_block_queue {
	spinlock_t lock;
	struct list_head free;
@@ -827,6 +829,9 @@
	int cpu;
	int flush = 0;

+ if (!vmap_initialized)
+ return;
+

	for_each_possible_cpu(cpu) {
		struct vmap_block_queue *vbq = &per_cpu(vmap_block_queue, cpu);
		struct vmap_block *vb;
@@ -940,6 +945,8 @@
		INIT_LIST_HEAD(&vbq->dirty);
		vbq->nr_dirty = 0;
	}

+
+ vmap_initialized = true;
}

void unmap_kernel_range(unsigned long addr, unsigned long size)

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

#1 lore raw
Quoted 6 lines · Jeremy Fitzhardinge

I've been having a few problems with Xen, I suspect as a result of the lazy unmapping in vmalloc.c.

One immediate one is that vm_unmap_aliases() will oops if you call it before vmalloc_init() is called, which can happen in the Xen case. RFC patch below.

Sure, we could do that. If you add an unlikely, and a __read_mostly, I'd ack it. Thanks for picking this up.

Quoted 4 lines

But the bigger problem I'm seeing is that despite calling vm_unmap_aliases() at the pertinent places, I'm still seeing errors resulting from stray aliases. Is it possible that vm_unmap_aliases() could be missing some, or not completely synchronous?

It's possible, but of course that would not be by design ;)

I've had another look over it, and nothing obvious comes to mind.

Actually, there may be a slight problem with the per-cpu KVA flushing (it doesn't clear the dirty map after flushing, so it would be possible to see the warning in vunmap_pte_range trigger, I'll have to fix that). But I can't see your problem yet.

It would be nice to narrow it down... Could you replace lazy_max_pages call with 0, then change the 3rd and 4th parameters of __purge_vmap_area_lazy in purge_vmap_area_lazy with 1 and 1 rather than 0 and 0?

Quoted 29 lines

Subject: vmap: cope with vm_unmap_aliases before vmalloc_init()

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

diff -r 42c8b29f7ccf mm/vmalloc.c

mm/vmalloc.c +5 -0
@@ -591,6 +591,8 @@
 
 #define VMAP_BLOCK_SIZE		(VMAP_BBMAP_BITS * PAGE_SIZE)
 
+static bool vmap_initialized = false;
+
 struct vmap_block_queue {
 	spinlock_t lock;
 	struct list_head free;
@@ -827,6 +829,9 @@
 	int cpu;
 	int flush = 0;
 
+	if (!vmap_initialized)
+		return;
+
 	for_each_possible_cpu(cpu) {
 		struct vmap_block_queue *vbq = &per_cpu(vmap_block_queue, cpu);
 		struct vmap_block *vb;

mm/vmalloc.c line 952

Quoted 8 lines
mm/vmalloc.c +2 -0
@@ -940,6 +945,8 @@
 		INIT_LIST_HEAD(&vbq->dirty);
 		vbq->nr_dirty = 0;
 	}
+
+	vmap_initialized = true;
 }
 
 void unmap_kernel_range(unsigned long addr, unsigned long size)

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

#2 lore raw
Quoted 5 lines · Nick Piggin
Quoted 3 lines · Jeremy Fitzhardinge

I've been having a few problems with Xen, I suspect as a result of the lazy unmapping in vmalloc.c.

Quoted 8 lines
Quoted 4 lines

One immediate one is that vm_unmap_aliases() will oops if you call it before vmalloc_init() is called, which can happen in the Xen case. RFC patch below.

Sure, we could do that. If you add an unlikely, and a __read_mostly, I'd ack it. Thanks for picking this up.

OK, will respin accordingly.

Quoted 11 lines
Quoted 5 lines

But the bigger problem I'm seeing is that despite calling vm_unmap_aliases() at the pertinent places, I'm still seeing errors resulting from stray aliases. Is it possible that vm_unmap_aliases() could be missing some, or not completely synchronous?

It's possible, but of course that would not be by design ;)

I've had another look over it, and nothing obvious comes to mind.

I found the problem and fixed it; I was just doing the operations in the wrong order.

Thanks,
J

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

#3 lore raw

PATCH 1/2 vmap: cope with vm_unmap_aliases before vmalloc_init()

In reply to Nick Piggin

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

---
mm/vmalloc.c | 7 +++++++

1 file changed, 7 insertions(+)

===================================================================

mm/vmalloc.c +0 -0
@@ -592,6 +592,8 @@
 

#define VMAP_BLOCK_SIZE (VMAP_BBMAP_BITS * PAGE_SIZE)

+static bool vmap_initialized __read_mostly = false; +

struct vmap_block_queue {
	spinlock_t lock;
	struct list_head free;
@@ -828,6 +830,9 @@
	int cpu;
	int flush = 0;

+ if (unlikely(!vmap_initialized))
+ return;
+

	for_each_possible_cpu(cpu) {
		struct vmap_block_queue *vbq = &per_cpu(vmap_block_queue, cpu);
		struct vmap_block *vb;
@@ -941,6 +946,8 @@
		INIT_LIST_HEAD(&vbq->dirty);
		vbq->nr_dirty = 0;
	}

+
+ vmap_initialized = true;
}

void unmap_kernel_range(unsigned long addr, unsigned long size)

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

#4 lore raw

Re: [PATCH 1/2] vmap: cope with vm_unmap_aliases before vmalloc_init()

Quoted 4 lines · Jeremy Fitzhardinge

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

Ping? Nick, Ingo: do you want to pick these up, or shall I send them to Linus myself?

Thanks,
J

Quoted 42 lines

---
mm/vmalloc.c | 7 +++++++

1 file changed, 7 insertions(+)

===================================================================

mm/vmalloc.c +7 -0
@@ -592,6 +592,8 @@
 
 #define VMAP_BLOCK_SIZE        (VMAP_BBMAP_BITS * PAGE_SIZE)
 
+static bool vmap_initialized __read_mostly = false;
+
 struct vmap_block_queue {
    spinlock_t lock;
    struct list_head free;
@@ -828,6 +830,9 @@
    int cpu;
    int flush = 0;
 
+    if (unlikely(!vmap_initialized))
+        return;
+
    for_each_possible_cpu(cpu) {
        struct vmap_block_queue *vbq = &per_cpu(vmap_block_queue, cpu);
        struct vmap_block *vb;
@@ -941,6 +946,8 @@
        INIT_LIST_HEAD(&vbq->dirty);
        vbq->nr_dirty = 0;
    }
+
+    vmap_initialized = true;
 }
 
 void unmap_kernel_range(unsigned long addr, unsigned long size)
Signature · 5 lines
-- 
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/

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

Applied
#5 lore raw

Re: [PATCH 1/2] vmap: cope with vm_unmap_aliases before vmalloc_init()

Quoted 8 lines · Jeremy Fitzhardinge
Quoted 4 lines · Jeremy Fitzhardinge

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

Ping? Nick, Ingo: do you want to pick these up, or shall I send them to Linus myself?

i've applied them to tip/core/urgent and will send them to Linus unless Nick or Andrew has objections.

Ingo

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

#6 lore raw

Re: [PATCH 1/2] vmap: cope with vm_unmap_aliases before vmalloc_init()

Quoted 5 lines · Ingo Molnar
Quoted 4 lines · Jeremy Fitzhardinge
Quoted 3 lines · Jeremy Fitzhardinge

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Ping? Nick, Ingo: do you want to pick these up, or shall I send them to Linus myself?

i've applied them to tip/core/urgent and will send them to Linus unless Nick or Andrew has objections.

Thanks, yeah ack from me on those. I'm generally expecting Andrew to pick up and merge mm patches, but I guess he wasn't cc'ed this time. Anyway, if Ingo gets them upstream, that would be fine too.

Thanks,
Nick

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

#7 lore raw

Re: [PATCH 1/2] vmap: cope with vm_unmap_aliases before vmalloc_init()

Quoted 11 lines · Nick Piggin
Quoted 10 lines · Ingo Molnar
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 4 lines · Jeremy Fitzhardinge

Xen can end up calling vm_unmap_aliases() before vmalloc_init() has been called. In this case its safe to make it a simple no-op.

Ping? Nick, Ingo: do you want to pick these up, or shall I send them to Linus myself?

Quoted 6 lines
Quoted 2 lines

i've applied them to tip/core/urgent and will send them to Linus unless Nick or Andrew has objections.

Thanks, yeah ack from me on those. I'm generally expecting Andrew to pick up and merge mm patches, but I guess he wasn't cc'ed this time. Anyway, if Ingo gets them upstream, that would be fine too.

linux-mm was Cc:-ed - but the crux of the changes was in Xen code, so i guess Andrew was looking at me and i was looking at him, expecting the other guy to do the work ;-)

anyway, it's all queued up and tested now.

Ingo

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

#8 lore raw

PATCH 2/2 xen: make sure stray alias mappings are gone before pinning

In reply to Nick Piggin

Xen requires that all mappings of pagetable pages are read-only, so that they can't be updated illegally. As a result, if a page is being turned into a pagetable page, we need to make sure all its mappings are RO.

If the page had been used for ioremap or vmalloc, it may still have left over mappings as a result of not having been lazily unmapped. This change makes sure we explicitly mop them all up before pinning the page.

Unlike aliases created by kmap, the there can be vmalloc aliases even for non-high pages, so we must do the flush unconditionally.

---

arch/x86/xen/enlighten.c |    5 +++--
arch/x86/xen/mmu.c       |    9 ++++++---
2 files changed, 9 insertions(+), 5 deletions(-)

===================================================================

arch/x86/xen/enlighten.c +0 -0
@@ -863,15 +863,16 @@
	if (PagePinned(virt_to_page(mm->pgd))) {
		SetPagePinned(page);
+		vm_unmap_aliases();
		if (!PageHighMem(page)) {
			make_lowmem_page_readonly(__va(PFN_PHYS((unsigned long)pfn)));
			if (level == PT_PTE && USE_SPLIT_PTLOCKS)
				pin_pagetable_pfn(MMUEXT_PIN_L1_TABLE, pfn);
-		} else
+		} else {
			/* make sure there are no stray mappings of
			   this page */
			kmap_flush_unused();
-			vm_unmap_aliases();
+		}
	}
}

===================================================================

arch/x86/xen/mmu.c +0 -0
@@ -840,13 +840,16 @@
   read-only, and can be pinned. */

static void __xen_pgd_pin(struct mm_struct *mm, pgd_t *pgd) {
+ vm_unmap_aliases();
+

	xen_mc_batch();

- if (xen_pgd_walk(mm, xen_pin_page, USER_LIMIT)) {
- /* re-enable interrupts for kmap_flush_unused */
+ if (xen_pgd_walk(mm, xen_pin_page, USER_LIMIT)) {
+ /* re-enable interrupts for flushing */

		xen_mc_issue(0);
+
		kmap_flush_unused();
-		vm_unmap_aliases();
+
		xen_mc_batch();
	}

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