Date: Tue, 20 Jul 2010 01:55:52 -0500 From: Alan Cox <alc@cs.rice.edu> To: "Jayachandran C." <c.jayachandran@gmail.com> Cc: Alan Cox <alc@cs.rice.edu>, freebsd-mips@freebsd.org Subject: Re: svn commit: r208589 - head/sys/mips/mips Message-ID: <4C454878.4070001@cs.rice.edu> In-Reply-To: <AANLkTimVNpvdeGkFcwhhuJJnA6u76-Fo2PwaiparZSK8@mail.gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
>>
>> 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.)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4C454878.4070001>