From owner-freebsd-mips@FreeBSD.ORG Tue Jul 20 06:56:03 2010 Return-Path: Delivered-To: freebsd-mips@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 55E081065673 for ; Tue, 20 Jul 2010 06:56:03 +0000 (UTC) (envelope-from alc@cs.rice.edu) Received: from mail.cs.rice.edu (mail.cs.rice.edu [128.42.1.31]) by mx1.freebsd.org (Postfix) with ESMTP id 143FE8FC16 for ; Tue, 20 Jul 2010 06:56:02 +0000 (UTC) Received: from mail.cs.rice.edu (localhost.localdomain [127.0.0.1]) by mail.cs.rice.edu (Postfix) with ESMTP id 85E342C2ACE; Tue, 20 Jul 2010 01:56:02 -0500 (CDT) X-Virus-Scanned: by amavis-2.4.0 at mail.cs.rice.edu Received: from mail.cs.rice.edu ([127.0.0.1]) by mail.cs.rice.edu (mail.cs.rice.edu [127.0.0.1]) (amavisd-new, port 10024) with LMTP id BPTfpZ4DahVo; Tue, 20 Jul 2010 01:55:54 -0500 (CDT) Received: from adsl-216-63-78-18.dsl.hstntx.swbell.net (adsl-216-63-78-18.dsl.hstntx.swbell.net [216.63.78.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.cs.rice.edu (Postfix) with ESMTP id 6664D2C2A91; Tue, 20 Jul 2010 01:55:53 -0500 (CDT) Message-ID: <4C454878.4070001@cs.rice.edu> Date: Tue, 20 Jul 2010 01:55:52 -0500 From: Alan Cox User-Agent: Thunderbird 2.0.0.24 (X11/20100501) MIME-Version: 1.0 To: "Jayachandran C." References: <4C07E07B.9060802@cs.rice.edu> <4C09345F.9040300@cs.rice.edu> <4C0D2BEA.6060103@cs.rice.edu> <4C0D3F40.2070101@cs.rice.edu> <20100607202844.GU83316@deviant.kiev.zoral.com.ua> <4C0D64B7.7060604@cs.rice.edu> <4C0DE424.9080601@cs.rice.edu> <4C3E0144.50402@cs.rice.edu> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Alan Cox , freebsd-mips@freebsd.org Subject: Re: svn commit: r208589 - head/sys/mips/mips X-BeenThere: freebsd-mips@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to MIPS List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Jul 2010 06:56:03 -0000 Jayachandran C. wrote: > On Wed, Jul 14, 2010 at 11:56 PM, Alan Cox 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.)