linux-kernel - Re: [PATCH 3/3] block: introducing a bias over deadline's fifo

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: <da41bac4-015f-4df0-b8cc-6c8b2f468e12@kernel.dk>
Date: Thu, 8 Feb 2024 17:10:58 -0700
From: Jens Axboe <axboe@...nel.dk>
To: Zhaoyang Huang <huangzhaoyang@...il.com>
Cc: "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Juri Lelli <juri.lelli@...hat.com>,
 Vincent Guittot <vincent.guittot@...aro.org>, Yu Zhao <yuzhao@...gle.com>,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
 steve.kang@...soc.com
Subject: Re: [PATCH 3/3] block: introducing a bias over deadline's fifo_time

On 2/8/24 5:02 PM, Zhaoyang Huang wrote:
> On Fri, Feb 9, 2024 at 1:49?AM Jens Axboe <axboe@...nel.dk> wrote:
>>
>> On 2/8/24 2:31 AM, zhaoyang.huang wrote:
>>> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
>>> index f958e79277b8..43c08c3d6f18 100644
>>> --- a/block/mq-deadline.c
>>> +++ b/block/mq-deadline.c
>>> @@ -15,6 +15,7 @@
>>>  #include <linux/compiler.h>
>>>  #include <linux/rbtree.h>
>>>  #include <linux/sbitmap.h>
>>> +#include "../kernel/sched/sched.h"
>>>
>>>  #include <trace/events/block.h>
>>>
>>> @@ -802,6 +803,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>>       u8 ioprio_class = IOPRIO_PRIO_CLASS(ioprio);
>>>       struct dd_per_prio *per_prio;
>>>       enum dd_prio prio;
>>> +     int fifo_expire;
>>>
>>>       lockdep_assert_held(&dd->lock);
>>>
>>> @@ -840,7 +842,9 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>>               /*
>>>                * set expire time and add to fifo list
>>>                */
>>> -             rq->fifo_time = jiffies + dd->fifo_expire[data_dir];
>>> +             fifo_expire = task_is_realtime(current) ? dd->fifo_expire[data_dir] :
>>> +                     CFS_PROPORTION(current, dd->fifo_expire[data_dir]);
>>> +             rq->fifo_time = jiffies + fifo_expire;
>>>               insert_before = &per_prio->fifo_list[data_dir];
>>>  #ifdef CONFIG_BLK_DEV_ZONED
>>>               /*
>>
>> Hard pass on this blatant layering violation. Just like the priority
>> changes, this utterly fails to understand how things are properly
>> designed.
> IMHO, I don't think this is a layering violation. bio_set_ioprio is
> the one which introduces the scheduler thing into the block layer,
> this commit just wants to do a little improvement based on that. This
> commit helps CFS task save some IO time when preempted by RT heavily.

Listen, both this and the previous content ioprio thing show a glaring
misunderstanding of how to design these kinds of things. You have no
grasp of what the different layers do, or how they interact. I'm not
sure how to put this kindly, but it's really an awful idea to hardcore
some CFS helper into the IO scheduler. The fact that you had to fiddle
around with headers to make it work was the first warning sign, and the
fact that you didn't stop at that point to consider how it could be
properly done makes it even worse.

You need to stop sending kernel patches until you understand basic
software design. Neither of these patches are going anywhere until this
happens. There's been plenty of feedback to telling you that, but you
seem to just ignore it and plow on ahead. Stop.

-- 
Jens Axboe