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

Groups > linux.kernel > #1397599 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled Shaohua Li <shli@fb.com> - 2016-05-10 02:30 +0200
    Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled Jeff Moyer <jmoyer@redhat.com> - 2016-05-10 18:00 +0200
      Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace  isn't enabled Steven Rostedt <rostedt@goodmis.org> - 2016-05-11 01:40 +0200
        Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled Jeff Moyer <jmoyer@redhat.com> - 2016-05-11 15:50 +0200
      Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't  enabled Shaohua Li <shli@fb.com> - 2016-05-11 02:30 +0200
        Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't enabled Jeff Moyer <jmoyer@redhat.com> - 2016-05-11 15:50 +0200
          Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't  enabled Shaohua Li <shli@fb.com> - 2016-05-11 19:00 +0200
    Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't  enabled Simon Guo <wei.guo.simon@gmail.com> - 2016-05-11 10:20 +0200
      Re: [PATCH 3/3] blk-throttle: don't parse cgroup path if trace isn't  enabled Shaohua Li <shli@fb.com> - 2016-05-11 19:00 +0200

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

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

[toc] | [next] | [standalone]

#1398275

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;

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

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

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

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

#1399096

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

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

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

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.

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

#1399091

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

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

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

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

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

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web