Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1435978 > unrolled thread

[PATCH] relay: Add global mode support for buffer-only channels

Back to article view | Back to linux.kernel

Contents

  [PATCH] relay: Add global mode support for buffer-only channels akash.goel@intel.com - 2016-07-03 18:10 +0200
    Re: [PATCH] relay: Add global mode support for buffer-only channels Chris Wilson <chris@chris-wilson.co.uk> - 2016-07-04 10:10 +0200
      Re: [PATCH] relay: Add global mode support for buffer-only channels "Goel, Akash" <akash.goel@intel.com> - 2016-07-04 15:10 +0200

#1435978 — [PATCH] relay: Add global mode support for buffer-only channels

From: Akash Goel <akash.goel@intel.com>

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 <eduard.munteanu@linux360.ro>
Cc: Tom Zanussi <tzanussi@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 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 {
+		dentry = chan->cb->create_buf_file(NULL, NULL,
+					S_IRUSR, buf,
+					&chan->is_global);
+		/* We did not expected the client to return a valid dentry,
+		 * was now just looking for the global info from the client.
+		 * Now onus is on client only to release this dentry pointer.
+		 */
+		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])) {
+			WARN_ONCE(1, KERN_ERR "CPU 0 has no buffer!\n");
+			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))
+			err = -EINVAL;
+		else
+			relay_set_buf_dentry(chan->buf[0], dentry);
+
+		mutex_unlock(&relay_channels_mutex);
+		return err;
+	}
+
 	curr_cpu = get_cpu();
 	/*
 	 * The CPU hotplug notifier ran before us and created buffers with
@@ -706,6 +737,7 @@ int relay_late_setup_files(struct rchan *chan,
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(relay_late_setup_files);
 
 /**
  *	relay_switch_subbuf - switch to a new sub-buffer
-- 
1.9.2

[toc] | [next] | [standalone]

#1436246

On Sun, Jul 03, 2016 at 09:45:27PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> 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 <eduard.munteanu@linux360.ro>
> Cc: Tom Zanussi <tzanussi@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  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 */

> +		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

> +			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))

?

> +			err = -EINVAL;
> +		else
> +			relay_set_buf_dentry(chan->buf[0], dentry);
> +
> +		mutex_unlock(&relay_channels_mutex);
> +		return err;

This could be written:

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

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [next] | [standalone]

#1436484

On 7/4/2016 1:30 PM, Chris Wilson wrote:
> On Sun, Jul 03, 2016 at 09:45:27PM +0530, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@intel.com>
>>
>> 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 <eduard.munteanu@linux360.ro>
>> Cc: Tom Zanussi <tzanussi@gmail.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>  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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web