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]
Date: Thu, 18 Jan 2024 08:25:03 -0700
From: Jens Axboe <axboe@...nel.dk>
To: Zhaoyang Huang <huangzhaoyang@...il.com>
Cc: "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
 steve.kang@...soc.com
Subject: Re: [RFC PATCH 1/1] block: introduce activity based ioprio

On 1/18/24 12:48 AM, Zhaoyang Huang wrote:
>>> +static enum dd_prio dd_req_ioprio(struct request *rq)
>>> +{
>>> +     enum dd_prio prio;
>>> +     const u8 ioprio_class = dd_rq_ioclass(rq);
>>> +#ifdef CONFIG_ACTIVITY_BASED_IOPRIO
>>> +     struct bio *bio;
>>> +     struct bio_vec bv;
>>> +     struct bvec_iter iter;
>>> +     struct page *page;
>>> +     int gen = 0;
>>> +     int cnt = 0;
>>> +
>>> +     if (req_op(rq) == REQ_OP_READ) {
>>> +             __rq_for_each_bio(bio, rq) {
>>> +                     bio_for_each_bvec(bv, bio, iter) {
>>> +                             page = bv.bv_page;
>>> +                             gen += PageWorkingset(page) ? 1 : 0;
>>> +                             cnt++;
>>> +                     }
>>> +             }
>>> +             prio = (gen >= cnt / 2) ? ioprio_class_to_prio[IOPRIO_CLASS_RT] :
>>> +                     ioprio_class_to_prio[ioprio_class];
>>> +     } else
>>> +             prio = ioprio_class_to_prio[ioprio_class];
>>> +#else
>>> +     prio = ioprio_class_to_prio[ioprio_class];
>>> +#endif
>>> +     return prio;
>>> +}
>>
>> This is pretty awful imho, you're iterating the pages which isn't
>> exactly cheap. There's also a ternary operator (get rid of it), and
>> magic cnt / 2 which isn't even explained.

> ok. The iterating is on purpose here to not enlarge the bio and
> request structure. The magic number would be replaced by an more
> sensible criteria like MULTI_GEN's thrashing tier things.

It's totally backwards, you're adding code that both dips into parts it
very much should not, and attempting to fix things up after the fact.
Again, not passing any judgement on whether this kind of thing makes
sense, but if it did, you'd do it when adding the page to the bio, as I
mentioned elsewhere in the email. There's no need to grow struct bio, it
already has an ioprio field.

-- 
Jens Axboe


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