Re: [PATCH 14/14] FRV: Better mmap support in uClinux

[Christoph Hellwig <hch@infradead.org>]

On Tue, Nov 02, 2004 at 04:43:25PM +0000, David Howells wrote:
> 
> > > 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.

I think it should stay in procfs.  And we already have an mmu and a no-mmu
function there.

> > > +/* 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.

So provide a proper iterator once you do that.  Global lists are very
bad coding style.

> > >  	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.

shmem_zero_setup is already duplicated in shmem.c and tiny-shmem.c just
with different vm_ops.

So yes, please provide a set_devzero_vmops macro that is NULL for no-MMU
ports and allows to merge the existing two copies of the function.