Messages in this thread | | | Date | Wed, 21 Dec 2022 09:50:37 +0900 | Subject | Re: [PATCH V10 1/8] block, bfq: split sync bfq_queues on a per-actuator basis | From | Damien Le Moal <> |
| |
On 2022/12/20 22:10, Paolo Valente wrote: >>> - /* >>> - * Does queue (or any parent entity) exceed number of requests that >>> - * should be available to it? Heavily limit depth so that it cannot >>> - * consume more available requests and thus starve other entities. >>> - */ >>> - if (bfqq && bfqq_request_over_limit(bfqq, limit)) >>> - depth = 1; >>> + for (act_idx = 0; act_idx < bfqd->num_actuators; act_idx++) { >>> + struct bfq_queue *bfqq = >>> + bic ? bic_to_bfqq(bic, op_is_sync(opf), act_idx) : NULL; >> >> Commented already: why not add a "if (!bfqq) return NULL;" in >> bic_to_bfqq() ? > > You have probably missed my reply on this. The problem is that your > proposal would improve code (only) here, but it would entail the above > control for all the other invocations, for which it is useless :(
But then you have *a lot* of "if (bfqd)" tests that are useless elsewhere since bic_to_bfqq() never returns NULL.
And for this line, I personally would prefer seeing something like:
struct bfq_queue *bfqq;
if (bic) bfqd = bic_to_bfqq(bic, op_is_sync(opf), act_idx) else bfqd = NULL;
Which is a lot simpler to read.
-- Damien Le Moal Western Digital Research
| |