Re: [PATCH] blk: optimize blk_trace macros for cgroup case PING..

[Dmitry Monakhov]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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: signature.asc
Description: PGP signature