Received-SPF: pass (google.com: domain of linux-kernel+bounces-210243-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <86eb3dd0-77a1-4d1d-8e62-38c46bd7563a@acm.org>
Date: Tue, 11 Jun 2024 09:26:54 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4] 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
References: <20240611073519.323680-1-dongliang.cui@unisoc.com>
Content-Language: en-US
From: Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <20240611073519.323680-1-dongliang.cui@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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.