diff mbox series

[v5] block: Add ioprio to block_rq tracepoint

Message ID 20240614074936.113659-1-dongliang.cui@unisoc.com (mailing list archive)
State New, archived
Headers show
Series [v5] block: Add ioprio to block_rq tracepoint | expand

Commit Message

Dongliang Cui June 14, 2024, 7:49 a.m. UTC
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(-)

Comments

Bart Van Assche June 14, 2024, 4:40 p.m. UTC | #1
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.
dongliang cui June 17, 2024, 7:59 a.m. UTC | #2
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.
>
Bart Van Assche June 17, 2024, 5:02 p.m. UTC | #3
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.
Steven Rostedt June 17, 2024, 5:07 p.m. UTC | #4
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
Bart Van Assche June 17, 2024, 9:30 p.m. UTC | #5
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>
Chaitanya Kulkarni June 18, 2024, 12:28 a.m. UTC | #6
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
dongliang cui June 28, 2024, 6:38 a.m. UTC | #7
On Tue, Jun 18, 2024 at 8:29 AM Chaitanya Kulkarni
<chaitanyak@nvidia.com> wrote:
>
> 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
>
>
kindly ping...
Jens Axboe June 28, 2024, 4:36 p.m. UTC | #8
On Fri, 14 Jun 2024 15:49:36 +0800, 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]
> 
> [...]

Applied, thanks!

[1/1] block: Add ioprio to block_rq tracepoint
      commit: aa6ff4eb7c10d9a6532db3ea9e78124bf14e70ae

Best regards,
diff mbox series

Patch

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)
 );
 
 /**