Re: UVM patch for PR port-xen/45975

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Sat, Feb 18, 2012 at 09:30:25AM -0800, Chuck Silvers wrote:
> hi manuel,
> 
> this is fine in general, but there are a few details:
> 
>  - it would be better to remove the mappings before freeing the page
>    on all platforms, not just xen.  that's just a good practice.
>    if you're concerned about a potential performance impact,
>    you could process the pages in batches of 16 or so using a little
>    array on the stack, which should handle most operations in one batch.
>    I did a quick experiment just now and found that most calls
>    to uvm_km_pgremove_intrsafe() are for 4 pages or less.
>    (I wonder why those aren't using some higher-level cache?)
>    about 10% of the calls were for 16 pages, and none were larger.

I'll look at this. I also prefer to have the same code for Xen and
non-Xen. One of the reasons to do it this way was to have the less
possible impact on non-Xen kernels, for pullup to the release branch.
But maybe it's not a problem.
For the performance impact, it's probably non-measurable on a x86
system, but other pmap.c have what looks like expensive cache/mmu
operation at the end of pamp_kremove(). This is where a loop calling
pmap_kremove() for one page can have a significant impact.
I initially though about building a list of pages to free, which
is also not completely free. If an array of 16 is enough to cover most of
the calls, things are much better :)

> 
>  - the calls to pmap_update() should be moved along with the calls to
>    pmap_kremove().

But not all pmap_kremove() are followed by a pmap_update() in the initial code.
Is it a bug ?

> 
>  - a non-xen kernel with the patch dies during boot with:
> 
> panic: kernel diagnostic assertion "!pmap_extract(pmap_kernel(), loopva, 
> NULL)" failed: file "/home/chs/netbsd/src/sys/uvm/uvm_km.c", line 749 
> loopva=0xffff80001d1df000 loopsize=0x3000 vmem=0xffffffff80f01ae8

Ops. I can see why; we have start == end at the end of the loop.
We need to keep the start value. Restructuring the code to batch
calls to pmap_update() will probably fix it too :)

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--