[PATCH 1/3] blktrace: delete garbage for message trace

[PATCH 1/3] blktrace: delete garbage for message trace

[Shaohua Li]

commit f4a1d08ce65 introduces a regression. Originally for
BLK_TN_MESSAGE, we add message in trace and return. The commit ignores
the early return and add garbage info.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 kernel/trace/blktrace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index f94e7a2..8ae9ea2 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1349,6 +1349,7 @@ static enum print_line_t print_one_line(struct trace_iterator *iter,
 	if (t->action == BLK_TN_MESSAGE) {
 		log_action(iter, long_act ? "message" : "m");
 		blk_log_msg(s, iter->ent);
+		return trace_handle_return(s);
 	}
 
 	if (unlikely(what == 0 || what >= ARRAY_SIZE(what2act)))
-- 
2.8.0.rc2

[PATCH 2/3] blktrace: add missed mask name

[Shaohua Li]

BLK_TC_NOTIFY is missed in mask_maps, so we can't print out notify or
set mask with 'notify' name.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 kernel/trace/blktrace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 8ae9ea2..9aef865 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1552,6 +1552,7 @@ static const struct {
 	{ BLK_TC_COMPLETE,	"complete"	},
 	{ BLK_TC_FS,		"fs"		},
 	{ BLK_TC_PC,		"pc"		},
+	{ BLK_TC_NOTIFY,	"notify"	},
 	{ BLK_TC_AHEAD,		"ahead"		},
 	{ BLK_TC_META,		"meta"		},
 	{ BLK_TC_DISCARD,	"discard"	},
-- 
2.8.0.rc2

[PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Shaohua Li]

if trace isn't enabled, parsing cgroup path just wastes cpu

Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/blk-throttle.c         | 5 ++---
 include/linux/blktrace_api.h | 9 +++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 2149a1d..47a3e54 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
  *
  * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
  * throtl_grp; otherwise, just "throtl".
- *
- * TODO: this should be made a function and name formatting should happen
- * after testing whether blktrace is enabled.
  */
 #define throtl_log(sq, fmt, args...)	do {				\
 	struct throtl_grp *__tg = sq_to_tg((sq));			\
 	struct throtl_data *__td = sq_to_td((sq));			\
 									\
 	(void)__td;							\
+	if (likely(!blk_trace_note_message_enabled(__td->queue)))	\
+		break;							\
 	if ((__tg)) {							\
 		char __pbuf[128];					\
 									\
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index afc1343..0f3172b 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -57,6 +57,14 @@ void __trace_note_message(struct blk_trace *, const char *fmt, ...);
 	} while (0)
 #define BLK_TN_MAX_MSG		128
 
+static inline bool blk_trace_note_message_enabled(struct request_queue *q)
+{
+	struct blk_trace *bt = q->blk_trace;
+	if (likely(!bt))
+		return false;
+	return bt->act_mask & BLK_TC_NOTIFY;
+}
+
 extern void blk_add_driver_data(struct request_queue *q, struct request *rq,
 				void *data, size_t len);
 extern int blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
@@ -79,6 +87,7 @@ extern struct attribute_group blk_trace_attr_group;
 # define blk_trace_remove(q)				(-ENOTTY)
 # define blk_add_trace_msg(q, fmt, ...)			do { } while (0)
 # define blk_trace_remove_sysfs(dev)			do { } while (0)
+# define blk_trace_note_message_enabled(q)		(false)
 static inline int blk_trace_init_sysfs(struct device *dev)
 {
 	return 0;
-- 
2.8.0.rc2

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Simon Guo]

Hi, 
On Mon, May 09, 2016 at 05:22:15PM -0700, Shaohua Li wrote:
> if trace isn't enabled, parsing cgroup path just wastes cpu
> 
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  block/blk-throttle.c         | 5 ++---
>  include/linux/blktrace_api.h | 9 +++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 2149a1d..47a3e54 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
>   *
>   * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
>   * throtl_grp; otherwise, just "throtl".
> - *
> - * TODO: this should be made a function and name formatting should happen
> - * after testing whether blktrace is enabled.
>   */
>  #define throtl_log(sq, fmt, args...)	do {				\
>  	struct throtl_grp *__tg = sq_to_tg((sq));			\
>  	struct throtl_data *__td = sq_to_td((sq));			\
>  									\
>  	(void)__td;							\
> +	if (likely(!blk_trace_note_message_enabled(__td->queue)))	\
> +		break;							\
>  	if ((__tg)) {							\
>  		char __pbuf[128];					\
>  									\
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index afc1343..0f3172b 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -57,6 +57,14 @@ void __trace_note_message(struct blk_trace *, const char *fmt, ...);
>  	} while (0)
>  #define BLK_TN_MAX_MSG		128
>  
> +static inline bool blk_trace_note_message_enabled(struct request_queue *q)
> +{
> +	struct blk_trace *bt = q->blk_trace;
> +	if (likely(!bt))
> +		return false;
> +	return bt->act_mask & BLK_TC_NOTIFY;
> +}
Is there any reason to skip following condition?
        if (unlikely(bt->trace_state != Blktrace_running &&
                     !blk_tracer_enabled))

Thanks,
- Simon

Re: [PATCH 1/3] blktrace: delete garbage for message trace

[Jeff Moyer]

Shaohua Li <shli@fb.com> writes:

> commit f4a1d08ce65 introduces a regression. Originally for
> BLK_TN_MESSAGE, we add message in trace and return. The commit ignores
> the early return and add garbage info.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  kernel/trace/blktrace.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index f94e7a2..8ae9ea2 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -1349,6 +1349,7 @@ static enum print_line_t print_one_line(struct trace_iterator *iter,
>  	if (t->action == BLK_TN_MESSAGE) {
>  		log_action(iter, long_act ? "message" : "m");
>  		blk_log_msg(s, iter->ent);
> +		return trace_handle_return(s);
>  	}
>  
>  	if (unlikely(what == 0 || what >= ARRAY_SIZE(what2act)))

It would have been closer to the original to put the 'out' label back
in, but this is fine.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 1/3] blktrace: delete garbage for message trace

[Jens Axboe]

On 05/09/2016 06:22 PM, Shaohua Li wrote:
> commit f4a1d08ce65 introduces a regression. Originally for
> BLK_TN_MESSAGE, we add message in trace and return. The commit ignores
> the early return and add garbage info.

Applied this, and 2+3/3. Thanks Shaohua.

-- 
Jens Axboe

Re: [PATCH 2/3] blktrace: add missed mask name

[Jeff Moyer]

Shaohua Li <shli@fb.com> writes:

> BLK_TC_NOTIFY is missed in mask_maps, so we can't print out notify or
> set mask with 'notify' name.

For the text-based interface, yeah.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  kernel/trace/blktrace.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 8ae9ea2..9aef865 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -1552,6 +1552,7 @@ static const struct {
>  	{ BLK_TC_COMPLETE,	"complete"	},
>  	{ BLK_TC_FS,		"fs"		},
>  	{ BLK_TC_PC,		"pc"		},
> +	{ BLK_TC_NOTIFY,	"notify"	},
>  	{ BLK_TC_AHEAD,		"ahead"		},
>  	{ BLK_TC_META,		"meta"		},
>  	{ BLK_TC_DISCARD,	"discard"	},

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Jeff Moyer]

Shaohua Li <shli@fb.com> writes:

> if trace isn't enabled, parsing cgroup path just wastes cpu
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  block/blk-throttle.c         | 5 ++---
>  include/linux/blktrace_api.h | 9 +++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 2149a1d..47a3e54 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
>   *
>   * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
>   * throtl_grp; otherwise, just "throtl".
> - *
> - * TODO: this should be made a function and name formatting should happen
> - * after testing whether blktrace is enabled.

You've only addressed the second part of the TODO, please don't remove
the first part.

>   */
>  #define throtl_log(sq, fmt, args...)	do {				\
>  	struct throtl_grp *__tg = sq_to_tg((sq));			\
>  	struct throtl_data *__td = sq_to_td((sq));			\
>  									\
>  	(void)__td;							\
> +	if (likely(!blk_trace_note_message_enabled(__td->queue)))	\
> +		break;							\
>  	if ((__tg)) {							\
>  		char __pbuf[128];					\
>  									\
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index afc1343..0f3172b 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -57,6 +57,14 @@ void __trace_note_message(struct blk_trace *, const char *fmt, ...);
>  	} while (0)
>  #define BLK_TN_MAX_MSG		128
>  
> +static inline bool blk_trace_note_message_enabled(struct request_queue *q)
> +{
> +	struct blk_trace *bt = q->blk_trace;
> +	if (likely(!bt))
> +		return false;
> +	return bt->act_mask & BLK_TC_NOTIFY;

Do we want to return !!(bt->act_mask & BLK_TC_NOTIFY)?

-Jeff

> +}
> +
>  extern void blk_add_driver_data(struct request_queue *q, struct request *rq,
>  				void *data, size_t len);
>  extern int blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
> @@ -79,6 +87,7 @@ extern struct attribute_group blk_trace_attr_group;
>  # define blk_trace_remove(q)				(-ENOTTY)
>  # define blk_add_trace_msg(q, fmt, ...)			do { } while (0)
>  # define blk_trace_remove_sysfs(dev)			do { } while (0)
> +# define blk_trace_note_message_enabled(q)		(false)
>  static inline int blk_trace_init_sysfs(struct device *dev)
>  {
>  	return 0;

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Steven Rostedt]

On Tue, 10 May 2016 11:52:15 -0400
Jeff Moyer <jmoyer@redhat.com> wrote:

 
> > +static inline bool blk_trace_note_message_enabled(struct request_queue *q)
> > +{
> > +	struct blk_trace *bt = q->blk_trace;
> > +	if (likely(!bt))
> > +		return false;
> > +	return bt->act_mask & BLK_TC_NOTIFY;  
> 
> Do we want to return !!(bt->act_mask & BLK_TC_NOTIFY)?

The return type is bool. I would think that gcc would be smart enough
to make the conversion.

To check, I compiled the following function:

bool testbool(int x)
{
        return x & 1<<3;
}

and the result was:


0000000000000000 <testbool>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   89 7d fc                mov    %edi,-0x4(%rbp)
   7:   8b 45 fc                mov    -0x4(%rbp),%eax
   a:   83 e0 08                and    $0x8,%eax
   d:   85 c0                   test   %eax,%eax
   f:   0f 95 c0                setne  %al
  12:   5d                      pop    %rbp
  13:   c3                      retq   

I get the same by adding !!(x & 1<<3)

Looks like it does the conversion.

-- Steve

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Shaohua Li]

On Tue, May 10, 2016 at 11:52:15AM -0400, Jeff Moyer wrote:
> Shaohua Li <shli@fb.com> writes:
> 
> > if trace isn't enabled, parsing cgroup path just wastes cpu
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> >  block/blk-throttle.c         | 5 ++---
> >  include/linux/blktrace_api.h | 9 +++++++++
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> > index 2149a1d..47a3e54 100644
> > --- a/block/blk-throttle.c
> > +++ b/block/blk-throttle.c
> > @@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
> >   *
> >   * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
> >   * throtl_grp; otherwise, just "throtl".
> > - *
> > - * TODO: this should be made a function and name formatting should happen
> > - * after testing whether blktrace is enabled.
> 
> You've only addressed the second part of the TODO, please don't remove
> the first part.

alright, I'll send a patch to convert it to a function.

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Jeff Moyer]

Shaohua Li <shli@fb.com> writes:

> On Tue, May 10, 2016 at 11:52:15AM -0400, Jeff Moyer wrote:
>> Shaohua Li <shli@fb.com> writes:
>> 
>> > if trace isn't enabled, parsing cgroup path just wastes cpu
>> >
>> > Signed-off-by: Shaohua Li <shli@fb.com>
>> > ---
>> >  block/blk-throttle.c         | 5 ++---
>> >  include/linux/blktrace_api.h | 9 +++++++++
>> >  2 files changed, 11 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> > index 2149a1d..47a3e54 100644
>> > --- a/block/blk-throttle.c
>> > +++ b/block/blk-throttle.c
>> > @@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
>> >   *
>> >   * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
>> >   * throtl_grp; otherwise, just "throtl".
>> > - *
>> > - * TODO: this should be made a function and name formatting should happen
>> > - * after testing whether blktrace is enabled.
>> 
>> You've only addressed the second part of the TODO, please don't remove
>> the first part.
>
> alright, I'll send a patch to convert it to a function.

Heh.  I figured you'd just put that part of the TODO back in there.  But
actually addressing it sounds better.  ;-)

