[PATCH 0/2] tracing: Fix document and blktrace

[PATCH 0/2] tracing: Fix document and blktrace

[Masami Hiramatsu]

Hi,

Here are 2 small fixes for ftrace, one is documentation fix to
add stacktrace action and another is blktrace fix so that
user can set the current enablement parameter without an error.

Thank you,

---

Masami Hiramatsu (2):
      docs: tracing: Add stacktrace filter command
      tracing/blktrace: Fix to allow setting same value


 Documentation/trace/ftrace.rst |    3 +++
 kernel/trace/blktrace.c        |   11 +++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/2] docs: tracing: Add stacktrace filter command

[Masami Hiramatsu]

Add a description of stacktrace filter command.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-doc@vger.kernel.org
---
 Documentation/trace/ftrace.rst |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
index a20d34955333..81378a8b631e 100644
--- a/Documentation/trace/ftrace.rst
+++ b/Documentation/trace/ftrace.rst
@@ -2987,6 +2987,9 @@ The following commands are supported:
   command, it only prints out the contents of the ring buffer for the
   CPU that executed the function that triggered the dump.
 
+- stacktrace:
+  When the function is hit, a stack trace is recorded.
+
 trace_pipe
 ----------
 

[PATCH 2/2] tracing/blktrace: Fix to allow setting same value

[Masami Hiramatsu]

Current trace-enable attribute in sysfs returns an error
if user writes the same setting value as current one,
e.g.

  # cat /sys/block/sda/trace/enable
  0
  # echo 0 > /sys/block/sda/trace/enable
  bash: echo: write error: Invalid argument
  # echo 1 > /sys/block/sda/trace/enable
  # echo 1 > /sys/block/sda/trace/enable
  bash: echo: write error: Device or resource busy

But this is not a preferred behavior, it should ignore
if new setting is same as current one. This fixes the
problem as below.

  # cat /sys/block/sda/trace/enable
  0
  # echo 0 > /sys/block/sda/trace/enable
  # echo 1 > /sys/block/sda/trace/enable
  # echo 1 > /sys/block/sda/trace/enable

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
---
 kernel/trace/blktrace.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 987d9a9ae283..e67db1e8a000 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q)
 	struct blk_trace *bt;
 
 	bt = xchg(&q->blk_trace, NULL);
-	if (bt == NULL)
-		return -EINVAL;
+	if (bt != NULL) {
+		put_probe_ref();
+		blk_trace_free(bt);
+	}
 
-	put_probe_ref();
-	blk_trace_free(bt);
 	return 0;
 }
 
@@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q,
 	struct blk_trace *bt = NULL;
 	int ret = -ENOMEM;
 
+	if (q->blk_trace)
+		return 0;
+
 	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
 	if (!bt)
 		return -ENOMEM;

Re: [PATCH 1/2] docs: tracing: Add stacktrace filter command

[Steven Rostedt]

On Thu, 16 Aug 2018 18:50:47 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Add a description of stacktrace filter command.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-doc@vger.kernel.org

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Jon, want to pull this patch into your tree?

-- Steve


> ---
>  Documentation/trace/ftrace.rst |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
> index a20d34955333..81378a8b631e 100644
> --- a/Documentation/trace/ftrace.rst
> +++ b/Documentation/trace/ftrace.rst
> @@ -2987,6 +2987,9 @@ The following commands are supported:
>    command, it only prints out the contents of the ring buffer for the
>    CPU that executed the function that triggered the dump.
>  
> +- stacktrace:
> +  When the function is hit, a stack trace is recorded.
> +
>  trace_pipe
>  ----------
>  

Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value

[Steven Rostedt]

