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

[David Howells]

> > 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/