linux-kernel - Re: [PATCH] block/mq-deadline: Replace DD_PRIO_MAX with DD_PRIO

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] [day] [month] [year] [list]

Message-ID: <CAAWJmAbumBHVo-8neC+b8WqJzzLbWqARpc3vkBsgs9j08wh3kA@mail.gmail.com>
Date: Fri, 29 Aug 2025 14:19:21 +0800
From: Tao pilgrim <pilgrimtao@...il.com>
To: Bart Van Assche <bvanassche@....org>
Cc: axboe@...nel.dk, linux-block@...r.kernel.org, linux-kernel@...r.kernel.org, 
	chengkaitao <chengkaitao@...inos.cn>
Subject: Re: [PATCH] block/mq-deadline: Replace DD_PRIO_MAX with DD_PRIO_COUNT

On Fri, Aug 29, 2025 at 12:38 PM Bart Van Assche <bvanassche@....org> wrote:
>
> On 8/28/25 6:54 PM, chengkaitao wrote:
> > diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> > index b9b7cdf1d3c9..1a031922c447 100644
> > --- a/block/mq-deadline.c
> > +++ b/block/mq-deadline.c
> > @@ -41,19 +41,16 @@ static const int fifo_batch = 16;       /* # of sequential requests treated as o
> >   enum dd_data_dir {
> >       DD_READ         = READ,
> >       DD_WRITE        = WRITE,
> > +     DD_DIR_COUNT    = 2
> >   };
> >
> > -enum { DD_DIR_COUNT = 2 };
> > -
>
> This change is not an improvement in my opinion because it makes it
> less clear what the role of DD_DIR_COUNT is.

DD_DIR_COUNT is used to count the number of members in the
enum dd_data_dir{}, and should be placed inside the enum dd_data_dir.
There are many such examples in the kernel, such as:
KFENCE_COUNTER_COUNT,__MTHP_STAT_COUNT,CHANNELMSG_COUNT,
ETH_RSS_HASH_FUNCS_COUNT,DEPOT_COUNTER_COUNT....
>
> >   enum dd_prio {
> > -     DD_RT_PRIO      = 0,
> > -     DD_BE_PRIO      = 1,
> > -     DD_IDLE_PRIO    = 2,
> > -     DD_PRIO_MAX     = 2,
> > +     DD_RT_PRIO,
> > +     DD_BE_PRIO,
> > +     DD_IDLE_PRIO,
>
> There is code that depends on DD_RT_PRIO < DD_BE_PRIO < DD_IDLE_PRIO so
> I'd like to keep the explicit enum values.

Wouldn't it be better to simply add a comment for this purpose?
>
> > +     DD_PRIO_COUNT
> >   };
> >
> > -enum { DD_PRIO_COUNT = 3 };
>
> I see the above change as a step backwards because it makes the role of
> DD_PRIO_COUNT less clear.

Defining DD_PRIO_COUNT within the enum dd_prio{} clearly indicates
that DD_PRIO_COUNT serves solely for the enum dd_prio{}. If a new
member is added to enum dd_prio{} in the future, DD_PRIO_COUNT
would not need to be modified separately.
>
> > -     for (prio = DD_BE_PRIO; prio <= DD_PRIO_MAX; prio++) {
> > +     for (prio = DD_BE_PRIO; prio < DD_PRIO_COUNT; prio++) {
>
> The current code is easier to read IMHO than the new code.

I believe programmers are intelligent enough to understand the meanings
of *_COUNT and *_MAX without needing to define them independently,
especially in cases like *_COUNT = *_MAX + 1, where _MAX seems rather
redundant.
>
The above are my personal thoughts. The purpose of this patch is to make
the code more concise, and not merging it into the mainline won't have any
significant impact.

Looking forward to your reply.
-- 
Yours,
Kaitao Cheng