On Thu, 16 Aug 2018 18:51:15 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Current trace-enable attribute in sysfs returns an error
> if user writes the same setting value as current one,
> e.g.
> 
>   # cat /sys/block/sda/trace/enable
>   0
>   # echo 0 > /sys/block/sda/trace/enable
>   bash: echo: write error: Invalid argument
>   # echo 1 > /sys/block/sda/trace/enable
>   # echo 1 > /sys/block/sda/trace/enable
>   bash: echo: write error: Device or resource busy
> 
> But this is not a preferred behavior, it should ignore
> if new setting is same as current one. This fixes the
> problem as below.
> 
>   # cat /sys/block/sda/trace/enable
>   0
>   # echo 0 > /sys/block/sda/trace/enable
>   # echo 1 > /sys/block/sda/trace/enable
>   # echo 1 > /sys/block/sda/trace/enable
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org
> ---
>  kernel/trace/blktrace.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 987d9a9ae283..e67db1e8a000 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q)
>  	struct blk_trace *bt;
>  
>  	bt = xchg(&q->blk_trace, NULL);
> -	if (bt == NULL)
> -		return -EINVAL;
> +	if (bt != NULL) {
> +		put_probe_ref();
> +		blk_trace_free(bt);
> +	}
>  
> -	put_probe_ref();
> -	blk_trace_free(bt);
>  	return 0;
>  }
>  
> @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q,
>  	struct blk_trace *bt = NULL;
>  	int ret = -ENOMEM;
>  
> +	if (q->blk_trace)
> +		return 0;
> +
>  	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
>  	if (!bt)
>  		return -ENOMEM;

Hmm, why not just do this? Does this fix it too?

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 987d9a9ae283..10b2b9c5fd25 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	mutex_lock(&q->blk_trace_mutex);
 
 	if (attr == &dev_attr_enable) {
+		if (!!value == !!q->blk_trace)
+			goto out_unlock_bdev;
 		if (value)
 			ret = blk_trace_setup_queue(q, bdev);
 		else

-- Steve

Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value

[Masami Hiramatsu]

On Thu, 16 Aug 2018 10:38:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 16 Aug 2018 18:51:15 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Current trace-enable attribute in sysfs returns an error
> > if user writes the same setting value as current one,
> > e.g.
> > 
> >   # cat /sys/block/sda/trace/enable
> >   0
> >   # echo 0 > /sys/block/sda/trace/enable
> >   bash: echo: write error: Invalid argument
> >   # echo 1 > /sys/block/sda/trace/enable
> >   # echo 1 > /sys/block/sda/trace/enable
> >   bash: echo: write error: Device or resource busy
> > 
> > But this is not a preferred behavior, it should ignore
> > if new setting is same as current one. This fixes the
> > problem as below.
> > 
> >   # cat /sys/block/sda/trace/enable
> >   0
> >   # echo 0 > /sys/block/sda/trace/enable
> >   # echo 1 > /sys/block/sda/trace/enable
> >   # echo 1 > /sys/block/sda/trace/enable
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: linux-block@vger.kernel.org
> > ---
> >  kernel/trace/blktrace.c |   11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > index 987d9a9ae283..e67db1e8a000 100644
> > --- a/kernel/trace/blktrace.c
> > +++ b/kernel/trace/blktrace.c
> > @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q)
> >  	struct blk_trace *bt;
> >  
> >  	bt = xchg(&q->blk_trace, NULL);
> > -	if (bt == NULL)
> > -		return -EINVAL;
> > +	if (bt != NULL) {
> > +		put_probe_ref();
> > +		blk_trace_free(bt);
> > +	}
> >  
> > -	put_probe_ref();
> > -	blk_trace_free(bt);
> >  	return 0;
> >  }
> >  
> > @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q,
> >  	struct blk_trace *bt = NULL;
> >  	int ret = -ENOMEM;
> >  
> > +	if (q->blk_trace)
> > +		return 0;
> > +
> >  	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
> >  	if (!bt)
> >  		return -ENOMEM;
> 
> Hmm, why not just do this? Does this fix it too?

Ah, yes, but we need to clear "ret" in that case.

Thanks!

> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 987d9a9ae283..10b2b9c5fd25 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
>  	mutex_lock(&q->blk_trace_mutex);
>  
>  	if (attr == &dev_attr_enable) {
> +		if (!!value == !!q->blk_trace)
> +			goto out_unlock_bdev;
>  		if (value)
>  			ret = blk_trace_setup_queue(q, bdev);
>  		else
> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value

[Steven Rostedt]

On Fri, 17 Aug 2018 01:38:47 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 16 Aug 2018 10:38:02 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 16 Aug 2018 18:51:15 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > Current trace-enable attribute in sysfs returns an error
> > > if user writes the same setting value as current one,
> > > e.g.
> > > 
> > >   # cat /sys/block/sda/trace/enable
> > >   0
> > >   # echo 0 > /sys/block/sda/trace/enable
> > >   bash: echo: write error: Invalid argument
> > >   # echo 1 > /sys/block/sda/trace/enable
> > >   # echo 1 > /sys/block/sda/trace/enable
> > >   bash: echo: write error: Device or resource busy
> > > 
> > > But this is not a preferred behavior, it should ignore
> > > if new setting is same as current one. This fixes the
> > > problem as below.
> > > 
> > >   # cat /sys/block/sda/trace/enable
> > >   0
> > >   # echo 0 > /sys/block/sda/trace/enable
> > >   # echo 1 > /sys/block/sda/trace/enable
> > >   # echo 1 > /sys/block/sda/trace/enable
> > > 
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: Jens Axboe <axboe@kernel.dk>
> > > Cc: linux-block@vger.kernel.org
> > > ---
> > >  kernel/trace/blktrace.c |   11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > index 987d9a9ae283..e67db1e8a000 100644
> > > --- a/kernel/trace/blktrace.c
> > > +++ b/kernel/trace/blktrace.c
> > > @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q)
> > >  	struct blk_trace *bt;
> > >  
> > >  	bt = xchg(&q->blk_trace, NULL);
> > > -	if (bt == NULL)
> > > -		return -EINVAL;
> > > +	if (bt != NULL) {
> > > +		put_probe_ref();
> > > +		blk_trace_free(bt);
> > > +	}
> > >  
> > > -	put_probe_ref();
> > > -	blk_trace_free(bt);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q,
> > >  	struct blk_trace *bt = NULL;
> > >  	int ret = -ENOMEM;
> > >  
> > > +	if (q->blk_trace)
> > > +		return 0;
> > > +
> > >  	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
> > >  	if (!bt)
> > >  		return -ENOMEM;  
> > 
> > Hmm, why not just do this? Does this fix it too?  
> 
> Ah, yes, but we need to clear "ret" in that case.

Sure.

> 
> Thanks!
> 
> > 
> > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > index 987d9a9ae283..10b2b9c5fd25 100644
> > --- a/kernel/trace/blktrace.c
> > +++ b/kernel/trace/blktrace.c
> > @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
> >  	mutex_lock(&q->blk_trace_mutex);
> >  

> >  	if (attr == &dev_attr_enable) {
> > +		if (!!value == !!q->blk_trace) {
    +                   ret = 0;
> > +			goto out_unlock_bdev;
    +		}

-- Steve

> >  		if (value)
> >  			ret = blk_trace_setup_queue(q, bdev);
> >  		else
> > 
> > -- Steve  
> 
> 

Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value

[Masami Hiramatsu]

On Thu, 16 Aug 2018 12:49:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > > 
> > > Hmm, why not just do this? Does this fix it too?  
> > 
> > Ah, yes, but we need to clear "ret" in that case.
> 
> Sure.
> 
> > 
> > Thanks!
> > 
> > > 
> > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > index 987d9a9ae283..10b2b9c5fd25 100644
> > > --- a/kernel/trace/blktrace.c
> > > +++ b/kernel/trace/blktrace.c
> > > @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
> > >  	mutex_lock(&q->blk_trace_mutex);
> > >  
> 
> > >  	if (attr == &dev_attr_enable) {
> > > +		if (!!value == !!q->blk_trace) {
>     +                   ret = 0;
> > > +			goto out_unlock_bdev;
>     +		}
> 

OK, I tested and ensured that this works.

Tested-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/2] docs: tracing: Add stacktrace filter command

[Jonathan Corbet]

On Thu, 16 Aug 2018 08:50:31 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 16 Aug 2018 18:50:47 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Add a description of stacktrace filter command.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: linux-doc@vger.kernel.org  
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> Jon, want to pull this patch into your tree?

Applied (finally), thanks.

jon