On Fri, Jan 28, 2005 at 01:38:22PM -0600, Tom Zanussi wrote:
> +extern void * alloc_rchan_buf(unsigned long size,
> + struct page ***page_array,
> + int *page_count);
> +extern void free_rchan_buf(void *buf,
> + struct page **page_array,
> + int page_count);
As these will be "polluting" the global namespace of the kernel, could
you add "relayfs_" to the front of them?
Also, any reason why you haven't exported the fops of relayfs so that
others can use this in their filesystems (like proc and debugfs)?
> +/**
> + * rchan_create_file - create relay file, including parent directories
> + * @chanpath: path to file, including filename
> + * @dentry: result dentry
> + * @data: data to associate with the file
> + *
> + * Returns 0 if successful, negative otherwise.
> + */
> +static inline int rchan_create_file(const char *chanpath,
> + struct dentry **dentry,
> + struct rchan_buf *data)
> +{
> + int err;
> + const char * fname;
> + struct dentry *topdir;
> +
> + err = rchan_create_dir(chanpath, &fname, &topdir);
> + if (err && (err != -EEXIST))
> + return err;
> +
> + err = relayfs_create_file(fname, topdir, dentry, data, S_IRUSR);
> +
> + return err;
> +}
Why not just return the * to the dentry? Gets rid of one paramater...
Also, if a path is passed to this function, when the "channel" is
finally torn down, that path is not removed, right? Or did I miss that
in the code somewhere?
> +/**
> + * check_attribute_flags - check sanity of channel attributes
> + * @flags: channel attributes
> + *
> + * Returns 0 if successful, negative otherwise.
> + */
> +static inline int check_attribute_flags(u32 *attribute_flags)
> +{
> + u32 flags = *attribute_flags;
> +
> + if ((flags & RELAY_MODE_CONTINUOUS) &&
> + (flags & RELAY_MODE_NO_OVERWRITE))
> + return -EINVAL; /* Can't have it both ways */
> +
> + if (!(flags & RELAY_MODE_CONTINUOUS) &&
> + !(flags & RELAY_MODE_NO_OVERWRITE))
> + *attribute_flags |= RELAY_MODE_CONTINUOUS; /* Default */
> +
> + return 0;
> +}
Why be nice to your caller? Just force them to get the flags right, and
if not, error out, you don't have to provide a "default" as this is a
required parameter to start up a channel, right?
> +/*
> + * Per-cpu relay channel buffer
> + */
> +struct rchan_buf
> +{
> + void *start; /* start of channel buffer */
> + void *data; /* start of current sub-buffer */
> + local_t offset; /* current offset into sub-buffer */
> + unsigned bufsize; /* sub-buffer size */
> + unsigned alloc_size; /* total buffer size allocated */
> + unsigned nbufs; /* number of sub-buffers */
> + unsigned bufs_produced; /* count of sub-buffers produced */
> + unsigned bufs_consumed; /* count of sub-buffers consumed */
> + wait_queue_head_t read_wait; /* reader wait queue */
> + struct work_struct wake_readers; /* reader wake-up work struct */
> + struct rchan_callbacks *cb; /* client callbacks */
> + struct work_struct work; /* remove file work struct */
> + u32 flags; /* relay channel attributes */
> + struct dentry *dentry; /* channel file dentry */
> + atomic_t refcount; /* channel refcount */
Please use struct kref for this.
> + struct page **page_array; /* array of current buffer pages */
> + int page_count; /* number of current buffer pages */
> + unsigned *padding; /* padding counts per sub-buffer */
> + unsigned *commit; /* commit counts per sub-buffer */
> + int finalized; /* buffer has been finalized */
> +} ____cacheline_aligned;
thanks,
greg k-h
-
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/
