svn commit: r208589 - head/sys/mips/mips

[Alan Cox]

Jayachandran C. wrote:
> On Wed, Jul 14, 2010 at 11:56 PM, Alan Cox <alc at cs.rice.edu> wrote:
> [....]
>   
>> This makes sense.  I have the following requests.  All but 1.a. are simple,
>> mechanical changes.
>>
>> 1. Move vm_phys_page_alloc() to vm_page.c and rename it to
>> vm_page_alloc_freelist().
>>
>> 1.a. The new vm_page_alloc_freelist() should really have a "req" parameter
>> like vm_page_alloc() and duplicate the following from vm_page_alloc():
>>
>>   mtx_lock(&vm_page_queue_free_mtx);
>>   if (cnt.v_free_count + cnt.v_cache_count > cnt.v_free_reserved ||
>>       (page_req == VM_ALLOC_SYSTEM &&
>>       cnt.v_free_count + cnt.v_cache_count > cnt.v_interrupt_free_min) ||
>>       (page_req == VM_ALLOC_INTERRUPT &&
>>       cnt.v_free_count + cnt.v_cache_count > 0)) {
>>
>> In essence, user page table page allocation shouldn't be allowed to allocate
>> the last available page.  Only pmap_growkernel() should use
>> VM_ALLOC_INTERRUPT.  (See either the amd64 or i386 pmap.)
>>
>> You could also drop the "pool" parameter and pass VM_FREEPOOL_DIRECT to
>> vm_phys_alloc_freelist_pages() from vm_page_alloc_freelist().  (This is
>> consistent with what vm_page_alloc() does for VM_ALLOC_NOOBJ.)
>>
>> 2. Move vm_page_alloc_init() to vm_page.c as well.  (And add it to
>> vm_page.h.)
>>
>> 3. Make vm_phys_alloc_freelist_pages() extern.
>>
>>
>>     
>>> Since I had originally added the UMA zone for  page table
>>> pages for MIPS, which has the issues you had noted, I would like to
>>> fix this...
>>>       
>> The patch introduces a few unnecessary blank lines.  Can you please remove
>> these:
>>     
> [...]
>   
>> FreeBSD style(9) requires parentheses around the "m":
>>     
> [...]
>   
>> +                       return m;
>>     
>
> [freebsd-mips cc-ed, for comments on MIPS side]
>
> Here's is the updated patch with the review comments taken care of. I
> have been testing this on MIPS, could not see any regression so far.
>
> --
> Redo the page table page allocation on MIPS, based on suggestion from
> alc at . The current UMA zone based allocation can be replaced by a
> scheme that creates a new free list for the KSEG0 region, and a new
> function in sys/vm to allocate pages from a specific free page list.
>
> The changes are :
> - Revert the earlier changes in MIPS pmap.c that added UMA zone for
> page table pages.
> - Add a new freelist VM_FREELIST_HIGHMEM to vmparam.h for memory that
> is not directly mapped (in 32bit compilation). Normal page allocations
> will first try the HIGHMEM freelist and then the default freelist
> (which is for the KSEG0 direct mapped memory).
> - Add a new function 'vm_page_t vm_page_alloc_freelist(int flind, int
> order, int req)' to vm/vm_page.c to allocate a page from a specified
> freelist.  The MIPS page table pages will be allocated using this
> function from the KSEG0 freelist.
> - Move the page initialization code from  vm_phys_alloc_contig() to a
> new function vm_page_alloc_init(), and use this function to initialize
> pages in vm_page_alloc_freelist() too.
> -  Split the  vm_phys_alloc_pages(pool, order) to create
> vm_phys_alloc_freelist_pages(int flind, int pool, int order), and use
> this function from both vm_page_alloc_freelist() and
> vm_phys_alloc_pages().
> --
>   

The patch looks good.  After the following stylistic changes are made, 
please go ahead and commit the patch.

@@ -110,6 +110,7 @@
 #define        VM_MAXUSER_ADDRESS      ((vm_offset_t)0x80000000)
 #define        VM_MIN_KERNEL_ADDRESS   ((vm_offset_t)0xC0000000)
 #define        VM_MAX_KERNEL_ADDRESS   ((vm_offset_t)0xFFFFC000)
+#define        VM_HIGHMEM_ADDRESS      ((vm_paddr_t)0x20000000)
 #endif
 #if 0
 #define        KERNBASE                (VM_MIN_KERNEL_ADDRESS)

VM_HIGHMEM_ADDRESS is a physical address whereas the other constants 
defined here are virtual addresses.  I would move VM_HIGHMEM_ADDRESS to 
the same block of definitions where VM_FREELIST_HIGHMEM is defined.

 static void
-pmap_ptpgzone_dtor(void *mem, int size, void *arg)
+pmap_grow_pte_page_cache()
 {
-#ifdef INVARIANTS
-       static char zeropage[PAGE_SIZE];
-
-       KASSERT(size == PAGE_SIZE,
-               ("pmap_ptpgzone_dtor: invalid size %d", size));
-       KASSERT(bcmp(mem, zeropage, PAGE_SIZE) == 0,
-               ("pmap_ptpgzone_dtor: freeing a non-zeroed page"));
-#endif
+       vm_contig_grow_cache(3, 0, MIPS_KSEG0_LARGEST_PHYS);
 }

Style(9) calls for a blank line between the opening "{" and the 
"vm_contig_grow_cache()" call.


 static vm_page_t
-pmap_alloc_pte_page(pmap_t pmap, unsigned int index, int wait, 
vm_offset_t *vap)
+pmap_alloc_pte_page(unsigned int index, int req)
 {
...
-       if (va == NULL)
+       m = vm_page_alloc_freelist(VM_FREELIST_DEFAULT, 0, req);
+       if (m == NULL)
                return (NULL);
 
For clarity of intent here, I would suggest #define'ing an alias for 
VM_FREELIST_DEFAULT called VM_FREELIST_DIRECT in vmparam.h and use that 
alias here.

While having VM_FREELIST_DEFAULT defined as a non-zero value works, it 
still makes me a little queasy.  That queasiness would be lessened by 
using VM_FREELIST_DIRECT here instead of VM_FREELIST_DEFAULT.  Moreover, 
the notion of VM_FREELIST_DEFAULT may have to change with the addition 
of NUMA support, having and using VM_FREELIST_DIRECT may shield MIPS 
from those changes.


        m->pindex = index;
        m->valid = VM_PAGE_BITS_ALL;
-       m->wire_count = 1;
-       if (!locked)
-               vm_page_unlock_queues();
-

You can eliminate the assignment to "->valid".  This field is not 
meaningful for VM_ALLOC_NOOBJ pages, like page table pages.


 vm_page_t vm_phys_alloc_pages(int pool, int order);
+vm_page_t vm_phys_alloc_freelist_pages(int flind, int pool, int order);

Please swap the above two declarations so that alphabetical ordering is 
preserved.


+ * The caller has to drop the vnode retuned in drop, if it is not NULL.

"retuned" should be "returned"


+void
+vm_page_alloc_init(vm_page_t m, struct vnode **drop)

I think that the patch would be simpler if vm_page_alloc_init() returned 
a "struct vnode *"


+        * Do not allocate reserved pages unless the req has asked for it

Please add a period to the end of the sentence.


+void vm_page_alloc_init (vm_page_t, struct vnode **);

Please remove the space between "init" and "(".  (The similar spaces in 
some of nearby declarations are historical artifacts that new code 
needn't duplicate.)