linux-kernel - Re: [PATCH V10 1/8] block, bfq: split sync bfq

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]

Date:   Wed, 21 Dec 2022 11:13:07 +0100
From:   Paolo Valente <paolo.valente@...aro.org>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        linux-block <linux-block@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Arie van der Hoeven <arie.vanderhoeven@...gate.com>,
        Rory Chen <rory.c.chen@...gate.com>,
        Glen Valante <glen.valante@...aro.org>,
        Gabriele Felici <felicigb@...il.com>,
        Carmine Zaccagnino <carmine@...minezacc.com>
Subject: Re: [PATCH V10 1/8] block, bfq: split sync bfq_queues on a
 per-actuator basis



> Il giorno 21 dic 2022, alle ore 01:50, Damien Le Moal <damien.lemoal@...nsource.wdc.com> ha scritto:
> 
> 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.
> 

I'm probably misunderstanding your point, sorry.  Could you point me
to one of the places where there is the useless control that would go
away if we add your proposed control inside bic_to_bfqq?  (of course
apart form the above one, which seems to be the only one to me)

> 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.
> 
> 

Ok, as I have your blessing on this point, I'll send a V12 with also
this change.

Thanks,
Paolo

> -- 
> Damien Le Moal
> Western Digital Research
>