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

[Jayachandran C.]

On Tue, Jul 20, 2010 at 12:25 PM, Alan Cox <alc at cs.rice.edu> wrote:
> 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.

I have made the changes below, and the code is checked in with
revision r210327. I have tested this on XLR, but  let me know if this
causes any regression on other platform.

Also, many thanks to alc for patiently reviewing the changes.

JC.

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