Now with relayfs integrated and the relay_file_operations exported for
use by other file systems, I wonder what people think about adding in a
sysfs attribute for setting up channel buffers.
The conventional relayfs doesn't make a lot of sense for the use cases
where there are multiple devices to stream data from, particularly if
they're already mapped out through the driver model. Rather than
duplicating device enumeration, simply adding this as an attribute seems
to work reasonably well.
Tom did some work on the rchan_callbacks for more easily implementing
relay files in other file systems, and it would be nice to use this in a
non-debug context, without duplicating device enumeration in multiple
locations.
---
fs/sysfs/Makefile | 1
fs/sysfs/dir.c | 26 ++++++++++++++++++
fs/sysfs/inode.c | 5 +++
fs/sysfs/relay.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/sysfs.h | 27 ++++++++++++++++++-
5 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/fs/sysfs/Makefile b/fs/sysfs/Makefile
index 7a1ceb9..676bf3c 100644
--- a/fs/sysfs/Makefile
+++ b/fs/sysfs/Makefile
@@ -4,3 +4,4 @@
obj-y := inode.o file.o dir.o symlink.o mount.o bin.o \
group.o
+obj-$(CONFIG_RELAYFS_FS) += relay.o
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 49bd219..379daa3 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -87,6 +87,20 @@ static int init_file(struct inode * inod
return 0;
}
+#ifdef CONFIG_RELAYFS_FS
+static int init_relay_file(struct inode * inode)
+{
+ extern struct file_operations relay_file_operations;
+ inode->i_fop = &relay_file_operations;
+ return 0;
+}
+#else
+static int init_relay_file(struct inode * inode)
+{
+ return -EINVAL;
+}
+#endif
+
static int init_symlink(struct inode * inode)
{
inode->i_op = &sysfs_symlink_inode_operations;
@@ -165,6 +179,7 @@ int sysfs_create_dir(struct kobject * ko
static int sysfs_attach_attr(struct sysfs_dirent * sd, struct dentry * dentry)
{
struct attribute * attr = NULL;
+ struct relay_attribute * rel_attr = NULL;
struct bin_attribute * bin_attr = NULL;
int (* init) (struct inode *) = NULL;
int error = 0;
@@ -172,6 +187,10 @@ static int sysfs_attach_attr(struct sysf
if (sd->s_type & SYSFS_KOBJ_BIN_ATTR) {
bin_attr = sd->s_element;
attr = &bin_attr->attr;
+ } else if (sd->s_type & SYSFS_KOBJ_RELAY_ATTR) {
+ rel_attr = sd->s_element;
+ attr = &rel_attr->attr;
+ init = init_relay_file;
} else {
attr = sd->s_element;
init = init_file;
@@ -189,6 +208,13 @@ static int sysfs_attach_attr(struct sysf
dentry->d_inode->i_size = bin_attr->size;
dentry->d_inode->i_fop = &bin_fops;
}
+
+ if (rel_attr) {
+ unsigned int size = rel_attr->subbuf_size * rel_attr->n_subbufs;
+ dentry->d_inode->i_size = ((size - 1) & PAGE_MASK) + PAGE_SIZE;
+ dentry->d_inode->u.generic_ip = rel_attr->rchan_buf;
+ }
+
dentry->d_op = &sysfs_dentry_ops;
d_rehash(dentry);
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 689f7bc..d41cb6d 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -174,6 +174,7 @@ const unsigned char * sysfs_get_name(str
{
struct attribute * attr;
struct bin_attribute * bin_attr;
+ struct relay_attribute * rel_attr;
struct sysfs_symlink * sl;
if (!sd || !sd->s_element)
@@ -192,6 +193,10 @@ const unsigned char * sysfs_get_name(str
bin_attr = sd->s_element;
return bin_attr->attr.name;
+ case SYSFS_KOBJ_RELAY_ATTR:
+ rel_attr = sd->s_element;
+ return rel_attr->attr.name;
+
case SYSFS_KOBJ_LINK:
sl = sd->s_element;
return sl->link_name;
--- linux-2.6/fs/sysfs/relay.c 2005-06-14 13:29:20.446304896 +0300
+++ linux-2.6/fs/sysfs/relay.c 2006-02-19 18:54:21.583577778 +0200
@@ -0,0 +1,70 @@
+/*
+ * relay.c - relay channel buffer support for sysfs
+ *
+ * Copyright (C) 2006 Paul Mundt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+#include <linux/relayfs_fs.h>
+#include <linux/namei.h>
+#include <linux/module.h>
+#include "sysfs.h"
+
+static struct dentry *
+sysfs_create_buf_file(const char *filename, struct dentry *dentry, int mode,
+ struct rchan_buf *buf, int *is_global)
+{
+ struct relay_attribute *attr = (struct relay_attribute *)dentry;
+ struct dentry *parent = attr->kobj->dentry;
+ int ret;
+
+ attr->attr.mode = mode;
+ attr->rchan_buf = buf;
+
+ ret = sysfs_add_file(parent, &attr->attr, SYSFS_KOBJ_RELAY_ATTR);
+ if (unlikely(ret != 0))
+ return NULL;
+
+ dentry = lookup_one_len(filename, parent, strlen(filename));
+ if (IS_ERR(dentry))
+ sysfs_hash_and_remove(parent, filename);
+
+ return dentry;
+}
+
+static int sysfs_remove_buf_file(struct dentry *dentry)
+{
+ sysfs_hash_and_remove(dentry, dentry->d_name.name);
+ return 0;
+}
+
+struct rchan_callbacks sysfs_rchan_callbacks = {
+ .create_buf_file = sysfs_create_buf_file,
+ .remove_buf_file = sysfs_remove_buf_file,
+};
+
+int sysfs_create_relay_file(struct kobject *kobj, struct relay_attribute *attr)
+{
+ BUG_ON(!kobj || !kobj->dentry || !attr);
+
+ attr->kobj = kobj;
+
+ attr->rchan = relay_open(attr->attr.name, (void *)attr,
+ attr->subbuf_size, attr->n_subbufs,
+ &sysfs_rchan_callbacks);
+ if (IS_ERR(attr->rchan))
+ return PTR_ERR(attr->rchan);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(sysfs_create_relay_file);
+
+void sysfs_remove_relay_file(struct kobject *kobj, struct relay_attribute *attr)
+{
+ BUG_ON(!attr);
+
+ relay_close(attr->rchan);
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_relay_file);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 392da5a..2e75d49 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -60,6 +60,15 @@ struct bin_attribute {
struct vm_area_struct *vma);
};
+struct relay_attribute {
+ struct attribute attr;
+ struct rchan *rchan;
+ struct rchan_buf *rchan_buf;
+ struct kobject *kobj;
+ size_t subbuf_size;
+ size_t n_subbufs;
+};
+
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *,char *);
ssize_t (*store)(struct kobject *,struct attribute *,const char *, size_t);
@@ -81,7 +90,23 @@ struct sysfs_dirent {
#define SYSFS_KOBJ_ATTR 0x0004
#define SYSFS_KOBJ_BIN_ATTR 0x0008
#define SYSFS_KOBJ_LINK 0x0020
-#define SYSFS_NOT_PINNED (SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR | SYSFS_KOBJ_LINK)
+#define SYSFS_KOBJ_RELAY_ATTR 0x0040
+#define SYSFS_NOT_PINNED (SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR | \
+ SYSFS_KOBJ_LINK | SYSFS_KOBJ_RELAY_ATTR)
+
+#if defined(CONFIG_RELAYFS_FS) && defined(CONFIG_SYSFS)
+int sysfs_create_relay_file(struct kobject *, struct relay_attribute *);
+void sysfs_remove_relay_file(struct kobject *, struct relay_attribute *);
+#else
+static inline int sysfs_create_relay_file(struct kobject *kobj, struct relay_attribute *attr)
+{
+ return 0;
+}
+
+static inline void sysfs_remove_relay_file(struct kobject *kobj, struct relay_attribute *attr)
+{
+}
+#endif /* CONFIG_RELAYFS_FS && CONFIG_SYSFS */
#ifdef CONFIG_SYSFS
On Sun, Feb 19, 2006 at 07:17:48PM +0200, Paul Mundt wrote:
> Now with relayfs integrated and the relay_file_operations exported for
> use by other file systems, I wonder what people think about adding in a
> sysfs attribute for setting up channel buffers.
>
> The conventional relayfs doesn't make a lot of sense for the use cases
> where there are multiple devices to stream data from, particularly if
> they're already mapped out through the driver model. Rather than
> duplicating device enumeration, simply adding this as an attribute seems
> to work reasonably well.
>
> Tom did some work on the rchan_callbacks for more easily implementing
> relay files in other file systems, and it would be nice to use this in a
> non-debug context, without duplicating device enumeration in multiple
> locations.
Hmm, this actually makes a lot of sense. At this point we should probably
move the guts of relayfs into kernel/relay.c or something and add a different
config option for it. Given this and debugs we could probably kill the
separate relayfs filesystem implementation.
Note that Pat isn't the sysfs maintainer anymore :)
On Sun, Feb 19, 2006 at 07:17:48PM +0200, Paul Mundt wrote:
> Now with relayfs integrated and the relay_file_operations exported for
> use by other file systems, I wonder what people think about adding in a
> sysfs attribute for setting up channel buffers.
>
> The conventional relayfs doesn't make a lot of sense for the use cases
> where there are multiple devices to stream data from, particularly if
> they're already mapped out through the driver model. Rather than
> duplicating device enumeration, simply adding this as an attribute seems
> to work reasonably well.
>
> Tom did some work on the rchan_callbacks for more easily implementing
> relay files in other file systems, and it would be nice to use this in a
> non-debug context, without duplicating device enumeration in multiple
> locations.
Looks good, I like it. This properly handles the module owner stuff,
too, right?
And I agree with Christoph, with this change, you don't need a separate
relayfs mount anymore.
thanks,
greg k-h
On Sun, Feb 19, 2006 at 09:56:23AM -0800, Greg KH wrote:
> Note that Pat isn't the sysfs maintainer anymore :)
>
My mistake, I'll check MAINTAINERS instead of the file comments next
time.
> On Sun, Feb 19, 2006 at 07:17:48PM +0200, Paul Mundt wrote:
> > Now with relayfs integrated and the relay_file_operations exported for
> > use by other file systems, I wonder what people think about adding in a
> > sysfs attribute for setting up channel buffers.
>
> Looks good, I like it. This properly handles the module owner stuff,
> too, right?
>
Could you elaborate on which module owner issue you're referring to?
struct relay_attribute encapsulates a struct attribute, and it's handled
the same way as the other attribute types (I modelled it after
struct bin_attribute), and I don't see any places that I missed.
When setting up the relay attribute, it's just a matter of:
static struct relay_attribute dev_relay_attr = {
.attr = {
.owner = THIS_MODULE,
...
},
...
};
Let me know if I've missed anything.
> And I agree with Christoph, with this change, you don't need a separate
> relayfs mount anymore.
>
Yes, that's where I was going with this, but I figured I'd give the
relayfs people a chance to object to it going away first.
If with this in sysfs and simple handling through debugfs people are
content with the relay interface for whatever need, then getting rid of
relayfs entirely is certainly the best option. We certainly don't need
more pointless virtual file systems.
I'll work up a patch set for doing this as per Cristoph's kernel/relay.c
suggestion. Thanks for the feedback.
Paul Mundt writes:
> On Sun, Feb 19, 2006 at 09:56:23AM -0800, Greg KH wrote:
>
[...]
> > And I agree with Christoph, with this change, you don't need a separate
> > relayfs mount anymore.
> >
> Yes, that's where I was going with this, but I figured I'd give the
> relayfs people a chance to object to it going away first.
>
> If with this in sysfs and simple handling through debugfs people are
> content with the relay interface for whatever need, then getting rid of
> relayfs entirely is certainly the best option. We certainly don't need
> more pointless virtual file systems.
>
> I'll work up a patch set for doing this as per Cristoph's kernel/relay.c
> suggestion. Thanks for the feedback.
Considering that I recently offered to post a patch that would do
essentially the same thing, I can't have any objections to this. ;-)
But just to make sure I'm not missing anything in the patches, please
let me know if any of the following is incorrect. What they do is
remove the fs part of relayfs and move the remaining code into a
single file, while leaving everthing else basically intact i.e. the
relayfs kernel API remains the same and existing clients would only
need to make relatively minor changes:
- find a new home for their relay files i.e. sysfs, debufs or procfs.
- replace any relayfs-specific code with their counterparts in the new
filesystem i.e. directory creation/removal, non-relay ('control')
file creation/removal.
- change userspace apps to look for the relay files in the new
filesystem instead of relayfs e.g. change /relay/* to /sys/*
in the relay file pathnames.
Although I personally don't have any problems with doing this, I've
added some of the authors of current relayfs applications to the cc:
list in case they might have any objections to it. The major relayfs
applications I'm aware of are:
- blktrace, currently in the -mm tree. This could probably move its
relayfs files to sysfs using your new interface.
- LTT, not sure where LTT would want to move.
- systemtap. sytemtap uses relayfs as one possible transport. The
other is a proc-based transport, so logically it would make sense
for systemtap to move its relay files to /proc.
Tom
On Sun, Feb 19 2006, Tom Zanussi wrote:
> Paul Mundt writes:
> > On Sun, Feb 19, 2006 at 09:56:23AM -0800, Greg KH wrote:
> >
>
> [...]
>
> > > And I agree with Christoph, with this change, you don't need a separate
> > > relayfs mount anymore.
> > >
> > Yes, that's where I was going with this, but I figured I'd give the
> > relayfs people a chance to object to it going away first.
> >
> > If with this in sysfs and simple handling through debugfs people are
> > content with the relay interface for whatever need, then getting rid of
> > relayfs entirely is certainly the best option. We certainly don't need
> > more pointless virtual file systems.
> >
> > I'll work up a patch set for doing this as per Cristoph's kernel/relay.c
> > suggestion. Thanks for the feedback.
>
> Considering that I recently offered to post a patch that would do
> essentially the same thing, I can't have any objections to this. ;-)
>
> But just to make sure I'm not missing anything in the patches, please
> let me know if any of the following is incorrect. What they do is
> remove the fs part of relayfs and move the remaining code into a
> single file, while leaving everthing else basically intact i.e. the
> relayfs kernel API remains the same and existing clients would only
> need to make relatively minor changes:
>
> - find a new home for their relay files i.e. sysfs, debufs or procfs.
>
> - replace any relayfs-specific code with their counterparts in the new
> filesystem i.e. directory creation/removal, non-relay ('control')
> file creation/removal.
>
> - change userspace apps to look for the relay files in the new
> filesystem instead of relayfs e.g. change /relay/* to /sys/*
> in the relay file pathnames.
>
> Although I personally don't have any problems with doing this, I've
> added some of the authors of current relayfs applications to the cc:
> list in case they might have any objections to it. The major relayfs
> applications I'm aware of are:
>
> - blktrace, currently in the -mm tree. This could probably move its
> relayfs files to sysfs using your new interface.
blktrace just needs minor file location changes to work with this
scheme, so no problem for me.
I think the patch is a good idea, it's a lot nicer than a separate fs.
--
Jens Axboe
On Sun, Feb 19, 2006 at 11:29:23PM -0600, Tom Zanussi wrote:
> But just to make sure I'm not missing anything in the patches, please
> let me know if any of the following is incorrect. What they do is
> remove the fs part of relayfs and move the remaining code into a
> single file, while leaving everthing else basically intact i.e. the
> relayfs kernel API remains the same and existing clients would only
> need to make relatively minor changes:
>
> - find a new home for their relay files i.e. sysfs, debufs or procfs.
>
> - replace any relayfs-specific code with their counterparts in the new
> filesystem i.e. directory creation/removal, non-relay ('control')
> file creation/removal.
>
> - change userspace apps to look for the relay files in the new
> filesystem instead of relayfs e.g. change /relay/* to /sys/*
> in the relay file pathnames.
>
Yes, that's correct.
With the patch I posted in reply to Dave it's also possible to keep
the relayfs mount point that just wraps in to the new CONFIG_RELAY
abstraction. Going this route (in place of the last 2 patches of my
patch set) might make the migration a little less painful, but as there
are no in-tree users for the kernel side, it's debatable whether keeping
it around is worthwhile at all.
The changes needed both in the kernel and user space client side are
quite trivial anyways. Likewise it's also possible for the sysfs
attribute patch to be applied first with the rest outstanding to let
people start switching to the sysfs interface.
Either way, as there seems to be enough interest in this, it would be
nice to get it in to -mm for testing at least, people can follow
blktrace's example on working with CONFIG_RELAY.
The first 3 patches in my series can be used in addition to the
follow-patch to Dave for introducing the CONFIG_RELAY interface while
retaining legacy relayfs behaviour, so this might be the best way to go
for -mm testing. The later patches can be rolled in once the few users
have switched, if it's deemed that they're actually relevant as users.
I'm fine with either approach, as long as I can use CONFIG_RELAY without
having another mount point to deal with.
> Although I personally don't have any problems with doing this, I've
> added some of the authors of current relayfs applications to the cc:
> list in case they might have any objections to it. The major relayfs
> applications I'm aware of are:
>
> - blktrace, currently in the -mm tree. This could probably move its
> relayfs files to sysfs using your new interface.
>
Jens pointed out that this should be quite easy, so that shouldn't be an
issue.
> - LTT, not sure where LTT would want to move.
>
LTT is already quite invasive on the kernel side. If they're interested
in continually using relayfs rather than switching to something else,
then they can use the patch I posted to Dave for stripping relayfs down
to work with CONFIG_RELAY. This leaves relayfs implemented in 312 lines,
which seems painless enough for the people that care about the API being
consistent.
> - systemtap. sytemtap uses relayfs as one possible transport. The
> other is a proc-based transport, so logically it would make sense
> for systemtap to move its relay files to /proc.
Again, this is out-of-tree, and the kernel side changes to accomodate
some other file system are trivial.
If that's all the users, then fixing these up would seem to be the better
alternative to leaving in a wrapper file system that people might want to
use for future interfaces.
* Tom Zanussi ([email protected]) wrote:
> Paul Mundt writes:
> > On Sun, Feb 19, 2006 at 09:56:23AM -0800, Greg KH wrote:
> >
>
> [...]
>
> > > And I agree with Christoph, with this change, you don't need a separate
> > > relayfs mount anymore.
> > >
> > Yes, that's where I was going with this, but I figured I'd give the
> > relayfs people a chance to object to it going away first.
> >
> > If with this in sysfs and simple handling through debugfs people are
> > content with the relay interface for whatever need, then getting rid of
> > relayfs entirely is certainly the best option. We certainly don't need
> > more pointless virtual file systems.
> >
> > I'll work up a patch set for doing this as per Cristoph's kernel/relay.c
> > suggestion. Thanks for the feedback.
>
> Considering that I recently offered to post a patch that would do
> essentially the same thing, I can't have any objections to this. ;-)
>
> But just to make sure I'm not missing anything in the patches, please
> let me know if any of the following is incorrect. What they do is
> remove the fs part of relayfs and move the remaining code into a
> single file, while leaving everthing else basically intact i.e. the
> relayfs kernel API remains the same and existing clients would only
> need to make relatively minor changes:
>
> - find a new home for their relay files i.e. sysfs, debufs or procfs.
>
> - replace any relayfs-specific code with their counterparts in the new
> filesystem i.e. directory creation/removal, non-relay ('control')
> file creation/removal.
>
> - change userspace apps to look for the relay files in the new
> filesystem instead of relayfs e.g. change /relay/* to /sys/*
> in the relay file pathnames.
>
> Although I personally don't have any problems with doing this, I've
> added some of the authors of current relayfs applications to the cc:
> list in case they might have any objections to it. The major relayfs
> applications I'm aware of are:
>
> - LTT, not sure where LTT would want to move.
>
Hi,
LTTng currently uses some particular features from relayfs. I would like to make
sure that they will still be available.
* LTTng uses the "void *private" private data pointer extensively. It's very
useful to pass a kernel client specific data structure to the client
callbacks.
* LTTng does have its own ltt_poll and ltt_ioctl that are all what is needed to
control the interaction with the file (along with the relayfs mmap/unmap).
In this scenario, the sysfs relay attribute creation would look like :
- create an empty attr
- fill some of attr members
- sysfs_create_relay_file(kobj, attr);
(it will overwrite some attr members : kobj, rchan, rchan_buf)
* set specific LTTng file operations on the inode
* set the "private" field of the rchan structure.
The two operations marked with a * would need supplementary parameters to
sysfs_create_relay_file. Or is there something I missed ?
Mathieu
OpenPGP public key: http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Mon, Feb 20, 2006 at 08:05:56AM -0500, Mathieu Desnoyers wrote:
> * LTTng does have its own ltt_poll and ltt_ioctl that are all what is needed to
> control the interaction with the file (along with the relayfs mmap/unmap).
>
> In this scenario, the sysfs relay attribute creation would look like :
>
> - create an empty attr
> - fill some of attr members
> - sysfs_create_relay_file(kobj, attr);
> (it will overwrite some attr members : kobj, rchan, rchan_buf)
> * set specific LTTng file operations on the inode
defintily not on sysfs. sysfs doesn't allow drivers to modify the
file operations for good reasons. it'll probably work the same as-is
when you use the rely file operations on debugfs or a custom filesystem,
although you're code will have zero chance to get merged when it modifies
an existing file operations struct or adds an ioctl.
On Mon, Feb 20, 2006 at 08:05:56AM -0500, Mathieu Desnoyers wrote:
> LTTng currently uses some particular features from relayfs. I would like to make
> sure that they will still be available.
>
> * LTTng uses the "void *private" private data pointer extensively. It's very
> useful to pass a kernel client specific data structure to the client
> callbacks.
> * LTTng does have its own ltt_poll and ltt_ioctl that are all what is needed to
> control the interaction with the file (along with the relayfs mmap/unmap).
>
> In this scenario, the sysfs relay attribute creation would look like :
>
> - create an empty attr
> - fill some of attr members
> - sysfs_create_relay_file(kobj, attr);
> (it will overwrite some attr members : kobj, rchan, rchan_buf)
> * set specific LTTng file operations on the inode
> * set the "private" field of the rchan structure.
>
->rchan is set after sysfs_create_relay_file(), you can set
->rchan->private after it's created. All that happens in
sysfs_create_relay_file() as far as the relay interface is concerned is
wrapping relay_open(), where ->rchan is first allocated.
As far as setting specific file operations for the inode, that's
definitely not in line with the sysfs way of doing things. If you need
to do this, go with debugfs, or stick with the relayfs patch on top of
CONFIG_RELAY as this stuff is clearly not going to be merged into
mainline as Cristoph also pointed out.
debugfs gives you roughly all of the same functionality, and the kernel
side implementation for what LTTng will need should be quite trivial. LTT
is arguably a debugging thing anyways, so debugfs seems like a more
appropriate match than trying to beat sysfs into submission with this
workload.
* Paul Mundt ([email protected]) wrote:
> On Mon, Feb 20, 2006 at 08:05:56AM -0500, Mathieu Desnoyers wrote:
> > LTTng currently uses some particular features from relayfs. I would like to make
> > sure that they will still be available.
> >
> > * LTTng uses the "void *private" private data pointer extensively. It's very
> > useful to pass a kernel client specific data structure to the client
> > callbacks.
> > * LTTng does have its own ltt_poll and ltt_ioctl that are all what is needed to
> > control the interaction with the file (along with the relayfs mmap/unmap).
> >
> > In this scenario, the sysfs relay attribute creation would look like :
> >
> > - create an empty attr
> > - fill some of attr members
> > - sysfs_create_relay_file(kobj, attr);
> > (it will overwrite some attr members : kobj, rchan, rchan_buf)
> > * set specific LTTng file operations on the inode
> > * set the "private" field of the rchan structure.
> >
> ->rchan is set after sysfs_create_relay_file(), you can set
> ->rchan->private after it's created. All that happens in
> sysfs_create_relay_file() as far as the relay interface is concerned is
> wrapping relay_open(), where ->rchan is first allocated.
>
I though about it, but it would create a race where the channel could be
accessed from user space before the private data pointer is set or the specific
inode operations are set.
> As far as setting specific file operations for the inode, that's
> definitely not in line with the sysfs way of doing things. If you need
> to do this, go with debugfs, or stick with the relayfs patch on top of
> CONFIG_RELAY as this stuff is clearly not going to be merged into
> mainline as Cristoph also pointed out.
>
Those inode operations are generic enough to eventually be integrated to
relayfs. The poll is enhanced to support multiple readers. For the ioctl, it's
just a matter of getting/pulling buffer segments, reading the number of
subbuffers and their size, which I think really fits the i/o control purpose for
such a file.
> debugfs gives you roughly all of the same functionality, and the kernel
> side implementation for what LTTng will need should be quite trivial. LTT
> is arguably a debugging thing anyways, so debugfs seems like a more
> appropriate match than trying to beat sysfs into submission with this
> workload.
>
debugfs seems to offer really too much flexibility for what LTTng needs. It
doesn't really have to redefine "new" operations : the poll/ioctl used by LTTng
could in fact be integrated into RelayFS and simplify the file reading
operation.
Mathieu
OpenPGP public key: http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Sun, Feb 19, 2006 at 08:52:54PM +0200, Paul Mundt wrote:
> On Sun, Feb 19, 2006 at 09:56:23AM -0800, Greg KH wrote:
> > Note that Pat isn't the sysfs maintainer anymore :)
> >
> My mistake, I'll check MAINTAINERS instead of the file comments next
> time.
>
> > On Sun, Feb 19, 2006 at 07:17:48PM +0200, Paul Mundt wrote:
> > > Now with relayfs integrated and the relay_file_operations exported for
> > > use by other file systems, I wonder what people think about adding in a
> > > sysfs attribute for setting up channel buffers.
> >
> > Looks good, I like it. This properly handles the module owner stuff,
> > too, right?
> >
> Could you elaborate on which module owner issue you're referring to?
> struct relay_attribute encapsulates a struct attribute, and it's handled
> the same way as the other attribute types (I modelled it after
> struct bin_attribute), and I don't see any places that I missed.
>
> When setting up the relay attribute, it's just a matter of:
>
> static struct relay_attribute dev_relay_attr = {
> .attr = {
> .owner = THIS_MODULE,
> ...
> },
> ...
> };
>
> Let me know if I've missed anything.
No, you are right, just wanted to make sure that the .owner stuff is
there.
One thing to note, a lot of people forgot to set that field for binary
attribute files, while "normal" attributes get it set "automatically"
due to the macro that was used to create them. You might consider also
creating a macro for this struture so people can not forget to set the
field.
thanks,
greg k-h
On Mon, Feb 20, 2006 at 12:37:32PM -0500, Mathieu Desnoyers wrote:
>
> debugfs seems to offer really too much flexibility for what LTTng needs. It
> doesn't really have to redefine "new" operations : the poll/ioctl used by LTTng
> could in fact be integrated into RelayFS and simplify the file reading
> operation.
Heh, you don't _have_ to use all of the flexibility in debugfs if you
don't want to do so. Why not create a simple debugfs wrapper function
(like the ones already present) to create the file in the format that
you wish to have it in? I'd be glad to accept such a patch.
And yes, I think LTT should be using debugfs, as that is exactly what it
was created for (debugging stuff.)
thanks,
greg k-h
On Mon, Feb 20, 2006 at 02:24:49PM -0800, Greg KH wrote:
> One thing to note, a lot of people forgot to set that field for binary
> attribute files, while "normal" attributes get it set "automatically"
> due to the macro that was used to create them. You might consider also
> creating a macro for this struture so people can not forget to set the
> field.
>
Good point, how about this?
This adds a simple __ATTR_RELAY() to help people define relay attributes,
this takes care of things like getting the module owner right.
Signed-off-by: Paul Mundt <[email protected]>
---
include/linux/sysfs.h | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
5a1440482f48b3ab4b8365a8081295869992bad2
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 0faca48..36a078e 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -44,6 +44,16 @@ struct attribute_group {
.show = _name##_show, \
}
+#define __ATTR_RELAY(_name,_buffer_size,_nr_buffers) { \
+ .attr = { \
+ .owner = THIS_MODULE, \
+ .name = __stringify(_name), \
+ .mode = 0400, \
+ }, \
+ .subbuf_size = _buffer_size, \
+ .n_subbufs = _nr_buffers, \
+}
+
#define __ATTR_NULL { .attr = { .name = NULL } }
#define attr_name(_attr) (_attr).attr.name
On Mon, Feb 20, 2006 at 12:37:32PM -0500, Mathieu Desnoyers wrote:
> Those inode operations are generic enough to eventually be integrated to
> relayfs. The poll is enhanced to support multiple readers. For the ioctl, it's
> just a matter of getting/pulling buffer segments, reading the number of
> subbuffers and their size, which I think really fits the i/o control purpose for
> such a file.
>
Wait a minute, you're talking about inode operations, but then talking
about poll() and ioctl(), which are clearly file operations. Can you
clarify what exactly it is you want to overload? Changing inode
operations seems like a really bad design decision..
> debugfs seems to offer really too much flexibility for what LTTng needs. It
> doesn't really have to redefine "new" operations : the poll/ioctl used by LTTng
> could in fact be integrated into RelayFS and simplify the file reading
> operation.
>
Too much flexibility is not something people usually argue as a a point
against adoption, especially for something as lightweight as debugfs,
that's certainly a new one :-)
If you're talking about struct file_operations, then it would seem you
would be best off sticking with debugfs. If the improved file operations
are suitable for kernel/relay.c then they can be integrated there and
then you don't have to worry about overloading the normal
relay_file_operations through some helper functions to hand off to
debugfs..
If you add native debugfs helper functions for creating relay files and
working in line with CONFIG_RELAY, then I'm sure these can be rolled back
in to debugfs proper, which should ease some of the LTTng maintenance.
I don't really see what having your own mount point and stubbed file
system would buy you over this..
* Paul Mundt ([email protected]) wrote:
> Wait a minute, you're talking about inode operations, but then talking
> about poll() and ioctl(), which are clearly file operations. Can you
> clarify what exactly it is you want to overload? Changing inode
> operations seems like a really bad design decision..
>
You are right, I'm talking about file operations.
> Too much flexibility is not something people usually argue as a a point
> against adoption, especially for something as lightweight as debugfs,
> that's certainly a new one :-)
>
Ok, I think we agree that it's not a technical matter. See below for
precisions.
> If you're talking about struct file_operations, then it would seem you
> would be best off sticking with debugfs. If the improved file operations
> are suitable for kernel/relay.c then they can be integrated there and
> then you don't have to worry about overloading the normal
> relay_file_operations through some helper functions to hand off to
> debugfs..
>
> If you add native debugfs helper functions for creating relay files and
> working in line with CONFIG_RELAY, then I'm sure these can be rolled back
> in to debugfs proper, which should ease some of the LTTng maintenance.
>
> I don't really see what having your own mount point and stubbed file
> system would buy you over this..
>
As I understand the problem, LTTng could technically sit either in debugfs,
procfs, sysfs, relayfs : all these (except debugfs) have been tried with the
original LTT. I just want to spare time and not go into the same debates all
over again.
The problem sits in how tracing is seen. To kernel hackers, it is seen as an
interesting kernel debugging tool, while for the vast majority of LTTng
users, it is seen as a useful system wide information gathering tool to debug
_their_ system wide problems involving programs, librairies and drivers.
Therefore, I am reluctant to put a system monitoring tool in a filesystem
clearly indicated for kernel debugging, as the main audience for LTTng is
not kernel hackers, but user space application programmers.
Mathieu
OpenPGP public key: http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Tue, Feb 21, 2006 at 11:48:53AM -0500, Mathieu Desnoyers wrote:
> The problem sits in how tracing is seen. To kernel hackers, it is seen as an
> interesting kernel debugging tool, while for the vast majority of LTTng
> users, it is seen as a useful system wide information gathering tool to debug
> _their_ system wide problems involving programs, librairies and drivers.
And as a debug tool, debugfs seems like the perfict place for it, as you
have just stated :)
And please, no matter what you decide on, do NOT put those files in
proc, as they have nothing to do with processes.
thanks,
greg k-h
Greg KH wrote:
> And as a debug tool, debugfs seems like the perfict place for it, as you
> have just stated :)
So then the statement is that debugfs is for _ALL_ kinds of debugging,
including application debugging. IOW, anyone considering debugfs as
an fs exclusive to kernel-developers is WRONG. IOW2, distros should
_NOT_ shy away from enabling debugfs by default on their production
kernels.
Right?
Just want to make sure this is clear as it has been my impression
all along (and it seems that of others out there), and I may have
very well been wrong, that debugfs is solely for kernel developers.
Thanks,
Karim
--
President / Opersys Inc.
Embedded Linux Training and Expertise
http://www.opersys.com / 1.866.677.4546
On Wed, Feb 22, 2006 at 04:27:17PM -0500, Karim Yaghmour wrote:
>
> Greg KH wrote:
> > And as a debug tool, debugfs seems like the perfict place for it, as you
> > have just stated :)
>
> So then the statement is that debugfs is for _ALL_ kinds of debugging,
> including application debugging. IOW, anyone considering debugfs as
> an fs exclusive to kernel-developers is WRONG. IOW2, distros should
> _NOT_ shy away from enabling debugfs by default on their production
> kernels.
>
> Right?
Sure, I don't see why not. But if a distro doesn't include anything in
their kernels that use debugfs, then they have no reason to enable it.
thanks,
greg k-h