[PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Eduard - Gabriel Munteanu]

relay_open() can now handle NULL base_filename, to register a
buffer-only channel. Using a new function, relay_late_setup_files(), one
can assign files after creating the channel, thus allowing for doing
early tracing in the kernel, before VFS is up.

Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
---
 Documentation/filesystems/relay.txt |   11 ++++
 include/linux/relay.h               |    4 ++
 kernel/relay.c                      |  102 ++++++++++++++++++++++++++---------
 3 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/Documentation/filesystems/relay.txt b/Documentation/filesystems/relay.txt
index 094f2d2..b59c228 100644
--- a/Documentation/filesystems/relay.txt
+++ b/Documentation/filesystems/relay.txt
@@ -161,6 +161,7 @@ TBD(curr. line MT:/API/)
     relay_close(chan)
     relay_flush(chan)
     relay_reset(chan)
+    relay_late_setup_files(chan, base_filename, parent)
 
   channel management typically called on instigation of userspace:
 
@@ -294,6 +295,16 @@ user-defined data with a channel, and is immediately available
 (including in create_buf_file()) via chan->private_data or
 buf->chan->private_data.
 
+Buffer-only channels
+--------------------
+
+These channels have no files associated and can be created with
+relay_open(NULL, NULL, ...). Such channels are useful in scenarios such
+as when doing early tracing in the kernel, before the VFS is up. In these
+cases, one may open a channel a buffer-only channel and then call
+relay_late_setup_files() when the kernel is ready to handle files, to expose
+the buffered data to the userspace.
+
 Channel 'modes'
 ---------------
 
diff --git a/include/linux/relay.h b/include/linux/relay.h
index 6cd8c44..ca9ee3f 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -68,6 +68,7 @@ struct rchan
 	int is_global;			/* One global buffer ? */
 	struct list_head list;		/* for channel list */
 	struct dentry *parent;		/* parent dentry passed to open */
+	int has_base_filename;		/* has a filename associated? */
 	char base_filename[NAME_MAX];	/* saved base filename */
 };
 
@@ -169,6 +170,9 @@ struct rchan *relay_open(const char *base_filename,
 			 size_t n_subbufs,
 			 struct rchan_callbacks *cb,
 			 void *private_data);
+extern int relay_late_setup_files(struct rchan *chan,
+				  const char *base_filename,
+				  struct dentry *parent);
 extern void relay_close(struct rchan *chan);
 extern void relay_flush(struct rchan *chan);
 extern void relay_subbufs_consumed(struct rchan *chan,
diff --git a/kernel/relay.c b/kernel/relay.c
index 4c035a8..4bea94a 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -378,6 +378,35 @@ void relay_reset(struct rchan *chan)
 }
 EXPORT_SYMBOL_GPL(relay_reset);
 
+static int relay_setup_buf_file(struct rchan *chan,
+				struct rchan_buf *buf,
+				unsigned int cpu)
+{
+	struct dentry *dentry;
+ 	char *tmpname;
+
+	tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL);
+	if (!tmpname)
+		goto failed;
+	snprintf(tmpname, NAME_MAX, "%s%d", chan->base_filename, cpu);
+
+	/* Create file in fs */
+	dentry = chan->cb->create_buf_file(tmpname, chan->parent,
+					   S_IRUSR, buf, 
+					   &chan->is_global);
+	
+	if (!dentry)
+		goto failed;
+	buf->dentry = dentry;
+
+	kfree(tmpname);
+
+	return 0;
+
+failed:
+	return 1;
+}
+
 /*
  *	relay_open_buf - create a new relay channel buffer
  *
@@ -386,44 +415,30 @@ EXPORT_SYMBOL_GPL(relay_reset);
 static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 {
  	struct rchan_buf *buf = NULL;
-	struct dentry *dentry;
- 	char *tmpname;
 
  	if (chan->is_global)
 		return chan->buf[0];
 
-	tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL);
- 	if (!tmpname)
- 		goto end;
- 	snprintf(tmpname, NAME_MAX, "%s%d", chan->base_filename, cpu);
-
 	buf = relay_create_buf(chan);
 	if (!buf)
- 		goto free_name;
+ 		goto end;
+
+ 	if (chan->has_base_filename)
+		if (relay_setup_buf_file(chan, buf, cpu)) goto free_buf;
 
  	buf->cpu = cpu;
  	__relay_reset(buf, 1);
 
-	/* Create file in fs */
- 	dentry = chan->cb->create_buf_file(tmpname, chan->parent, S_IRUSR,
- 					   buf, &chan->is_global);
- 	if (!dentry)
- 		goto free_buf;
-
-	buf->dentry = dentry;
-
  	if(chan->is_global) {
  		chan->buf[0] = buf;
  		buf->cpu = 0;
   	}
 
