| Message ID | 20240614074936.113659-1-dongliang.cui@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] block: Add ioprio to block_rq tracepoint | expand |
On 6/14/24 12:49 AM, Dongliang Cui wrote: > - TP_printk("%d,%d %s (%s) %llu + %u [%d]", > + TP_printk("%d,%d %s (%s) %llu + %u %s,%u,%u [%d]", > MAJOR(__entry->dev), MINOR(__entry->dev), > __entry->rwbs, __get_str(cmd), > - (unsigned long long)__entry->sector, > - __entry->nr_sector, 0) > + (unsigned long long)__entry->sector, __entry->nr_sector, > + __print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio), > + IOPRIO_CLASS_STRINGS), > + IOPRIO_PRIO_HINT(__entry->ioprio), > + IOPRIO_PRIO_LEVEL(__entry->ioprio), 0) > ); Do we really want to include the constant "[0]" in the tracing output? Otherwise this patch looks good to me. Thanks, Bart.
On Sat, Jun 15, 2024 at 12:41 AM Bart Van Assche <bvanassche@acm.org> wrote: > > On 6/14/24 12:49 AM, Dongliang Cui wrote: > > - TP_printk("%d,%d %s (%s) %llu + %u [%d]", > > + TP_printk("%d,%d %s (%s) %llu + %u %s,%u,%u [%d]", > > MAJOR(__entry->dev), MINOR(__entry->dev), > > __entry->rwbs, __get_str(cmd), > > - (unsigned long long)__entry->sector, > > - __entry->nr_sector, 0) > > + (unsigned long long)__entry->sector, __entry->nr_sector, > > + __print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio), > > + IOPRIO_CLASS_STRINGS), > > + IOPRIO_PRIO_HINT(__entry->ioprio), > > + IOPRIO_PRIO_LEVEL(__entry->ioprio), 0) > > ); > > Do we really want to include the constant "[0]" in the tracing output? This is how it is printed in the source code. From the code flow point of view, there is no need to print this value in trace_block_rq_requeue. Do we need to consider the issue of uniform printing format? If not, I think we can delete it. > > Otherwise this patch looks good to me. > > Thanks, > > Bart. >
On 6/17/24 12:59 AM, dongliang cui wrote: > On Sat, Jun 15, 2024 at 12:41 AM Bart Van Assche <bvanassche@acm.org> wrote: >> >> On 6/14/24 12:49 AM, Dongliang Cui wrote: >>> - TP_printk("%d,%d %s (%s) %llu + %u [%d]", >>> + TP_printk("%d,%d %s (%s) %llu + %u %s,%u,%u [%d]", >>> MAJOR(__entry->dev), MINOR(__entry->dev), >>> __entry->rwbs, __get_str(cmd), >>> - (unsigned long long)__entry->sector, >>> - __entry->nr_sector, 0) >>> + (unsigned long long)__entry->sector, __entry->nr_sector, >>> + __print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio), >>> + IOPRIO_CLASS_STRINGS), >>> + IOPRIO_PRIO_HINT(__entry->ioprio), >>> + IOPRIO_PRIO_LEVEL(__entry->ioprio), 0) >>> ); >> >> Do we really want to include the constant "[0]" in the tracing output? > This is how it is printed in the source code. > From the code flow point of view, there is no need to print this value > in trace_block_rq_requeue. > Do we need to consider the issue of uniform printing format? If not, I > think we can delete it. I'm not aware of any other tracing statement that prints out a constant. Is there perhaps something that I'm missing or overlooking? Thanks, Bart.
On Mon, 17 Jun 2024 10:02:48 -0700 Bart Van Assche <bvanassche@acm.org> wrote: > >> Do we really want to include the constant "[0]" in the tracing output? > > This is how it is printed in the source code. > > From the code flow point of view, there is no need to print this value > > in trace_block_rq_requeue. > > Do we need to consider the issue of uniform printing format? If not, I > > think we can delete it. > > I'm not aware of any other tracing statement that prints out a constant. > Is there perhaps something that I'm missing or overlooking? The only time that is done, is if the trace event is used in multiple places and there's one place that the value will always be the same. -- Steve
On 6/17/24 10:07 AM, Steven Rostedt wrote: > On Mon, 17 Jun 2024 10:02:48 -0700 > Bart Van Assche <bvanassche@acm.org> wrote: > >>>> Do we really want to include the constant "[0]" in the tracing output? >>> This is how it is printed in the source code. >>> From the code flow point of view, there is no need to print this value >>> in trace_block_rq_requeue. >>> Do we need to consider the issue of uniform printing format? If not, I >>> think we can delete it. >> >> I'm not aware of any other tracing statement that prints out a constant. >> Is there perhaps something that I'm missing or overlooking? > > The only time that is done, is if the trace event is used in multiple > places and there's one place that the value will always be the same. Thanks for the clarification Steven. Hence: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 6/14/24 00:49, Dongliang Cui wrote: > Sometimes we need to track the processing order of requests with > ioprio set. So the ioprio of request can be useful information. > > Example： > > block_rq_insert: 8,0 RA 16384 () 6500840 + 32 be,0,6 [binder:815_3] > block_rq_issue: 8,0 RA 16384 () 6500840 + 32 be,0,6 [binder:815_3] > block_rq_complete: 8,0 RA () 6500840 + 32 be,0,6 [0] > > Signed-off-by: Dongliang Cui<dongliang.cui@unisoc.com> Looks useful to me. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
diff --git a/include/trace/events/block.h b/include/trace/events/block.h index 0e128ad51460..1527d5d45e01 100644 --- a/include/trace/events/block.h +++ b/include/trace/events/block.h @@ -9,9 +9,17 @@ #include <linux/blkdev.h> #include <linux/buffer_head.h> #include <linux/tracepoint.h> +#include <uapi/linux/ioprio.h> #define RWBS_LEN 8 +#define IOPRIO_CLASS_STRINGS \ + { IOPRIO_CLASS_NONE, "none" }, \ + { IOPRIO_CLASS_RT, "rt" }, \ + { IOPRIO_CLASS_BE, "be" }, \ + { IOPRIO_CLASS_IDLE, "idle" }, \ + { IOPRIO_CLASS_INVALID, "invalid"} + #ifdef CONFIG_BUFFER_HEAD DECLARE_EVENT_CLASS(block_buffer, @@ -82,6 +90,7 @@ TRACE_EVENT(block_rq_requeue, __field( dev_t, dev ) __field( sector_t, sector ) __field( unsigned int, nr_sector ) + __field( unsigned short, ioprio ) __array( char, rwbs, RWBS_LEN ) __dynamic_array( char, cmd, 1 ) ), @@ -90,16 +99,20 @@ TRACE_EVENT(block_rq_requeue, __entry->dev = rq->q->disk ? disk_devt(rq->q->disk) : 0; __entry->sector = blk_rq_trace_sector(rq); __entry->nr_sector = blk_rq_trace_nr_sectors(rq); + __entry->ioprio = rq->ioprio; blk_fill_rwbs(__entry->rwbs, rq->cmd_flags); __get_str(cmd)[0] = '\0'; ), - TP_printk("%d,%d %s (%s) %llu + %u [%d]", + TP_printk("%d,%d %s (%s) %llu + %u %s,%u,%u [%d]", MAJOR(__entry->dev), MINOR(__entry->dev), __entry->rwbs, __get_str(cmd), - (unsigned long long)__entry->sector, - __entry->nr_sector, 0) + (unsigned long long)__entry->sector, __entry->nr_sector, + __print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio), + IOPRIO_CLASS_STRINGS), + IOPRIO_PRIO_HINT(__entry->ioprio), + IOPRIO_PRIO_LEVEL(__entry->ioprio), 0) ); DECLARE_EVENT_CLASS(block_rq_completion, @@ -113,6 +126,7 @@ DECLARE_EVENT_CLASS(block_rq_completion, __field( sector_t, sector ) __field( unsigned int, nr_sector ) __field( int , error ) + __field( unsigned short, ioprio ) __array( char, rwbs, RWBS_LEN ) __dynamic_array( char, cmd, 1 ) ), @@ -122,16 +136,20 @@ DECLARE_EVENT_CLASS(block_rq_completion, __entry->sector = blk_rq_pos(rq); __entry->nr_sector = nr_bytes >> 9; __entry->error = blk_status_to_errno(error); + __entry->ioprio = rq->ioprio; blk_fill_rwbs(__entry->rwbs, rq->cmd_flags); __get_str(cmd)[0] = '\0'; ), - TP_printk("%d,%d %s (%s) %llu + %u [%d]", + TP_printk("%d,%d %s (%s) %llu + %u %s,%u,%u [%d]", MAJOR(__entry->dev), MINOR(__entry->dev), __entry->rwbs, __get_str(cmd), - (unsigned long long)__entry->sector, - __entry->nr_sector, __entry->error) + (unsigned long long)__entry->sector, __entry->nr_sector, + __print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio), + IOPRIO_CLASS_STRINGS), + IOPRIO_PRIO_HINT(__entry->ioprio), + IOPRIO_PRIO_LEVEL(__entry->ioprio), __entry->error) ); /** @@ -180,6 +198,7 @@ DECLARE_EVENT_CLASS(block_rq, __field( sector_t, sector ) __field( unsigned int, nr_sector ) __field( unsigned int, bytes ) + __field( unsigned short, ioprio ) __array( char, rwbs, RWBS_LEN ) __array( char, comm, TASK_COMM_LEN ) __dynamic_array( char, cmd, 1 ) @@ -190,17 +209,21 @@ DECLARE_EVENT_CLASS(block_rq, __entry->sector = blk_rq_trace_sector(rq); __entry->nr_sector = blk_rq_trace_nr_sectors(rq); __entry->bytes = blk_rq_bytes(rq); + __entry->ioprio = rq->ioprio; blk_fill_rwbs(__entry->rwbs, rq->cmd_flags); __get_str(cmd)[0] = '\0'; memcpy(__entry->comm, current->comm, TASK_COMM_LEN); ), - TP_printk("%d,%d %s %u (%s) %llu + %u [%s]", + TP_printk("%d,%d %s %u (%s) %llu + %u %s,%u,%u [%s]", MAJOR(__entry->dev), MINOR(__entry->dev), __entry->rwbs, __entry->bytes, __get_str(cmd), - (unsigned long long)__entry->sector, - __entry->nr_sector, __entry->comm) + (unsigned long long)__entry->sector, __entry->nr_sector, + __print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio), + IOPRIO_CLASS_STRINGS), + IOPRIO_PRIO_HINT(__entry->ioprio), + IOPRIO_PRIO_LEVEL(__entry->ioprio), __entry->comm) ); /**
Sometimes we need to track the processing order of requests with ioprio set. So the ioprio of request can be useful information. Example： block_rq_insert: 8,0 RA 16384 () 6500840 + 32 be,0,6 [binder:815_3] block_rq_issue: 8,0 RA 16384 () 6500840 + 32 be,0,6 [binder:815_3] block_rq_complete: 8,0 RA () 6500840 + 32 be,0,6 [0] Signed-off-by: Dongliang Cui <dongliang.cui@unisoc.com> --- Changes in v5: - Remove redundant changes. --- --- include/trace/events/block.h | 41 ++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-)