* [PATCH] blk: optimize blk_trace macros for cgroup case
@ 2015-04-11 21:17 Dmitry Monakhov
2015-04-17 11:23 ` [PATCH] blk: optimize blk_trace macros for cgroup case PING Dmitry Monakhov
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Monakhov @ 2015-04-11 21:17 UTC (permalink / raw)
To: linux-kernel; +Cc: axboe, Dmitry Monakhov
Usually blk_trace is not active, but cfq_log_xxx macros unconditionally
prepare cgroup path via blkg_path() which is suboptimal. This provoke
significant performance overhead
## Test
#modprobe null_blk queue_mode=1
#echo cfq > /sys/block/nullb0/queue/scheduler
#fio --ioengine=libaio --direct=1 --group_reporting --filename=/dev/nullb0 \
--rw=write --iodepth=64 --bs=4k --numjobs=8 --size=4G
| | baseline | w/ patch | gain |
| WR iops | 161483 | 189989 | +17% |
| stddev | 14.36 | 13.19 | |
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
block/blk-throttle.c | 17 ++++++++---------
block/cfq-iosched.c | 22 +++++++++++++---------
include/linux/blktrace_api.h | 5 +++++
3 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 5b9c6d5..6ce9750 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -238,21 +238,20 @@ 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 ((__tg)) { \
- char __pbuf[128]; \
+ if (is_blk_trace_active(__td->queue)) { \
+ if ((__tg)) { \
+ char __pbuf[128]; \
\
- blkg_path(tg_to_blkg(__tg), __pbuf, sizeof(__pbuf)); \
- blk_add_trace_msg(__td->queue, "throtl %s " fmt, __pbuf, ##args); \
- } else { \
- blk_add_trace_msg(__td->queue, "throtl " fmt, ##args); \
+ blkg_path(tg_to_blkg(__tg), __pbuf, sizeof(__pbuf)); \
+ blk_add_trace_msg(__td->queue, "throtl %s " fmt, __pbuf, ##args); \
+ } else { \
+ blk_add_trace_msg(__td->queue, "throtl " fmt, ##args); \
+ } \
} \
} while (0)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 5da8e6e..7b90897 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -625,20 +625,24 @@ static inline void cfqg_put(struct cfq_group *cfqg)
}
#define cfq_log_cfqq(cfqd, cfqq, fmt, args...) do { \
- char __pbuf[128]; \
+ if (is_blk_trace_active((cfqd)->queue)) { \
+ char __pbuf[128]; \
\
- blkg_path(cfqg_to_blkg((cfqq)->cfqg), __pbuf, sizeof(__pbuf)); \
- blk_add_trace_msg((cfqd)->queue, "cfq%d%c%c %s " fmt, (cfqq)->pid, \
- cfq_cfqq_sync((cfqq)) ? 'S' : 'A', \
- cfqq_type((cfqq)) == SYNC_NOIDLE_WORKLOAD ? 'N' : ' ',\
- __pbuf, ##args); \
+ blkg_path(cfqg_to_blkg((cfqq)->cfqg), __pbuf, sizeof(__pbuf)); \
+ blk_add_trace_msg((cfqd)->queue, "cfq%d%c%c %s " fmt, (cfqq)->pid, \
+ cfq_cfqq_sync((cfqq)) ? 'S' : 'A', \
+ cfqq_type((cfqq)) == SYNC_NOIDLE_WORKLOAD ? 'N' : ' ', \
+ __pbuf, ##args); \
+ } \
} while (0)
#define cfq_log_cfqg(cfqd, cfqg, fmt, args...) do { \
- char __pbuf[128]; \
+ if (is_blk_trace_active((cfqd)->queue)) { \
+ char __pbuf[128]; \
\
- blkg_path(cfqg_to_blkg(cfqg), __pbuf, sizeof(__pbuf)); \
- blk_add_trace_msg((cfqd)->queue, "%s " fmt, __pbuf, ##args); \
+ blkg_path(cfqg_to_blkg(cfqg), __pbuf, sizeof(__pbuf)); \
+ blk_add_trace_msg((cfqd)->queue, "%s " fmt, __pbuf, ##args); \
+ } \
} while (0)
static inline void cfqg_stats_update_io_add(struct cfq_group *cfqg,
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index afc1343..2981c9e 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -28,6 +28,11 @@ struct blk_trace {
atomic_t dropped;
};
+static inline bool is_blk_trace_active(struct request_queue *q)
+{
+ return q->blk_trace;
+}
+
extern int blk_trace_ioctl(struct block_device *, unsigned, char __user *);
extern void blk_trace_shutdown(struct request_queue *);
extern int do_blk_trace_setup(struct request_queue *q, char *name,
--
1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] blk: optimize blk_trace macros for cgroup case PING..
2015-04-11 21:17 [PATCH] blk: optimize blk_trace macros for cgroup case Dmitry Monakhov
@ 2015-04-17 11:23 ` Dmitry Monakhov
2015-04-17 15:10 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Monakhov @ 2015-04-17 11:23 UTC (permalink / raw)
To: axboe; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4582 bytes --]
Hi Jens, do you have any objections against this patch?
Please let me know if you have any and I'll fix it.
Dmitry Monakhov <dmonakhov@openvz.org> writes:
> Usually blk_trace is not active, but cfq_log_xxx macros unconditionally
> prepare cgroup path via blkg_path() which is suboptimal. This provoke
> significant performance overhead
>
> ## Test
> #modprobe null_blk queue_mode=1
> #echo cfq > /sys/block/nullb0/queue/scheduler
> #fio --ioengine=libaio --direct=1 --group_reporting --filename=/dev/nullb0 \
> --rw=write --iodepth=64 --bs=4k --numjobs=8 --size=4G
> | | baseline | w/ patch | gain |
> | WR iops | 161483 | 189989 | +17% |
> | stddev | 14.36 | 13.19 | |
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
> block/blk-throttle.c | 17 ++++++++---------
> block/cfq-iosched.c | 22 +++++++++++++---------
> include/linux/blktrace_api.h | 5 +++++
> 3 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 5b9c6d5..6ce9750 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -238,21 +238,20 @@ 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 ((__tg)) { \
> - char __pbuf[128]; \
> + if (is_blk_trace_active(__td->queue)) { \
> + if ((__tg)) { \
> + char __pbuf[128]; \
> \
> - blkg_path(tg_to_blkg(__tg), __pbuf, sizeof(__pbuf)); \
> - blk_add_trace_msg(__td->queue, "throtl %s " fmt, __pbuf, ##args); \
> - } else { \
> - blk_add_trace_msg(__td->queue, "throtl " fmt, ##args); \
> + blkg_path(tg_to_blkg(__tg), __pbuf, sizeof(__pbuf)); \
> + blk_add_trace_msg(__td->queue, "throtl %s " fmt, __pbuf, ##args); \
> + } else { \
> + blk_add_trace_msg(__td->queue, "throtl " fmt, ##args); \
> + } \
> } \
> } while (0)
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 5da8e6e..7b90897 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -625,20 +625,24 @@ static inline void cfqg_put(struct cfq_group *cfqg)
> }
>
> #define cfq_log_cfqq(cfqd, cfqq, fmt, args...) do { \
> - char __pbuf[128]; \
> + if (is_blk_trace_active((cfqd)->queue)) { \
> + char __pbuf[128]; \
> \
> - blkg_path(cfqg_to_blkg((cfqq)->cfqg), __pbuf, sizeof(__pbuf)); \
> - blk_add_trace_msg((cfqd)->queue, "cfq%d%c%c %s " fmt, (cfqq)->pid, \
> - cfq_cfqq_sync((cfqq)) ? 'S' : 'A', \
> - cfqq_type((cfqq)) == SYNC_NOIDLE_WORKLOAD ? 'N' : ' ',\
> - __pbuf, ##args); \
> + blkg_path(cfqg_to_blkg((cfqq)->cfqg), __pbuf, sizeof(__pbuf)); \
> + blk_add_trace_msg((cfqd)->queue, "cfq%d%c%c %s " fmt, (cfqq)->pid, \
> + cfq_cfqq_sync((cfqq)) ? 'S' : 'A', \
> + cfqq_type((cfqq)) == SYNC_NOIDLE_WORKLOAD ? 'N' : ' ', \
> + __pbuf, ##args); \
> + } \
> } while (0)
>
> #define cfq_log_cfqg(cfqd, cfqg, fmt, args...) do { \
> - char __pbuf[128]; \
> + if (is_blk_trace_active((cfqd)->queue)) { \
> + char __pbuf[128]; \
> \
> - blkg_path(cfqg_to_blkg(cfqg), __pbuf, sizeof(__pbuf)); \
> - blk_add_trace_msg((cfqd)->queue, "%s " fmt, __pbuf, ##args); \
> + blkg_path(cfqg_to_blkg(cfqg), __pbuf, sizeof(__pbuf)); \
> + blk_add_trace_msg((cfqd)->queue, "%s " fmt, __pbuf, ##args); \
> + } \
> } while (0)
>
> static inline void cfqg_stats_update_io_add(struct cfq_group *cfqg,
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index afc1343..2981c9e 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -28,6 +28,11 @@ struct blk_trace {
> atomic_t dropped;
> };
>
> +static inline bool is_blk_trace_active(struct request_queue *q)
> +{
> + return q->blk_trace;
> +}
> +
> extern int blk_trace_ioctl(struct block_device *, unsigned, char __user *);
> extern void blk_trace_shutdown(struct request_queue *);
> extern int do_blk_trace_setup(struct request_queue *q, char *name,
> --
> 1.7.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] blk: optimize blk_trace macros for cgroup case PING..
2015-04-17 11:23 ` [PATCH] blk: optimize blk_trace macros for cgroup case PING Dmitry Monakhov
@ 2015-04-17 15:10 ` Jens Axboe
0 siblings, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2015-04-17 15:10 UTC (permalink / raw)
To: Dmitry Monakhov; +Cc: linux-kernel
On 04/17/2015 05:23 AM, Dmitry Monakhov wrote:
>
> Hi Jens, do you have any objections against this patch?
> Please let me know if you have any and I'll fix it.
I think it looks fine, and it's a nice win! So good catch. I'll queue it
up for inclusion later in this cycle.
--
Jens Axboe
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-04-17 15:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-11 21:17 [PATCH] blk: optimize blk_trace macros for cgroup case Dmitry Monakhov
2015-04-17 11:23 ` [PATCH] blk: optimize blk_trace macros for cgroup case PING Dmitry Monakhov
2015-04-17 15:10 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox