Date: Wed, 21 Jul 2010 23:49:39 +0530 From: "Jayachandran C." <c.jayachandran@gmail.com> To: freebsd-mips@freebsd.org Cc: Alan Cox <alc@cs.rice.edu> Subject: Re: svn commit: r208589 - head/sys/mips/mips Message-ID: <AANLkTilgaPo4NZ9KD5-xWXfFqUkvTq3Lgjj1hSUfK8OT@mail.gmail.com> In-Reply-To: <4C454878.4070001@cs.rice.edu> References: <AANLkTimIa3jmBPMhWIOcY6DenGpZ2ZYmqwDTWspVx0-u@mail.gmail.com> <4C07E07B.9060802@cs.rice.edu> <AANLkTimjyPc_AXKP1yaJaF1BN7CAGBeNikVzcp9OCb4P@mail.gmail.com> <4C09345F.9040300@cs.rice.edu> <AANLkTinmFOZY3OlaoKStxlNIRBt2G2I4ILkQ1P0CjozG@mail.gmail.com> <4C0D2BEA.6060103@cs.rice.edu> <AANLkTikZxx_30H9geHvZYkYd0sE-wiuZljEd0PAi14ca@mail.gmail.com> <4C0D3F40.2070101@cs.rice.edu> <20100607202844.GU83316@deviant.kiev.zoral.com.ua> <4C0D64B7.7060604@cs.rice.edu> <AANLkTilBxdXxXrWC1cAT0wX9ubmFrvaAdk4feG6PwDYQ@mail.gmail.com> <4C0DE424.9080601@cs.rice.edu> <AANLkTinzIUOykgwtHlJ2vDwYS9as3ha_BYiy_qRd5h2Q@mail.gmail.com> <AANLkTimWh77REpi3ZD0BOihZit5qKNYNbtAx5PWQRYBX@mail.gmail.com> <AANLkTimRND9G9udHWzhN06wTJcRCN-OEjPPXctOoyj9_@mail.gmail.com> <AANLkTim0LyBsdOHOoBeORRSiJZsWO3eKIQqwM8gAmZ9-@mail.gmail.com> <AANLkTikH7PZ9ly0kRuMkfrkjJ0jYLUzCHy7ea0_vneWf@mail.gmail.com> <4C3E0144.50402@cs.rice.edu> <AANLkTimVNpvdeGkFcwhhuJJnA6u76-Fo2PwaiparZSK8@mail.gmail.com> <4C454878.4070001@cs.rice.edu>
index | next in thread | previous in thread | raw e-mail
On Tue, Jul 20, 2010 at 12:25 PM, Alan Cox <alc@cs.rice.edu> wrote:
> Jayachandran C. wrote:
>>
>> On Wed, Jul 14, 2010 at 11:56 PM, Alan Cox <alc@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@. 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.)
>
>
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTilgaPo4NZ9KD5-xWXfFqUkvTq3Lgjj1hSUfK8OT>