Thanks!
Jeff

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Jeff Moyer]

Steven Rostedt <rostedt@goodmis.org> writes:

>> Do we want to return !!(bt->act_mask & BLK_TC_NOTIFY)?
>
> The return type is bool. I would think that gcc would be smart enough
> to make the conversion.
>
> To check, I compiled the following function:
>
> bool testbool(int x)
> {
>         return x & 1<<3;
> }
>
> and the result was:
>
>
> 0000000000000000 <testbool>:
>    0:   55                      push   %rbp
>    1:   48 89 e5                mov    %rsp,%rbp
>    4:   89 7d fc                mov    %edi,-0x4(%rbp)
>    7:   8b 45 fc                mov    -0x4(%rbp),%eax
>    a:   83 e0 08                and    $0x8,%eax
>    d:   85 c0                   test   %eax,%eax
>    f:   0f 95 c0                setne  %al
>   12:   5d                      pop    %rbp
>   13:   c3                      retq   
>
> I get the same by adding !!(x & 1<<3)
>
> Looks like it does the conversion.

Cool, thanks!

-Jeff

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Shaohua Li]

On Wed, May 11, 2016 at 09:41:20AM -0400, Jeff Moyer wrote:
> Shaohua Li <shli@fb.com> writes:
> 
> > On Tue, May 10, 2016 at 11:52:15AM -0400, Jeff Moyer wrote:
> >> Shaohua Li <shli@fb.com> writes:
> >> 
> >> > if trace isn't enabled, parsing cgroup path just wastes cpu
> >> >
> >> > Signed-off-by: Shaohua Li <shli@fb.com>
> >> > ---
> >> >  block/blk-throttle.c         | 5 ++---
> >> >  include/linux/blktrace_api.h | 9 +++++++++
> >> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> >> > index 2149a1d..47a3e54 100644
> >> > --- a/block/blk-throttle.c
> >> > +++ b/block/blk-throttle.c
> >> > @@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
> >> >   *
> >> >   * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
> >> >   * throtl_grp; otherwise, just "throtl".
> >> > - *
> >> > - * TODO: this should be made a function and name formatting should happen
> >> > - * after testing whether blktrace is enabled.
> >> 
> >> You've only addressed the second part of the TODO, please don't remove
> >> the first part.
> >
> > alright, I'll send a patch to convert it to a function.
> 
> Heh.  I figured you'd just put that part of the TODO back in there.  But
> actually addressing it sounds better.  ;-)

