| Message ID | 20200629234314.10509-11-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:13PM -0700, Chaitanya Kulkarni wrote: > The only difference in block_get_rq and block_bio was the last param > passed __entry->nr_sector & bio->bi_iter.bi_size respectively. Since > that is not the case anymore replace block_get_rq class with block_bio > for block_getrq and block_sleeprq events, also adjust the code to handle > null bio case in block_bio. To me it seems like keeping the NULL bio case separate actually is a little simpler..
On 6/29/20 10:13 PM, Christoph Hellwig wrote: > On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote: >> The only difference in block_get_rq and block_bio was the last param >> passed __entry->nr_sector & bio->bi_iter.bi_size respectively. Since >> that is not the case anymore replace block_get_rq class with block_bio >> for block_getrq and block_sleeprq events, also adjust the code to handle >> null bio case in block_bio. > To me it seems like keeping the NULL bio case separate actually is a > little simpler.. > > Keeping it separate will have an extra event class and related event(s) for only handling null bio case. Also the block_get_rq class uses 4 comparisons with ?:. This patch reduces it to only one comparison in fast path. With above explanation does it make sense to get rid of the blk_get_rq ?
On Wed, Jul 01, 2020 at 04:45:03AM +0000, Chaitanya Kulkarni wrote: > On 6/29/20 10:13 PM, Christoph Hellwig wrote: > > On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote: > >> The only difference in block_get_rq and block_bio was the last param > >> passed __entry->nr_sector & bio->bi_iter.bi_size respectively. Since > >> that is not the case anymore replace block_get_rq class with block_bio > >> for block_getrq and block_sleeprq events, also adjust the code to handle > >> null bio case in block_bio. > > To me it seems like keeping the NULL bio case separate actually is a > > little simpler.. > > > > > > Keeping it separate will have an extra event class and related > event(s) for only handling null bio case. > > Also the block_get_rq class uses 4 comparisons with ?:. > This patch reduces it to only one comparison in fast path. > > With above explanation does it make sense to get rid of the > blk_get_rq ? Without this we don't need the request_queue argument to the bio class, as we can derive it from the bio, and don't have any conditionals at all. I'd rather keep the special case with a queue and an optional bio separate.
On 6/30/20 11:19 PM, Christoph Hellwig wrote: > On Wed, Jul 01, 2020 at 04:45:03AM +0000, Chaitanya Kulkarni wrote: >> On 6/29/20 10:13 PM, Christoph Hellwig wrote: >>> On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote: >>>> The only difference in block_get_rq and block_bio was the last param >>>> passed __entry->nr_sector & bio->bi_iter.bi_size respectively. Since >>>> that is not the case anymore replace block_get_rq class with block_bio >>>> for block_getrq and block_sleeprq events, also adjust the code to handle >>>> null bio case in block_bio. >>> To me it seems like keeping the NULL bio case separate actually is a >>> little simpler.. >>> >>> >> Keeping it separate will have an extra event class and related >> event(s) for only handling null bio case. >> >> Also the block_get_rq class uses 4 comparisons with ?:. >> This patch reduces it to only one comparison in fast path. >> >> With above explanation does it make sense to get rid of the >> blk_get_rq ? > Without this we don't need the request_queue argument to the bio > class, as we can derive it from the bio, and don't have any > conditionals at all. I'd rather keep the special case with a > queue and an optional bio separate. > Okay.
diff --git a/include/trace/events/block.h b/include/trace/events/block.h index d191d2cd1070..21f1daaf012b 100644 --- a/include/trace/events/block.h +++ b/include/trace/events/block.h @@ -272,11 +272,19 @@ DECLARE_EVENT_CLASS(block_bio, ), TP_fast_assign( - __entry->dev = bio_dev(bio); - __entry->sector = bio->bi_iter.bi_sector; - __entry->nr_sector = bio_sectors(bio); - blk_fill_rwbs(__entry->rwbs, bio->bi_opf); - memcpy(__entry->comm, current->comm, TASK_COMM_LEN); + if (bio) { + __entry->dev = bio_dev(bio); + __entry->sector = bio->bi_iter.bi_sector; + __entry->nr_sector = bio_sectors(bio); + blk_fill_rwbs(__entry->rwbs, bio->bi_opf); + memcpy(__entry->comm, current->comm, TASK_COMM_LEN); + } else { + __entry->dev = 0; + __entry->sector = 0; + __entry->nr_sector = 0; + blk_fill_rwbs(__entry->rwbs, 0); + memcpy(__entry->comm, current->comm, TASK_COMM_LEN); + } ), TP_printk("%d,%d %s %llu + %u [%s]", @@ -380,7 +388,7 @@ DECLARE_EVENT_CLASS(block_get_rq, * A request struct for queue has been allocated to handle the * block IO operation @bio. */ -DEFINE_EVENT(block_get_rq, block_getrq, +DEFINE_EVENT(block_bio, block_getrq, TP_PROTO(struct bio *bio), @@ -396,7 +404,7 @@ DEFINE_EVENT(block_get_rq, block_getrq, * available. This tracepoint event is generated each time the * process goes to sleep waiting for request struct become available. */ -DEFINE_EVENT(block_get_rq, block_sleeprq, +DEFINE_EVENT(block_bio, block_sleeprq, TP_PROTO(struct bio *bio),
The only difference in block_get_rq and block_bio was the last param passed __entry->nr_sector & bio->bi_iter.bi_size respectively. Since that is not the case anymore replace block_get_rq class with block_bio for block_getrq and block_sleeprq events, also adjust the code to handle null bio case in block_bio. Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> --- include/trace/events/block.h | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)