| Message ID | 20240611073519.323680-1-dongliang.cui@unisoc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] block: Add ioprio to block_rq tracepoint | expand |
On 6/11/24 12:35 AM, Dongliang Cui wrote: > +#define IOPRIO_CLASS_STRINGS \ > + { IOPRIO_CLASS_NONE, "none" }, \ > + { IOPRIO_CLASS_RT, "rt" }, \ > + { IOPRIO_CLASS_BE, "be" }, \ > + { IOPRIO_CLASS_IDLE, "idle" }, \ > + { IOPRIO_CLASS_INVALID, "invalid"} Shouldn't this array be defined in a C file instead of in a header file? > @@ -79,27 +87,32 @@ TRACE_EVENT(block_rq_requeue, > TP_ARGS(rq), > > TP_STRUCT__entry( > - __field( dev_t, dev ) > - __field( sector_t, sector ) > - __field( unsigned int, nr_sector ) > - __array( char, rwbs, RWBS_LEN ) > - __dynamic_array( char, cmd, 1 ) > + __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 ) > ), I see unnecessary whitespace changes. These changes make this patch harder to read than necessary. Please undo the whitespace changes. > DECLARE_EVENT_CLASS(block_rq_completion, > @@ -109,12 +122,13 @@ DECLARE_EVENT_CLASS(block_rq_completion, > TP_ARGS(rq, error, nr_bytes), > > TP_STRUCT__entry( > - __field( dev_t, dev ) > - __field( sector_t, sector ) > - __field( unsigned int, nr_sector ) > - __field( int , error ) > - __array( char, rwbs, RWBS_LEN ) > - __dynamic_array( char, cmd, 1 ) > + __field( dev_t, dev ) > + __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 ) > ), Also here, please do not reformat lines that are not modified otherwise. > @@ -176,13 +194,14 @@ DECLARE_EVENT_CLASS(block_rq, > TP_ARGS(rq), > > TP_STRUCT__entry( > - __field( dev_t, dev ) > - __field( sector_t, sector ) > - __field( unsigned int, nr_sector ) > - __field( unsigned int, bytes ) > - __array( char, rwbs, RWBS_LEN ) > - __array( char, comm, TASK_COMM_LEN ) > - __dynamic_array( char, cmd, 1 ) > + __field( dev_t, dev ) > + __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 ) > ), Same comment here. Thanks, Bart.
On Tue, 11 Jun 2024 09:26:54 -0700 Bart Van Assche <bvanassche@acm.org> wrote: > On 6/11/24 12:35 AM, Dongliang Cui wrote: > > +#define IOPRIO_CLASS_STRINGS \ > > + { IOPRIO_CLASS_NONE, "none" }, \ > > + { IOPRIO_CLASS_RT, "rt" }, \ > > + { IOPRIO_CLASS_BE, "be" }, \ > > + { IOPRIO_CLASS_IDLE, "idle" }, \ > > + { IOPRIO_CLASS_INVALID, "invalid"} > > Shouldn't this array be defined in a C file instead of in a header file? The way the TRACE_EVENT() macro works, this will not work in a C file. > - 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) > ); > It's used for __print_symbolic() which the TRACE_EVENT() macro logic (using header files) will expand it to something useful. -- Steve
On 6/11/24 9:54 AM, Steven Rostedt wrote: > On Tue, 11 Jun 2024 09:26:54 -0700 > Bart Van Assche <bvanassche@acm.org> wrote: > >> On 6/11/24 12:35 AM, Dongliang Cui wrote: >>> +#define IOPRIO_CLASS_STRINGS \ >>> + { IOPRIO_CLASS_NONE, "none" }, \ >>> + { IOPRIO_CLASS_RT, "rt" }, \ >>> + { IOPRIO_CLASS_BE, "be" }, \ >>> + { IOPRIO_CLASS_IDLE, "idle" }, \ >>> + { IOPRIO_CLASS_INVALID, "invalid"} >> >> Shouldn't this array be defined in a C file instead of in a header file? > > The way the TRACE_EVENT() macro works, this will not work in a C file. Hmm ... if the above array is terminated with a { -1, NULL } sentinel and if __print_symbolic() is changed into trace_print_symbols_seq(p, ...) then the above array can be moved into a C file, isn't it? Thanks, Bart.
On Tue, 11 Jun 2024 10:09:12 -0700 Bart Van Assche <bvanassche@acm.org> wrote: > On 6/11/24 9:54 AM, Steven Rostedt wrote: > > On Tue, 11 Jun 2024 09:26:54 -0700 > > Bart Van Assche <bvanassche@acm.org> wrote: > > > >> On 6/11/24 12:35 AM, Dongliang Cui wrote: > >>> +#define IOPRIO_CLASS_STRINGS \ > >>> + { IOPRIO_CLASS_NONE, "none" }, \ > >>> + { IOPRIO_CLASS_RT, "rt" }, \ > >>> + { IOPRIO_CLASS_BE, "be" }, \ > >>> + { IOPRIO_CLASS_IDLE, "idle" }, \ > >>> + { IOPRIO_CLASS_INVALID, "invalid"} > >> > >> Shouldn't this array be defined in a C file instead of in a header file? > > > > The way the TRACE_EVENT() macro works, this will not work in a C file. > > Hmm ... if the above array is terminated with a { -1, NULL } sentinel and if > __print_symbolic() is changed into trace_print_symbols_seq(p, ...) then the above > array can be moved into a C file, isn't it? > Then it breaks user space parsing. The reason for __print_symbolic() is that libtraceevent knows how to parse it. If you put the array into a C file, the above mappings will not show up in the tracefs format file for the event, and you'll just get "[FAILED TO PARSE]" output from the user space tracing tooling. -- Steve
On Tue, 11 Jun 2024 13:17:37 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > > Hmm ... if the above array is terminated with a { -1, NULL } sentinel and if > > __print_symbolic() is changed into trace_print_symbols_seq(p, ...) then the above > > array can be moved into a C file, isn't it? > > > > Then it breaks user space parsing. The reason for __print_symbolic() is > that libtraceevent knows how to parse it. If you put the array into a C > file, the above mappings will not show up in the tracefs format file for > the event, and you'll just get "[FAILED TO PARSE]" output from the user > space tracing tooling. Note, the trace headers are not normal headers. They are included multiple times (when TRACE_HEADER_MULTI_READ is defined). Only one C file will include this header with CREATE_TRACE_POINTS defined and these headers will then build global C functions and variables. So technically, this "array" is in C file and not in a header, as it will not be created unless a C file includes it with CREATE_TRACE_POINTS, and only one C file may do that (otherwise the kernel will fail to build). -- Steve
diff --git a/include/trace/events/block.h b/include/trace/events/block.h index 0e128ad51460..209d54dc9dce 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, @@ -79,27 +87,32 @@ TRACE_EVENT(block_rq_requeue, TP_ARGS(rq), TP_STRUCT__entry( - __field( dev_t, dev ) - __field( sector_t, sector ) - __field( unsigned int, nr_sector ) - __array( char, rwbs, RWBS_LEN ) - __dynamic_array( char, cmd, 1 ) + __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 ) ), TP_fast_assign( __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, @@ -109,12 +122,13 @@ DECLARE_EVENT_CLASS(block_rq_completion, TP_ARGS(rq, error, nr_bytes), TP_STRUCT__entry( - __field( dev_t, dev ) - __field( sector_t, sector ) - __field( unsigned int, nr_sector ) - __field( int , error ) - __array( char, rwbs, RWBS_LEN ) - __dynamic_array( char, cmd, 1 ) + __field( dev_t, dev ) + __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 ) ), TP_fast_assign( @@ -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) ); /** @@ -176,13 +194,14 @@ DECLARE_EVENT_CLASS(block_rq, TP_ARGS(rq), TP_STRUCT__entry( - __field( dev_t, dev ) - __field( sector_t, sector ) - __field( unsigned int, nr_sector ) - __field( unsigned int, bytes ) - __array( char, rwbs, RWBS_LEN ) - __array( char, comm, TASK_COMM_LEN ) - __dynamic_array( char, cmd, 1 ) + __field( dev_t, dev ) + __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 ) ), TP_fast_assign( @@ -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 v4: - Use macros to split ioprio. - Print ioprio hint. - Only storage ioprio in __entry. --- --- include/trace/events/block.h | 77 +++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 27 deletions(-)