linux-kernel - Re: [PATCH v4] block: Add ioprio to block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86eb3dd0-77a1-4d1d-8e62-38c46bd7563a@acm.org>
Date: Tue, 11 Jun 2024 09:26:54 -0700
From: Bart Van Assche <bvanassche@....org>
To: Dongliang Cui <dongliang.cui@...soc.com>, axboe@...nel.dk,
 rostedt@...dmis.org, mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 ebiggers@...nel.org
Cc: ke.wang@...soc.com, hongyu.jin.cn@...il.com, niuzhiguo84@...il.com,
 hao_hao.wang@...soc.com, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, akailash@...gle.com, cuidongliang390@...il.com
Subject: Re: [PATCH v4] block: Add ioprio to block_rq tracepoint

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.