Received-SPF: pass (google.com: domain of linux-kernel+bounces-186522-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <b2e4a072-a339-444e-abea-29388dcbfee6@acm.org>
Date: Wed, 22 May 2024 09:51:16 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] block: Add ioprio to block_rq tracepoint
To: Dongliang Cui <dongliang.cui@unisoc.com>, axboe@kernel.dk,
 rostedt@goodmis.org, mhiramat@kernel.org, mathieu.desnoyers@efficios.com,
 ebiggers@kernel.org
Cc: ke.wang@unisoc.com, hongyu.jin.cn@gmail.com, niuzhiguo84@gmail.com,
 hao_hao.wang@unisoc.com, linux-block@vger.kernel.org,
 linux-kernel@vger.kernel.org, akailash@google.com,
 cuidongliang390@gmail.com, Damien Le Moal <dlemoal@kernel.org>
References: <20240522090104.1751148-1-dongliang.cui@unisoc.com>
Content-Language: en-US
From: Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <20240522090104.1751148-1-dongliang.cui@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/22/24 02:01, 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"}
> +
>   #ifdef CONFIG_BUFFER_HEAD
>   DECLARE_EVENT_CLASS(block_buffer,
>   
> @@ -82,6 +90,8 @@ TRACE_EVENT(block_rq_requeue,
>   		__field(  dev_t,	dev			)
>   		__field(  sector_t,	sector			)
>   		__field(  unsigned int,	nr_sector		)
> +		__field(  unsigned int,	ioprio_class		)
> +		__field(  unsigned int, ioprio_value		)
>   		__array(  char,		rwbs,	RWBS_LEN	)
>   		__dynamic_array( char,	cmd,	1		)
>   	),
> @@ -90,16 +100,19 @@ 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_class = rq->ioprio >> IOPRIO_CLASS_SHIFT & 0x3;
> +		__entry->ioprio_value = rq->ioprio & 0xff;

Why to split the I/O priority field when storing it in __entry instead of when
the values are printed? Combined the ioprio bitfields occupy 16 bits. The above
patch reserves 64 bits in __entry. I think that's overkill. Additionally, some
bits of the I/O priority bits are discarded by the above code before I/O
priority information is reported.

Please split the I/O priority information into the three fields defined in
include/uapi/linux/ioprio.h (class, hint, prio) and use the macros from that
header file for splitting I/O priority information.

Thanks,

Bart.