Actually I'd like to give up. To convert it to a function, I need add a
var arg version of blk_add_trace_msg. And throtl_log adds new parameter
and change fmt, so I must allocate a new string to fit old fmt and new
parameter and new fmt string. It's not worthy the effort I think.

Thanks,
Shaohua

Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled

[Shaohua Li]

On Tue, May 10, 2016 at 01:46:13PM +0800, Simon Guo wrote:
> Hi, 
> On Mon, May 09, 2016 at 05:22:15PM -0700, Shaohua Li wrote:
> > if trace isn't enabled, parsing cgroup path just wastes cpu
> > 
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> >  block/blk-throttle.c         | 5 ++---
> >  include/linux/blktrace_api.h | 9 +++++++++
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> > index 2149a1d..47a3e54 100644
> > --- a/block/blk-throttle.c
> > +++ b/block/blk-throttle.c
> > @@ -211,15 +211,14 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
> >   *
> >   * The messages are prefixed with "throtl BLKG_NAME" if @sq belongs to a
> >   * throtl_grp; otherwise, just "throtl".
> > - *
> > - * TODO: this should be made a function and name formatting should happen
> > - * after testing whether blktrace is enabled.
> >   */
> >  #define throtl_log(sq, fmt, args...)	do {				\
> >  	struct throtl_grp *__tg = sq_to_tg((sq));			\
> >  	struct throtl_data *__td = sq_to_td((sq));			\
> >  									\
> >  	(void)__td;							\
> > +	if (likely(!blk_trace_note_message_enabled(__td->queue)))	\
> > +		break;							\
> >  	if ((__tg)) {							\
> >  		char __pbuf[128];					\
> >  									\
> > diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> > index afc1343..0f3172b 100644
> > --- a/include/linux/blktrace_api.h
> > +++ b/include/linux/blktrace_api.h
> > @@ -57,6 +57,14 @@ void __trace_note_message(struct blk_trace *, const char *fmt, ...);
> >  	} while (0)
> >  #define BLK_TN_MAX_MSG		128
> >  
> > +static inline bool blk_trace_note_message_enabled(struct request_queue *q)
> > +{
> > +	struct blk_trace *bt = q->blk_trace;
> > +	if (likely(!bt))
> > +		return false;
> > +	return bt->act_mask & BLK_TC_NOTIFY;
> > +}
> Is there any reason to skip following condition?
>         if (unlikely(bt->trace_state != Blktrace_running &&
>                      !blk_tracer_enabled))

It's marked unlikely and to add the check we must export
blk_tracer_enabled, so I ignored it. Though I don't know how unlikely it
is.

Thanks,
Shaohua