From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S261484AbUKBQrM (ORCPT ); Tue, 2 Nov 2004 11:47:12 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261480AbUKBQoy (ORCPT ); Tue, 2 Nov 2004 11:44:54 -0500 Received: from mx1.redhat.com ([66.187.233.31]:39559 "EHLO mx1.redhat.com") by vger.kernel.org with ESMTP id S261701AbUKBQnk (ORCPT ); Tue, 2 Nov 2004 11:43:40 -0500 From: David Howells In-Reply-To: <20041102095418.GE5841@infradead.org> References: <20041102095418.GE5841@infradead.org> <20040401020550.GG3150@beast> <200411011930.iA1JUMQe023259@warthog.cambridge.redhat.com> To: Christoph Hellwig Cc: torvalds@osdl.org, akpm@osdl.org, davidm@snapgear.com, linux-kernel@vger.kernel.org, uclinux-dev@uclinux.org Subject: Re: [PATCH 14/14] FRV: Better mmap support in uClinux User-Agent: EMH/1.14.1 SEMI/1.14.5 (Awara-Onsen) FLIM/1.14.5 (Demachiyanagi) APEL/10.6 Emacs/21.3 (i386-redhat-linux-gnu) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.5 - "Awara-Onsen") Content-Type: text/plain; charset=US-ASCII Date: Tue, 02 Nov 2004 16:43:25 +0000 Message-ID: <26128.1099413805@redhat.com> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org > > diff -uNr /warthog/kernels/linux-2.6.10-rc1-bk10/fs/proc/base.c linux-2.6.10-rc1-bk10-frv/fs/proc/base.c > ... > > please split this up into a mmu and a no-mmu function, in a mmu and nommu > file respectively. What would you say to this function being moved into the mm/ directory? Perhaps in mm/proc.c. > > +/* list of shareable VMAs */ > > +LIST_HEAD(shareable_vma_list); > > +DECLARE_RWSEM(shareable_vma_sem); > > the should both be static I'm intending to do a /proc file to allow this list to be viewed. > > + if (flags & MAP_FIXED || addr) { > > +// printk("can't do fixed-address/overlay mmap of RAM\n"); > > please avoid C++-style comments in core kernel code. I should make this #ifdef'd on a CONFIG_DEBUG_ option. > > +#ifdef CONFIG_MMU > > vma->vm_ops = &generic_file_vm_ops; > > +#endif > > please find a nice way to abstract this out without the ifdefs here. Why? The #ifdef tells you exactly what you need to know. This _is_ the right way to do it. Doing otherwise just makes the code less obvious. However, if you can come up with a better way, I'll review the patch for you. David