| Message ID | b395946862a1a963e6d74b70349322eceb5d42d4.1462838969.git.shli@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)