From owner-freebsd-mips@FreeBSD.ORG Wed Jul 21 18:19:41 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 AA6741065670 for ; Wed, 21 Jul 2010 18:19:41 +0000 (UTC) (envelope-from c.jayachandran@gmail.com) Received: from mail-vw0-f54.google.com (mail-vw0-f54.google.com [209.85.212.54]) by mx1.freebsd.org (Postfix) with ESMTP id 4C7BF8FC08 for ; Wed, 21 Jul 2010 18:19:40 +0000 (UTC) Received: by vws19 with SMTP id 19so9444578vws.13 for ; Wed, 21 Jul 2010 11:19:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=NQ4MtUB9IlJfDzx5Q68I4jMlAlUqQEKTgDVssUapS2U=; b=Efn+hsFbGT0B5W0l/zkcbOJpc/NMUOA8pA6xUGZ3kCUKDYyOW0BYirZMBo+Ze1aDZA EKME8AVIlBxektP4XPbnkslyNdbReAQymIfsoM/Bf/F1G/8OXN4zgm+ITcBd7J6AYMMq Iqnl+AC0ZUd9YCCxdP7Nmo1YRcnHRtr48VDNs= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=cRGA2+NxQL/p7pLkKbjePYDblyGJi7RjrijOZ2fsxmr3pC0Srq9IC2pQQksTSjy2BM QQCfx2SI1CFHEMqA7iFECI+w5mFDLGrUlst0PYq1Jti5ZLjQwKT6I59Md8ithQIrEdSS WI4w+OI++SYimHQeS9t1TTQk/FzflD1XdwmSY= MIME-Version: 1.0 Received: by 10.220.125.22 with SMTP id w22mr306465vcr.18.1279736379886; Wed, 21 Jul 2010 11:19:39 -0700 (PDT) Received: by 10.220.188.138 with HTTP; Wed, 21 Jul 2010 11:19:39 -0700 (PDT) In-Reply-To: <4C454878.4070001@cs.rice.edu> 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> <4C454878.4070001@cs.rice.edu> Date: Wed, 21 Jul 2010 23:49:39 +0530 Message-ID: From: "Jayachandran C." To: freebsd-mips@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Alan Cox 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: Wed, 21 Jul 2010 18:19:41 -0000 On Tue, Jul 20, 2010 at 12:25 PM, Alan Cox wrote: > Jayachandran C. wrote: >> >> On Wed, Jul 14, 2010 at 11:56 PM, Alan Cox wrote: >> [....] >> >>> >>> This makes sense. =A0I have the following requests. =A0All 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(): >>> >>> =A0mtx_lock(&vm_page_queue_free_mtx); >>> =A0if (cnt.v_free_count + cnt.v_cache_count > cnt.v_free_reserved || >>> =A0 =A0 =A0(page_req =3D=3D VM_ALLOC_SYSTEM && >>> =A0 =A0 =A0cnt.v_free_count + cnt.v_cache_count > cnt.v_interrupt_free_= min) || >>> =A0 =A0 =A0(page_req =3D=3D VM_ALLOC_INTERRUPT && >>> =A0 =A0 =A0cnt.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. =A0Only pmap_growkernel() should use >>> VM_ALLOC_INTERRUPT. =A0(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(). =A0(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. =A0(And add it to >>> vm_page.h.) >>> >>> 3. Make vm_phys_alloc_freelist_pages() extern. >>> >>> >>> >>>> >>>> Since I had originally added the UMA zone for =A0page 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. =A0Can you please >>> remove >>> these: >>> >> >> [...] >> >>> >>> FreeBSD style(9) requires parentheses around the "m": >>> >> >> [...] >> >>> >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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. =A0The MIPS page table pages will be allocated using this >> function from the KSEG0 freelist. >> - Move the page initialization code from =A0vm_phys_alloc_contig() to a >> new function vm_page_alloc_init(), and use this function to initialize >> pages in vm_page_alloc_freelist() too. >> - =A0Split the =A0vm_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. =A0After the following stylistic changes are made, > please go ahead and commit the patch. > > @@ -110,6 +110,7 @@ > #define =A0 =A0 =A0 =A0VM_MAXUSER_ADDRESS =A0 =A0 =A0((vm_offset_t)0x8000= 0000) > #define =A0 =A0 =A0 =A0VM_MIN_KERNEL_ADDRESS =A0 ((vm_offset_t)0xC0000000= ) > #define =A0 =A0 =A0 =A0VM_MAX_KERNEL_ADDRESS =A0 ((vm_offset_t)0xFFFFC000= ) > +#define =A0 =A0 =A0 =A0VM_HIGHMEM_ADDRESS =A0 =A0 =A0((vm_paddr_t)0x2000= 0000) > #endif > #if 0 > #define =A0 =A0 =A0 =A0KERNBASE =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(VM_MIN_KE= RNEL_ADDRESS) > > VM_HIGHMEM_ADDRESS is a physical address whereas the other constants defi= ned > here are virtual addresses. =A0I would move VM_HIGHMEM_ADDRESS to the sam= e > 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 > - =A0 =A0 =A0 static char zeropage[PAGE_SIZE]; > - > - =A0 =A0 =A0 KASSERT(size =3D=3D PAGE_SIZE, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ("pmap_ptpgzone_dtor: invalid size %d", siz= e)); > - =A0 =A0 =A0 KASSERT(bcmp(mem, zeropage, PAGE_SIZE) =3D=3D 0, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ("pmap_ptpgzone_dtor: freeing a non-zeroed = page")); > -#endif > + =A0 =A0 =A0 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) > { > ... > - =A0 =A0 =A0 if (va =3D=3D NULL) > + =A0 =A0 =A0 m =3D vm_page_alloc_freelist(VM_FREELIST_DEFAULT, 0, req); > + =A0 =A0 =A0 if (m =3D=3D NULL) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 st= ill > makes me a little queasy. =A0That queasiness would be lessened by using > VM_FREELIST_DIRECT here instead of VM_FREELIST_DEFAULT. =A0Moreover, the > notion of VM_FREELIST_DEFAULT may have to change with the addition of NUM= A > support, having and using VM_FREELIST_DIRECT may shield MIPS from those > changes. > > > =A0 =A0 =A0 m->pindex =3D index; > =A0 =A0 =A0 m->valid =3D VM_PAGE_BITS_ALL; > - =A0 =A0 =A0 m->wire_count =3D 1; > - =A0 =A0 =A0 if (!locked) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 vm_page_unlock_queues(); > - > > You can eliminate the assignment to "->valid". =A0This field is not meani= ngful > 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 *" > > > + =A0 =A0 =A0 =A0* Do not allocate reserved pages unless the req has aske= d 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 "(". =A0(The similar spaces in= some > of nearby declarations are historical artifacts that new code needn't > duplicate.) > >