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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
