| Message ID | 20200629234314.10509-6-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: blktrace framework cleanup | expand |
On Mon, Jun 29, 2020 at 04:43:08PM -0700, Chaitanya Kulkarni wrote: > Get rid of the wrapper for trace_block_rq_insert() and call the function > directly. I'd mention blk_mq_sched_request_inserted instead of the tracepoint in the subject and commit message. Otherwise this looks fine.
On 6/29/20 10:11 PM, Christoph Hellwig wrote: > On Mon, Jun 29, 2020 at 04:43:08PM -0700, Chaitanya Kulkarni wrote: >> Get rid of the wrapper for trace_block_rq_insert() and call the function >> directly. > I'd mention blk_mq_sched_request_inserted instead of the tracepoint > in the subject and commit message. Otherwise this looks fine. > Okay, will change the message.
Christoph, On 6/29/20 4:44 PM, Chaitanya Kulkarni wrote: > Get rid of the wrapper for trace_block_rq_insert() and call the function > directly. > > Signed-off-by: Chaitanya Kulkarni<chaitanya.kulkarni@wdc.com> > --- Can we re-consider adding this patch ? here are couple of reasons:- 1. Increase in the size of the text region of the object file:- By adding the trace header #include <trace/events/block.h> in io-scheduler where it is calling trace_block_rq_insert() increases the size of the text region of the object file kyber(+215) & bfq (+317) [1]. 2. Mandatory io-sched built-in kernel compilation:- When testing with a different io-sched KConfig options ("*" vs "M"), when kyber and bfq compilation option set to "M" having this patch reports error[2]. If I've not missed something here then can we drop this patch ? In case we really want to do this change it will need to have KConfig separate patch such that if tracing is selected it will force * selection (built-in KConfig) for schedulers in question and etc. Do we want to do this ? Regards, Chaitanya [1] Scheduler IO object size comparison :- Without this patch :- --------------------- # size block/bfq-iosched.o text data bss dec hex filename 62204 1011 32 63247 f70f block/bfq-iosched.o # size block/kyber-iosched.o text data bss dec hex filename 14808 2699 48 17555 4493 block/kyber-iosched.o With this patch :- ------------------ # size block/bfq-iosched.o text data bss dec hex filename 62521 1028 32 63581 f85d block/bfq-iosched.o # size block/kyber-iosched.o text data bss dec hex filename 15023 2716 48 17787 457b block/kyber-iosched.o [2] Error with selecting M for io-sched kyber & bfq :- ERROR: modpost: "__tracepoint_block_rq_insert" [block/bfq.ko] undefined! ERROR: modpost: "__tracepoint_block_rq_insert" [block/kyber-iosched.ko] undefined! make[2]: *** [Module.symvers] Error 1 make[2]: *** Deleting file `Module.symvers' make[1]: *** [modules] Error 2 make[1]: *** Waiting for unfinished jobs.... arch/x86/tools/insn_decoder_test: success: Decoded and checked 4932572 instructions TEST posttest arch/x86/tools/insn_sanity: Success: decoded and checked 1000000 random instructions with 0 errors (seed:0x4c6e1a40) CC arch/x86/boot/version.o VOFFSET arch/x86/boot/compressed/../voffset.h OBJCOPY arch/x86/boot/compressed/vmlinux.bin RELOCS arch/x86/boot/compressed/vmlinux.relocs CC arch/x86/boot/compressed/kaslr.o CC arch/x86/boot/compressed/misc.o GZIP arch/x86/boot/compressed/vmlinux.bin.gz MKPIGGY arch/x86/boot/compressed/piggy.S AS arch/x86/boot/compressed/piggy.o LD arch/x86/boot/compressed/vmlinux ZOFFSET arch/x86/boot/zoffset.h OBJCOPY arch/x86/boot/vmlinux.bin AS arch/x86/boot/header.o LD arch/x86/boot/setup.elf OBJCOPY arch/x86/boot/setup.bin BUILD arch/x86/boot/bzImage Setup is 15132 bytes (padded to 15360 bytes). System is 8951 kB CRC ff6eac72 Kernel: arch/x86/boot/bzImage is ready (#59) make: *** [__sub-make] Error 2
On Fri, Jul 03, 2020 at 11:29:25PM +0000, Chaitanya Kulkarni wrote: > Christoph, > > On 6/29/20 4:44 PM, Chaitanya Kulkarni wrote: > > Get rid of the wrapper for trace_block_rq_insert() and call the function > > directly. > > > > Signed-off-by: Chaitanya Kulkarni<chaitanya.kulkarni@wdc.com> > > --- > > Can we re-consider adding this patch ? here are couple of reasons:- > > 1. Increase in the size of the text region of the object file:- > > By adding the trace header #include <trace/events/block.h> > in io-scheduler where it is calling trace_block_rq_insert() > increases the size of the text region of the object file > kyber(+215) & bfq (+317) [1]. You really shouldn't have both loaded, never mind used at the same time. It also avoid a function call for the scheduler fast path. > 2. Mandatory io-sched built-in kernel compilation:- > > When testing with a different io-sched KConfig options ("*" vs "M"), > when kyber and bfq compilation option set to "M" having this patch > reports error[2]. See EXPORT_TRACEPOINT_SYMBOL_GPL.
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 50c8f034c01c..e2b9b700ed34 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -125,6 +125,8 @@ #include <linux/delay.h> #include <linux/backing-dev.h> +#include <trace/events/block.h> + #include "blk.h" #include "blk-mq.h" #include "blk-mq-tag.h" @@ -5507,7 +5509,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, spin_unlock_irq(&bfqd->lock); - blk_mq_sched_request_inserted(rq); + trace_block_rq_insert(rq); spin_lock_irq(&bfqd->lock); bfqq = bfq_init_rq(rq); diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index a3cade16ef80..20b6a59fbd5a 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -407,12 +407,6 @@ bool blk_mq_sched_try_insert_merge(struct request_queue *q, struct request *rq) } EXPORT_SYMBOL_GPL(blk_mq_sched_try_insert_merge); -void blk_mq_sched_request_inserted(struct request *rq) -{ - trace_block_rq_insert(rq); -} -EXPORT_SYMBOL_GPL(blk_mq_sched_request_inserted); - static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx, bool has_sched, struct request *rq) diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h index 126021fc3a11..04c40c695bf0 100644 --- a/block/blk-mq-sched.h +++ b/block/blk-mq-sched.h @@ -10,7 +10,6 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q, void blk_mq_sched_assign_ioc(struct request *rq); -void blk_mq_sched_request_inserted(struct request *rq); bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio, unsigned int nr_segs, struct request **merged_request); bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio, diff --git a/block/kyber-iosched.c b/block/kyber-iosched.c index a38c5ab103d1..e42d78deee90 100644 --- a/block/kyber-iosched.c +++ b/block/kyber-iosched.c @@ -13,6 +13,8 @@ #include <linux/module.h> #include <linux/sbitmap.h> +#include <trace/events/block.h> + #include "blk.h" #include "blk-mq.h" #include "blk-mq-debugfs.h" @@ -602,7 +604,7 @@ static void kyber_insert_requests(struct blk_mq_hw_ctx *hctx, list_move_tail(&rq->queuelist, head); sbitmap_set_bit(&khd->kcq_map[sched_domain], rq->mq_ctx->index_hw[hctx->type]); - blk_mq_sched_request_inserted(rq); + trace_block_rq_insert(rq); spin_unlock(&kcq->lock); } } diff --git a/block/mq-deadline.c b/block/mq-deadline.c index b57470e154c8..f3631a287466 100644 --- a/block/mq-deadline.c +++ b/block/mq-deadline.c @@ -18,6 +18,8 @@ #include <linux/rbtree.h> #include <linux/sbitmap.h> +#include <trace/events/block.h> + #include "blk.h" #include "blk-mq.h" #include "blk-mq-debugfs.h" @@ -496,7 +498,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, if (blk_mq_sched_try_insert_merge(q, rq)) return; - blk_mq_sched_request_inserted(rq); + trace_block_rq_insert(rq); if (at_head || blk_rq_is_passthrough(rq)) { if (at_head)
Get rid of the wrapper for trace_block_rq_insert() and call the function directly. Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> --- block/bfq-iosched.c | 4 +++- block/blk-mq-sched.c | 6 ------ block/blk-mq-sched.h | 1 - block/kyber-iosched.c | 4 +++- block/mq-deadline.c | 4 +++- 5 files changed, 9 insertions(+), 10 deletions(-)