- 	goto free_name;
+ 	goto end;
 
 free_buf:
  	relay_destroy_buf(buf);
  	buf = NULL;
-free_name:
- 	kfree(tmpname);
 end:
 	return buf;
 }
@@ -508,8 +523,8 @@ static int __cpuinit relay_hotcpu_callback(struct notifier_block *nb,
 
 /**
  *	relay_open - create a new relay channel
- *	@base_filename: base name of files to create
- *	@parent: dentry of parent directory, %NULL for root directory
+ *	@base_filename: base name of files to create, %NULL for buffering only
+ *	@parent: dentry of parent directory, %NULL for root directory or buffer
  *	@subbuf_size: size of sub-buffers
  *	@n_subbufs: number of sub-buffers
  *	@cb: client callback functions
@@ -531,8 +546,6 @@ struct rchan *relay_open(const char *base_filename,
 {
 	unsigned int i;
 	struct rchan *chan;
-	if (!base_filename)
-		return NULL;
 
 	if (!(subbuf_size && n_subbufs))
 		return NULL;
@@ -547,12 +560,17 @@ struct rchan *relay_open(const char *base_filename,
 	chan->alloc_size = FIX_SIZE(subbuf_size * n_subbufs);
 	chan->parent = parent;
 	chan->private_data = private_data;
-	strlcpy(chan->base_filename, base_filename, NAME_MAX);
+	if (base_filename) {
+		chan->has_base_filename = 1;
+		strlcpy(chan->base_filename, base_filename, NAME_MAX);
+	} else {
+		chan->has_base_filename = 0;
+	}
 	setup_callbacks(chan, cb);
 	kref_init(&chan->kref);
 
 	mutex_lock(&relay_channels_mutex);
-	for_each_online_cpu(i) {
+	for_each_present_cpu(i) {
 		chan->buf[i] = relay_open_buf(chan, i);
 		if (!chan->buf[i])
 			goto free_bufs;
@@ -563,7 +581,7 @@ struct rchan *relay_open(const char *base_filename,
 	return chan;
 
 free_bufs:
-	for_each_online_cpu(i) {
+	for_each_present_cpu(i) {
 		if (!chan->buf[i])
 			break;
 		relay_close_buf(chan->buf[i]);
@@ -576,6 +594,38 @@ free_bufs:
 EXPORT_SYMBOL_GPL(relay_open);
 
 /**
+ *	relay_late_setup_files - triggers file creation
+ *	@chan: channel to operate on
+ *	@base_filename: base name of files to create
+ *	@parent: dentry of parent directory, %NULL for root directory
+ *
+ *	Returns 0 is successful, non-zero otherwise.
+ *
+ *	Use to setup files for a previously buffer-only channel.
+ *	Useful to do early tracing in kernel, before VFS is up, for example.
+ */
+int relay_late_setup_files(struct rchan *chan,
+			   const char *base_filename,
+			   struct dentry *parent)
+{
+	unsigned int i;
+	
+	if (!chan || !base_filename)
+		return 1;
+	
+	strlcpy(chan->base_filename, base_filename, NAME_MAX);
+	chan->has_base_filename = 1;
+	chan->parent = parent;
+
+	mutex_lock(&relay_channels_mutex);
+	for_each_present_cpu(i)
+		relay_setup_buf_file(chan, chan->buf[i], i);
+	mutex_unlock(&relay_channels_mutex);
+
+	return 0;
+}
+		
+/**
  *	relay_switch_subbuf - switch to a new sub-buffer
  *	@buf: channel buffer
  *	@length: size of current event
-- 
1.5.2.5

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Mathieu Desnoyers]

* Eduard - Gabriel Munteanu (eduard.munteanu@linux360.ro) wrote:
> relay_open() can now handle NULL base_filename, to register a
> buffer-only channel. Using a new function, relay_late_setup_files(), one
> can assign files after creating the channel, thus allowing for doing
> early tracing in the kernel, before VFS is up.
> 

I like this :) But could we trace event sooner if we did not depend on
vmalloc to allocate the buffers ?

I am thinking of receiving a kernel parameter that would reserve a
buffer in the linear kernel mapping (kmalloc ?), or to compile-in a
static buffer, which could be used by relay instead of the vmalloc'd
buffer. That would permit to trace the memory allocator and some other
very early stuff. Basically, relay would have it's "memory pool" to
manage and would do its allocations from this fixed buffer. No free
would be required.

Mathieu

> Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> ---
>  Documentation/filesystems/relay.txt |   11 ++++
>  include/linux/relay.h               |    4 ++
>  kernel/relay.c                      |  102 ++++++++++++++++++++++++++---------
>  3 files changed, 91 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/filesystems/relay.txt b/Documentation/filesystems/relay.txt
> index 094f2d2..b59c228 100644
> --- a/Documentation/filesystems/relay.txt
> +++ b/Documentation/filesystems/relay.txt
> @@ -161,6 +161,7 @@ TBD(curr. line MT:/API/)
>      relay_close(chan)
>      relay_flush(chan)
>      relay_reset(chan)
> +    relay_late_setup_files(chan, base_filename, parent)
>  
>    channel management typically called on instigation of userspace:
>  
> @@ -294,6 +295,16 @@ user-defined data with a channel, and is immediately available
>  (including in create_buf_file()) via chan->private_data or
>  buf->chan->private_data.
>  
> +Buffer-only channels
> +--------------------
> +
> +These channels have no files associated and can be created with
> +relay_open(NULL, NULL, ...). Such channels are useful in scenarios such
> +as when doing early tracing in the kernel, before the VFS is up. In these
> +cases, one may open a channel a buffer-only channel and then call
> +relay_late_setup_files() when the kernel is ready to handle files, to expose
> +the buffered data to the userspace.
> +
>  Channel 'modes'
>  ---------------
>  
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index 6cd8c44..ca9ee3f 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -68,6 +68,7 @@ struct rchan
>  	int is_global;			/* One global buffer ? */
>  	struct list_head list;		/* for channel list */
>  	struct dentry *parent;		/* parent dentry passed to open */
> +	int has_base_filename;		/* has a filename associated? */
>  	char base_filename[NAME_MAX];	/* saved base filename */
>  };
>  
> @@ -169,6 +170,9 @@ struct rchan *relay_open(const char *base_filename,
>  			 size_t n_subbufs,
>  			 struct rchan_callbacks *cb,
>  			 void *private_data);
> +extern int relay_late_setup_files(struct rchan *chan,
> +				  const char *base_filename,
> +				  struct dentry *parent);
>  extern void relay_close(struct rchan *chan);
>  extern void relay_flush(struct rchan *chan);
>  extern void relay_subbufs_consumed(struct rchan *chan,
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 4c035a8..4bea94a 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -378,6 +378,35 @@ void relay_reset(struct rchan *chan)
>  }
>  EXPORT_SYMBOL_GPL(relay_reset);
>  
> +static int relay_setup_buf_file(struct rchan *chan,
> +				struct rchan_buf *buf,
> +				unsigned int cpu)
> +{
> +	struct dentry *dentry;
> + 	char *tmpname;
> +
> +	tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL);
> +	if (!tmpname)
> +		goto failed;
> +	snprintf(tmpname, NAME_MAX, "%s%d", chan->base_filename, cpu);
> +
> +	/* Create file in fs */
> +	dentry = chan->cb->create_buf_file(tmpname, chan->parent,
> +					   S_IRUSR, buf, 
> +					   &chan->is_global);
> +	
> +	if (!dentry)
> +		goto failed;
> +	buf->dentry = dentry;
> +
> +	kfree(tmpname);
> +
> +	return 0;
> +
> +failed:
> +	return 1;
> +}
> +
>  /*
>   *	relay_open_buf - create a new relay channel buffer
>   *
> @@ -386,44 +415,30 @@ EXPORT_SYMBOL_GPL(relay_reset);
>  static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
>  {
>   	struct rchan_buf *buf = NULL;
> -	struct dentry *dentry;
> - 	char *tmpname;
>  
>   	if (chan->is_global)
>  		return chan->buf[0];
>  
> -	tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL);
> - 	if (!tmpname)
> - 		goto end;
> - 	snprintf(tmpname, NAME_MAX, "%s%d", chan->base_filename, cpu);
> -
>  	buf = relay_create_buf(chan);
>  	if (!buf)
> - 		goto free_name;
> + 		goto end;
> +
> + 	if (chan->has_base_filename)
> +		if (relay_setup_buf_file(chan, buf, cpu)) goto free_buf;
>  
>   	buf->cpu = cpu;
>   	__relay_reset(buf, 1);
>  
> -	/* Create file in fs */
> - 	dentry = chan->cb->create_buf_file(tmpname, chan->parent, S_IRUSR,
> - 					   buf, &chan->is_global);
> - 	if (!dentry)
> - 		goto free_buf;
> -
> -	buf->dentry = dentry;
> -
>   	if(chan->is_global) {
>   		chan->buf[0] = buf;
>   		buf->cpu = 0;
>    	}
>  
> - 	goto free_name;
> + 	goto end;
>  
>  free_buf:
>   	relay_destroy_buf(buf);
>   	buf = NULL;
> -free_name:
> - 	kfree(tmpname);
>  end:
>  	return buf;
>  }
> @@ -508,8 +523,8 @@ static int __cpuinit relay_hotcpu_callback(struct notifier_block *nb,
>  
>  /**
>   *	relay_open - create a new relay channel
> - *	@base_filename: base name of files to create
> - *	@parent: dentry of parent directory, %NULL for root directory
> + *	@base_filename: base name of files to create, %NULL for buffering only
> + *	@parent: dentry of parent directory, %NULL for root directory or buffer
>   *	@subbuf_size: size of sub-buffers
>   *	@n_subbufs: number of sub-buffers
>   *	@cb: client callback functions
> @@ -531,8 +546,6 @@ struct rchan *relay_open(const char *base_filename,
>  {
>  	unsigned int i;
>  	struct rchan *chan;
> -	if (!base_filename)
> -		return NULL;
>  
>  	if (!(subbuf_size && n_subbufs))
>  		return NULL;
> @@ -547,12 +560,17 @@ struct rchan *relay_open(const char *base_filename,
>  	chan->alloc_size = FIX_SIZE(subbuf_size * n_subbufs);
>  	chan->parent = parent;
>  	chan->private_data = private_data;
> -	strlcpy(chan->base_filename, base_filename, NAME_MAX);
> +	if (base_filename) {
> +		chan->has_base_filename = 1;
> +		strlcpy(chan->base_filename, base_filename, NAME_MAX);
> +	} else {
> +		chan->has_base_filename = 0;
> +	}
>  	setup_callbacks(chan, cb);
>  	kref_init(&chan->kref);
>  
>  	mutex_lock(&relay_channels_mutex);
> -	for_each_online_cpu(i) {
> +	for_each_present_cpu(i) {
>  		chan->buf[i] = relay_open_buf(chan, i);
>  		if (!chan->buf[i])
>  			goto free_bufs;
> @@ -563,7 +581,7 @@ struct rchan *relay_open(const char *base_filename,
>  	return chan;
>  
>  free_bufs:
> -	for_each_online_cpu(i) {
> +	for_each_present_cpu(i) {
>  		if (!chan->buf[i])
>  			break;
>  		relay_close_buf(chan->buf[i]);
> @@ -576,6 +594,38 @@ free_bufs:
>  EXPORT_SYMBOL_GPL(relay_open);
>  
>  /**
> + *	relay_late_setup_files - triggers file creation
> + *	@chan: channel to operate on
> + *	@base_filename: base name of files to create
> + *	@parent: dentry of parent directory, %NULL for root directory
> + *
> + *	Returns 0 is successful, non-zero otherwise.
> + *
> + *	Use to setup files for a previously buffer-only channel.
> + *	Useful to do early tracing in kernel, before VFS is up, for example.
> + */
> +int relay_late_setup_files(struct rchan *chan,
> +			   const char *base_filename,
> +			   struct dentry *parent)
> +{
> +	unsigned int i;
> +	
> +	if (!chan || !base_filename)
> +		return 1;
> +	
> +	strlcpy(chan->base_filename, base_filename, NAME_MAX);
> +	chan->has_base_filename = 1;
> +	chan->parent = parent;
> +
> +	mutex_lock(&relay_channels_mutex);
> +	for_each_present_cpu(i)
> +		relay_setup_buf_file(chan, chan->buf[i], i);
> +	mutex_unlock(&relay_channels_mutex);
> +
> +	return 0;
> +}
> +		
> +/**
>   *	relay_switch_subbuf - switch to a new sub-buffer
>   *	@buf: channel buffer
>   *	@length: size of current event
> -- 
> 1.5.2.5
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Randy Dunlap]

On Fri, 4 Apr 2008 18:33:03 +0300 Eduard - Gabriel Munteanu wrote:

> relay_open() can now handle NULL base_filename, to register a
> buffer-only channel. Using a new function, relay_late_setup_files(), one
> can assign files after creating the channel, thus allowing for doing
> early tracing in the kernel, before VFS is up.
> 
> Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> ---
>  Documentation/filesystems/relay.txt |   11 ++++
>  include/linux/relay.h               |    4 ++
>  kernel/relay.c                      |  102 ++++++++++++++++++++++++++---------
>  3 files changed, 91 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/filesystems/relay.txt b/Documentation/filesystems/relay.txt
> index 094f2d2..b59c228 100644
> --- a/Documentation/filesystems/relay.txt
> +++ b/Documentation/filesystems/relay.txt
> @@ -161,6 +161,7 @@ TBD(curr. line MT:/API/)
>      relay_close(chan)
>      relay_flush(chan)
>      relay_reset(chan)
> +    relay_late_setup_files(chan, base_filename, parent)
>  
>    channel management typically called on instigation of userspace:
>  
> @@ -294,6 +295,16 @@ user-defined data with a channel, and is immediately available
>  (including in create_buf_file()) via chan->private_data or
>  buf->chan->private_data.
>  
> +Buffer-only channels
> +--------------------
> +
> +These channels have no files associated and can be created with
> +relay_open(NULL, NULL, ...). Such channels are useful in scenarios such
> +as when doing early tracing in the kernel, before the VFS is up. In these
> +cases, one may open a channel a buffer-only channel and then call
                       ~~~~~~~~~
d/a channel/

> +relay_late_setup_files() when the kernel is ready to handle files, to expose
> +the buffered data to the userspace.
> +
>  Channel 'modes'
>  ---------------
>  
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 4c035a8..4bea94a 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -378,6 +378,35 @@ void relay_reset(struct rchan *chan)
>  }
>  EXPORT_SYMBOL_GPL(relay_reset);
>  
> +static int relay_setup_buf_file(struct rchan *chan,
> +				struct rchan_buf *buf,
> +				unsigned int cpu)
> +{
> +	struct dentry *dentry;
> + 	char *tmpname;
> +
> +	tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL);
> +	if (!tmpname)
> +		goto failed;
> +	snprintf(tmpname, NAME_MAX, "%s%d", chan->base_filename, cpu);
> +
> +	/* Create file in fs */
> +	dentry = chan->cb->create_buf_file(tmpname, chan->parent,
> +					   S_IRUSR, buf, 
> +					   &chan->is_global);
> +	
> +	if (!dentry)
> +		goto failed;

above leaks tmpname ?

> +	buf->dentry = dentry;
> +
> +	kfree(tmpname);
> +
> +	return 0;
> +
> +failed:
> +	return 1;
> +}
> +
>  /*
>   *	relay_open_buf - create a new relay channel buffer
>   *
> @@ -386,44 +415,30 @@ EXPORT_SYMBOL_GPL(relay_reset);
>  static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
>  {
>   	struct rchan_buf *buf = NULL;
> -	struct dentry *dentry;
> - 	char *tmpname;
>  
>   	if (chan->is_global)
>  		return chan->buf[0];
>  
> -	tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL);
> - 	if (!tmpname)
> - 		goto end;
> - 	snprintf(tmpname, NAME_MAX, "%s%d", chan->base_filename, cpu);
> -
>  	buf = relay_create_buf(chan);
>  	if (!buf)
> - 		goto free_name;
> + 		goto end;
> +
> + 	if (chan->has_base_filename)
> +		if (relay_setup_buf_file(chan, buf, cpu)) goto free_buf;

Put 'goto free_buf;' on separate line.

>  
>   	buf->cpu = cpu;
>   	__relay_reset(buf, 1);
>  
> -	/* Create file in fs */
> - 	dentry = chan->cb->create_buf_file(tmpname, chan->parent, S_IRUSR,
> - 					   buf, &chan->is_global);
> - 	if (!dentry)
> - 		goto free_buf;
> -
> -	buf->dentry = dentry;
> -
>   	if(chan->is_global) {
>   		chan->buf[0] = buf;
>   		buf->cpu = 0;
>    	}
>  
> - 	goto free_name;
> + 	goto end;
>  
>  free_buf:
>   	relay_destroy_buf(buf);
>   	buf = NULL;
> -free_name:
> - 	kfree(tmpname);
>  end:
>  	return buf;
>  }
> @@ -576,6 +594,38 @@ free_bufs:
>  EXPORT_SYMBOL_GPL(relay_open);
>  
>  /**
> + *	relay_late_setup_files - triggers file creation
> + *	@chan: channel to operate on
> + *	@base_filename: base name of files to create
> + *	@parent: dentry of parent directory, %NULL for root directory
> + *
> + *	Returns 0 is successful, non-zero otherwise.

s/is/if/

> + *
> + *	Use to setup files for a previously buffer-only channel.
> + *	Useful to do early tracing in kernel, before VFS is up, for example.
> + */
> +int relay_late_setup_files(struct rchan *chan,
> +			   const char *base_filename,
> +			   struct dentry *parent)
> +{
> +	unsigned int i;
> +	
> +	if (!chan || !base_filename)
> +		return 1;
> +	
> +	strlcpy(chan->base_filename, base_filename, NAME_MAX);
> +	chan->has_base_filename = 1;
> +	chan->parent = parent;
> +
> +	mutex_lock(&relay_channels_mutex);
> +	for_each_present_cpu(i)
> +		relay_setup_buf_file(chan, chan->buf[i], i);
> +	mutex_unlock(&relay_channels_mutex);
> +
> +	return 0;
> +}
> +		
> +/**
>   *	relay_switch_subbuf - switch to a new sub-buffer
>   *	@buf: channel buffer
>   *	@length: size of current event

---
~Randy

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Eduard - Gabriel Munteanu]

On Fri, 4 Apr 2008 12:06:23 -0400
Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:

> I like this :) But could we trace event sooner if we did not depend on
> vmalloc to allocate the buffers ?
> 
> I am thinking of receiving a kernel parameter that would reserve a
> buffer in the linear kernel mapping (kmalloc ?), or to compile-in a
> static buffer, which could be used by relay instead of the vmalloc'd
> buffer. That would permit to trace the memory allocator and some other
> very early stuff. Basically, relay would have it's "memory pool" to
> manage and would do its allocations from this fixed buffer. No free
> would be required.
> 
> Mathieu

Sure. I'm doing this as part of my GSoC application (well, I haven't
been accepted yet). I had also proposed this to Pekka Enberg, my
mentor, but he was reluctant to hack the relay code more invasively. Now
that you agree with this, I'll work on it.

kmalloc() doesn't work before kmem_cache_init() and relay code uses
kmalloc(), not vmalloc(), IIRC.

I would go another way. Have the relay client call __get_free_pages()
or similar function, allocate the buffer and pass it on to a variant of
relay_open(), let's name it relay_early_open(). __get_free_pages() can
be used before kmem_cache_init() as far as I know. No free is required,
since early tracing code implies it's built into the kernel and not as
a module. What do you say?

Will you merge this or wait until I finish the very early tracing stuff?

Cheers,
Eduard

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Eduard - Gabriel Munteanu]

Hi,

Thanks for spotting these problems, Randy.  I'll fix them and resubmit.


Cheers,
Eduard

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Mathieu Desnoyers]

* Eduard - Gabriel Munteanu (eduard.munteanu@linux360.ro) wrote:
> On Fri, 4 Apr 2008 12:06:23 -0400
> Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:
> 
> > I like this :) But could we trace event sooner if we did not depend on
> > vmalloc to allocate the buffers ?
> > 
> > I am thinking of receiving a kernel parameter that would reserve a
> > buffer in the linear kernel mapping (kmalloc ?), or to compile-in a
> > static buffer, which could be used by relay instead of the vmalloc'd
> > buffer. That would permit to trace the memory allocator and some other
> > very early stuff. Basically, relay would have it's "memory pool" to
> > manage and would do its allocations from this fixed buffer. No free
> > would be required.
> > 
> > Mathieu
> 
> Sure. I'm doing this as part of my GSoC application (well, I haven't
> been accepted yet). I had also proposed this to Pekka Enberg, my
> mentor, but he was reluctant to hack the relay code more invasively. Now
> that you agree with this, I'll work on it.
> 
> kmalloc() doesn't work before kmem_cache_init() and relay code uses
> kmalloc(), not vmalloc(), IIRC.
> 

It currently uses both :

It uses alloc_page and vmap to allocate the buffers and it uses
kzalloc/kmalloc to allocate the relay structures.

> I would go another way. Have the relay client call __get_free_pages()
> or similar function, allocate the buffer and pass it on to a variant of
> relay_open(), let's name it relay_early_open(). __get_free_pages() can
> be used before kmem_cache_init() as far as I know. No free is required,
> since early tracing code implies it's built into the kernel and not as
> a module. What do you say?
> 

Then I guess we would depend on page_alloc_init(). It's not that bad,
but I guess the question is : do we prefer to let users specify the
buffer size to reserve for tracing on the kernel command line (that
would require __get_free_pages()) or do we compile-in a static buffer
size, which can be specified in the kernel configuration. I think
providing both could be an option : flexibility _and_ *very* early
tracing.

So making this relay interface flexible enough to receive a buffer
either reserved by __get_free_pages() or a static buffer seems like a
good compromise. It would also have to receive the buffer size from the
caller.

> Will you merge this or wait until I finish the very early tracing stuff?
> 

As far as I am concerned, I can only merge this in the LTTng project to
try it. I've added Tom Zanussi, the author of relay, who might have some
ideas to share with us. I would wait until we have a more precise idea
of the API before I modify LTTng to start using it.

Regards,

Mathieu

> Cheers,
> Eduard
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Eduard - Gabriel Munteanu]

On Fri, 4 Apr 2008 12:40:22 -0400
Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:
 
> Then I guess we would depend on page_alloc_init(). It's not that bad,
> but I guess the question is : do we prefer to let users specify the
> buffer size to reserve for tracing on the kernel command line (that
> would require __get_free_pages()) or do we compile-in a static buffer
> size, which can be specified in the kernel configuration. I think
> providing both could be an option : flexibility _and_ *very* early
> tracing.

I wasn't referring to the kernel's users specifying the buffer on the
command line. For example, LTTng code (I'm not familiar with it) could
allocate a buffer, by itself or using a helper function, and pass it to
relay_early_open(). Of course, it could read the command line and call
that helper function, passing the size required by the user as an
argument.

Basically, I'm saying that having relay code manage a large buffer,
allocating memory from it for each of its callers, could be very
complex. It would be easier and less complex to let each tracing
subsystem allocate memory on its own, either through a static buffer or
with __get_free_pages().

> So making this relay interface flexible enough to receive a buffer
> either reserved by __get_free_pages() or a static buffer seems like a
> good compromise. It would also have to receive the buffer size from
> the caller.

Yes, it shouldn't matter to the relay interface whether the buffer was
statically or dynamically allocated. All it needs to know is where it
starts and how long it is.

BTW, I'll resubmit a modified patch soon, as suggested by Randy Dunlap.


Eduard

Re: [PATCH] relay: add buffer-only functionality, allowing for early kernel tracing

[Eduard - Gabriel Munteanu]

On Fri, 4 Apr 2008 12:40:22 -0400
Mathieu Desnoyers <compudj@krystal.dyndns.org> wrote:

> As far as I am concerned, I can only merge this in the LTTng project
> to try it. I've added Tom Zanussi, the author of relay, who might
> have some ideas to share with us. I would wait until we have a more
> precise idea of the API before I modify LTTng to start using it.
> 
> Regards,
> 
> Mathieu

Hi,

I've tested my patch a bit more and ran into a problem, and fixed it.
Forget the old patch, it will panic the kernel if it has to cross
sub-buffer boundaries. I'll polish the new one a bit (maybe update the
docs) and resubmit, along with a piece of code from my GSoC project that
uses the early relay interface (may provide an easy to try demo).

Unfortunately, as for doing earlier than kmem_cache_init() tracing, I
don't have the time right now, as I have to carry on with my GSoC
project. I looked through the source more thoroughly and the needed
changes are not trivial at all. I'm not saying I won't work on it, but
it won't be a priority.

By the way, I got a "delivery failure" message... is something wrong
with Tom Zanussi's e-mail address? Google showed 2007 LKML entries with
his e-mail address as <zanussi at comcast dot net>. In any case, when
I'll resubmit the patch, I'll add this other address in the Cc list. Or
should we reach him another way?

I'd like to get this merged as it is so far, and mailing it directly to
akpm (or worse, Linus) doesn't sound like a good idea.


Cheers,
Eduard