vm_unmap_aliases and Xen

vm_unmap_aliases and Xen

[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.

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.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
diff -r 42c8b29f7ccf mm/vmalloc.c
--- a/mm/vmalloc.c	Wed Oct 22 12:43:39 2008 -0700
+++ b/mm/vmalloc.c	Wed Oct 22 21:39:00 2008 -0700
@@ -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)

Re: vm_unmap_aliases and Xen

[Nick Piggin]

On Friday 24 October 2008 10:48, Jeremy Fitzhardinge wrote:
> 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.


> 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?


> 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.
>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> diff -r 42c8b29f7ccf mm/vmalloc.c
> --- a/mm/vmalloc.c	Wed Oct 22 12:43:39 2008 -0700
> +++ b/mm/vmalloc.c	Wed Oct 22 21:39:00 2008 -0700
> @@ -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)

Re: vm_unmap_aliases and Xen

[Jeremy Fitzhardinge]

Nick Piggin wrote:
> On Friday 24 October 2008 10:48, Jeremy Fitzhardinge wrote:
>   
>> 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.
>   

OK, will respin accordingly.

>> 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

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

[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.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 mm/vmalloc.c |    7 +++++++
 1 file changed, 7 insertions(+)

===================================================================
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -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)

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

[Jeremy Fitzhardinge]

Jeremy Fitzhardinge wrote:
> 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.
>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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

Thanks,
    J

> ---
> mm/vmalloc.c |    7 +++++++
> 1 file changed, 7 insertions(+)
>
> ===================================================================
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -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 from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Jeremy Fitzhardinge wrote:
>> 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.
>>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>
> 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

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

[Nick Piggin]

On Thursday 06 November 2008 21:02, Ingo Molnar wrote:
> * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> > Jeremy Fitzhardinge wrote:
> >> 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.
> >>
> >> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >
> > 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

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

[Ingo Molnar]

* Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Thursday 06 November 2008 21:02, Ingo Molnar wrote:
> > * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> > > Jeremy Fitzhardinge wrote:
> > >> 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.
> > >>
> > >> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > >
> > > 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.

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

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

[Jeremy Fitzhardinge]

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.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 arch/x86/xen/enlighten.c |    5 +++--
 arch/x86/xen/mmu.c       |    9 ++++++---
 2 files changed, 9 insertions(+), 5 deletions(-)

===================================================================
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -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();
+		}
 	}
 }
 
===================================================================
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -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();
 	}