Messages in this thread | | | Date | Fri, 4 Feb 2005 21:10:14 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCH] relayfs redux, part 3 |
| |
First set of comments. Mostly ignores the actual filesystem sematics bits, that'll come next.
> +# > +# relayfs Makefile > +#
probably superflous comment ;-)
> + mem = vmap(*page_array, *page_count, GFP_KERNEL, PAGE_KERNEL);
Do you really need to vmap it, and eat up vmallocspace and TLB entries? Maybe some simple arithmetics on a page array could replace it?
> +#include <linux/smp_lock.h>
don't think you need this one.
> +/** > + * relayfs_open - open file op for relayfs files > + * @inode: the inode > + * @filp: the file > + * > + * Associates the channel buffer with the file, and increments the > + * channel buffer refcount. > + */ > +int relayfs_open(struct inode *inode, struct file *filp) > +{ > + struct rchan_buf *buf; > + int retval = 0; > + > + if (inode->u.generic_ip) {
Can inode->u.generic_ip ever be non-NULL? What about using ->alloc_inode and ->destroy_inode to have the rchan_buf allocated as part of the inode?
> + retval = buf->chan->cb->fileop_notify(buf, filp, RELAY_FILE_OPEN);
I think you should split ->fileop_notify into individual operations for each of the different events.
> +int relayfs_mmap(struct file *filp, struct vm_area_struct *vma) > +{ > + struct rchan_buf *buf = filp->private_data; > + > + return relay_mmap_buffer(buf, vma); > +}
> +#include <asm/io.h>
What do you need this one for?
> +#include <asm/current.h>
should already be there through <linux/sched.h>
> +#include <asm/bitops.h>
please use <linux/bitops.h>
> +#include <asm/pgtable.h>
what do you need this one for?
> +#include <asm/hardirq.h>
always use <linux/hardirq.h> instead
> +/* \begin{Code inspired from BTTV driver} */
...
> + while (size > 0) { > + page = kvirt_to_pa(pos); > + if (remap_pfn_range(vma, start, page >> PAGE_SHIFT, PAGE_SIZE, PAGE_SHARED)) > + return -EAGAIN; > + start += PAGE_SIZE; > + pos += PAGE_SIZE; > + size -= PAGE_SIZE; > + } > + > + return 0; > +}
Using remap_pfn_range on normal kernel memory and the PageReserved flags, etc.. is not nice. You'd better implement a ->nopage handler that just does a simple array lookup and install the page. And if you really care about the little time this spends in the pagefault path you can implement ->populate and support MAP_POPULATE mmaps :)
> +static struct rchan_buf *rchan_create_buf(struct rchan *chan) > +{ > + struct page **page_array; > + int page_count; > + struct rchan_buf *buf; > + > + if ((buf = kcalloc(1, sizeof(struct rchan_buf), GFP_KERNEL)) == NULL) > + return NULL;
this doesn't fir the normal linux style. Whic you use two lines below:
> + > + buf->padding = kmalloc(chan->n_subbufs * sizeof(unsigned *), GFP_KERNEL); > + if (buf->padding == NULL) > + goto free_buf;
> + if ((buf->start = relay_alloc_rchan_buf(chan->alloc_size, &page_array, &page_count)) == NULL) {
but here again not.
> + buf->page_array = NULL; > + buf->page_count = 0; > + goto free_commit;
also this would be cleaner if placed at another goto label at the end
> + if ((buf = rchan_create_buf(chan)) == NULL) > + return NULL;
so this style is used in various places. Please try to make it fit normal style everywhere.
Also you're using foo == NULL a lot while we mostly use !foo. This isn't important in anyway, but would make reading a little easier.
> +#define relay_get_buf(chan, cpu) (chan->buf[cpu]) > +#define relay_get_padding(buf, subbuf_idx) (buf->padding[subbuf_idx]) > +#define relay_get_commit(buf, subbuf_idx) (buf->commit[subbuf_idx])
I don't think these macros help following the code.
> + unsigned long flags; > + struct rchan_buf *buf = relay_get_buf(chan, smp_processor_id()); > + > + local_irq_save(flags);
you need to place the relay_get_buf inside the locked region, so that the return value from smp_processor_id() is stable.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |