Re: [PATCH] relay: Add global mode support for buffer-only channels
On 7/4/2016 1:30 PM, Chris Wilson wrote:
On Sun, Jul 03, 2016 at 09:45:27PM +0530, akash.g...@intel.com wrote:
From: Akash Goel
The following patch added support to use channels with no associated files.
relay: add buffer-only channels; useful for early logging
This is useful when the exact location of relay file is not known or the
the parent directory of relay file is not available, while creating the
channel and the logging has to start right from the boot.
But there was no provision to use global mode with buffer-only channels,
which is added by this patch, without modifying the interface where initially
there will be a dummy invocation of create_buf_file callback through which
kernel client can convey the need of a global buffer.
For the use case where drivers/kernel clients want a simple interface for the
userspace, which enables them to capture data/logs from relay file in order &
without any post processing, support of Global buffer mode is warranted.
Cc: Eduard - Gabriel Munteanu
Cc: Tom Zanussi
Cc: Chris Wilson
Signed-off-by: Akash Goel
---
kernel/relay.c | 32
1 file changed, 32 insertions(+)
diff --git a/kernel/relay.c b/kernel/relay.c
index 04d7cf3..b267384 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -451,6 +451,16 @@ static struct rchan_buf *relay_open_buf(struct rchan
*chan, unsigned int cpu)
if (!dentry)
goto free_buf;
relay_set_buf_dentry(buf, dentry);
+ } else {
The trailing block can be replaced with:
/* Only retrieve global info, nothing more, nothing less */
Thanks, will do like this.
+ dentry = chan->cb->create_buf_file(NULL, NULL,
+ S_IRUSR, buf,
+ &chan->is_global);
Watch alignment, align to the opening bracket.
+ if (WARN_ON(dentry))
+ goto free_buf;
}
buf->cpu = cpu;
@@ -666,6 +676,27 @@ int relay_late_setup_files(struct rchan *chan,
}
chan->has_base_filename = 1;
chan->parent = parent;
+
+ if (chan->is_global) {
+ if (unlikely(!chan->buf[0])) {
if (WARN_ON_ONCE(!chan->buf[0])) {
+ WARN_ONCE(1, KERN_ERR "CPU 0 has no buffer!\n");
The WARN includes the unlikely and the message here isn't any more
informative than the line itself, plus has the erroneous KERN_ERR
Thanks will combine the WARN and if check.
+ mutex_unlock(&relay_channels_mutex);
+ return -EINVAL;
+}
+
+ dentry = relay_create_buf_file(chan, chan->buf[0], 0);
+
+ if (unlikely(!dentry))
+ err = -EINVAL;
+ else if (WARN_ON(!chan->is_global))
?
Though have checked 'is_global' value before, but can't rule out the
clients toggling its value (inadvertently or maliciously).
Actually create_buf_file() callback will be invoked again from the
relay_create_buf_file() and pointer of 'chan->is_global' is passed in
that.
So to be on safer side, thought to validate the 'chan->is_global' value
after the call to relay_create_buf_file().
+ err = -EINVAL;
+ else
+ relay_set_buf_dentry(chan->buf[0], dentry);
+
+ mutex_unlock(&relay_channels_mutex);
+ return err;
This could be written:
Thanks this is much better.
Best Regards
Akash
if (chan->is_global) {
err = -EINVAL;
if (!WARN_ON_ONCE(!chan->buf[0])) {
dentry = relay_create_buf_file(chan, chan->buf[0], 0))
if (dentry) {
relay_set_buf_dentry(chan->buf[0], dentry);
err = 0;
}
}
mutex_unlock();
return err;
}
for a single exit/unlock path in this block.
-Chris
Re: [PATCH v2] relay: Add global mode support for buffer-only channels
On 7/12/2016 2:54 PM, Chris Wilson wrote:
On Mon, Jul 11, 2016 at 01:17:09PM -0700, Andrew Morton wrote:
On Mon, 11 Jul 2016 12:47:36 +0530 akash.g...@intel.com wrote:
From: Akash Goel
The following patch added support to use channels with no associated files.
relay: add buffer-only channels; useful for early logging
hm, 8 years ago. Normally we refer to previous commits using the form
20d8b67c06fa5e74f44e ("relay: add buffer-only channels; useful for
early logging"). But this one is so old that we should inform readers
about its vintage, so this form:
commit 20d8b67c06fa5e74f44e80b0a0fd68c8327f7c6a
Author: Eduard - Gabriel Munteanu
Date: Fri Jul 25 19:45:12 2008 -0700
relay: add buffer-only channels; useful for early logging
would be better.
This is useful when the exact location of relay file is not known or the
the parent directory of relay file is not available, while creating the
channel and the logging has to start right from the boot.
But there was no provision to use global mode with buffer-only channels,
which is added by this patch, without modifying the interface where initially
there will be a dummy invocation of create_buf_file callback through which
kernel client can convey the need of a global buffer.
For the use case where drivers/kernel clients want a simple interface for the
userspace, which enables them to capture data/logs from relay file in order &
without any post processing, support of Global buffer mode is warranted.
...
@@ -706,6 +727,7 @@ int relay_late_setup_files(struct rchan *chan,
return err;
}
+EXPORT_SYMBOL_GPL(relay_late_setup_files);
The export is unneeded and undocumented.
Something like:
diff --git a/kernel/relay.c b/kernel/relay.c
index 04d7cf3ef8cf..fd86f01de4b2 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -562,6 +562,10 @@ static int relay_hotcpu_callback(struct notifier_block *nb,
* attributes specified. The created channel buffer files
* will be named base_filename0...base_filenameN-1. File
* permissions will be %S_IRUSR.
+ *
+ * If opening a buffer (@parent = NULL) that you later wish to register
+ * in a filesystem, call relay_late_setup_files() once the @parent dentry
+ * is available.
*/
struct rchan *relay_open(const char *base_filename,
struct dentry *parent,
@@ -640,8 +644,12 @@ static void __relay_set_buf_dentry(void *info)
*
* Returns 0 if 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.
+ * Use to setup files for a previously buffer-only channel created
+ * by relay_open() with a NULL parent dentry.
+ *
+ * For example, this is useful for perfomring early tracing in kernel,
+ * before VFS is up and then exposing the early results once the dentry
+ * is available.
*/
int relay_late_setup_files(struct rchan *chan,
const char *base_filename,
with a comment in the changelog that modules using relay_open() in early
init also want to later register their buffer-only relays once debugfs is
available, e.g. i915.
Thanks much, will update the documentation as well as the changelog as
per the above.
But an export of symbol relay_late_setup_files() is still needed, just
like relay_open() is exported, in order to make it accessible to modules
like i915 ?
Best regards
Akash
-Chris
Re: [PATCH v2] relay: Add global mode support for buffer-only channels
On 7/12/2016 6:31 PM, Chris Wilson wrote: On Tue, Jul 12, 2016 at 06:20:06PM +0530, Goel, Akash wrote: Thanks much, will update the documentation as well as the changelog as per the above. But an export of symbol relay_late_setup_files() is still needed, just like relay_open() is exported, in order to make it accessible to modules like i915 ? Yes, we need the companion function in i915.ko. That needs to be explained in the patch notes to justify adding the EXPORT_SYMBOL. Otherwise without that context, it looks unnecessary as Andrew objected to. Won't your suggested updates to Documentation & changelog suffice ?. relay_late_setup_files() is to be used in conjunction with relay_open(), hence need to be exported. Do I also need to provide the corresponding i915 patch, which has a call to relay_late_setup_files() ? Best regards Akash -Chris
