linux-kernel - Re: [PATCH V6 4/8] block, bfq: turn bfqq_data into an array in bfq_io

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:   Tue, 6 Dec 2022 09:00:22 +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>,
        Gabriele Felici <felicigb@...il.com>,
        Gianmarco Lusvardi <glusvardi@...teo.net>,
        Giulio Barabino <giuliobarabino99@...il.com>,
        Emiliano Maccaferri <inbox@...lianomaccaferri.com>
Subject: Re: [PATCH V6 4/8] block, bfq: turn bfqq_data into an array in
 bfq_io_cq



> Il giorno 21 nov 2022, alle ore 01:39, Damien Le Moal <damien.lemoal@...nsource.wdc.com> ha scritto:
...
>> 
>> 			bfqq = bfq_split_bfqq(bic, bfqq);
>> 			split = true;
>> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
>> index f2e8ab91951c..e27897d66a0f 100644
>> --- a/block/bfq-iosched.h
>> +++ b/block/bfq-iosched.h
>> @@ -416,7 +416,7 @@ struct bfq_queue {
>> struct bfq_iocq_bfqq_data {
>> 	/*
>> 	 * Snapshot of the has_short_time flag before merging; taken
>> -	 * to remember its value while the queue is merged, so as to
>> +	 * to remember its values while the queue is merged, so as to
>> 	 * be able to restore it in case of split.
>> 	 */
>> 	bool saved_has_short_ttime;
>> @@ -430,7 +430,7 @@ struct bfq_iocq_bfqq_data {
>> 	u64 saved_tot_idle_time;
>> 
>> 	/*
>> -	 * Same purpose as the previous fields for the value of the
>> +	 * Same purpose as the previous fields for the values of the
>> 	 * field keeping the queue's belonging to a large burst
>> 	 */
>> 	bool saved_in_large_burst;
>> @@ -493,8 +493,12 @@ struct bfq_io_cq {
>> 	uint64_t blkcg_serial_nr; /* the current blkcg serial */
>> #endif
>> 
>> -	/* persistent data for associated synchronous process queue */
>> -	struct bfq_iocq_bfqq_data bfqq_data;
>> +	/*
>> +	 * Persistent data for associated synchronous process queues
>> +	 * (one queue per actuator, see field bfqq above). In
>> +	 * particular, each of these queues may undergo a merge.
>> +	 */
>> +	struct bfq_iocq_bfqq_data bfqq_data[BFQ_MAX_ACTUATORS];
> 
> I wonder if packing this together with struct bfq_queue would be cleaner.
> That would avoid the 2 arrays you have in this struct. Something like this:
> 
> struct bfq_queue_data {
> 	struct bfq_queue 	*bfqq[2];
> 	struct bfq_iocq_bfqq_data iocq_data;
> }
> 
> struct bfq_io_cq {
> 	...
> 	struct bfq_queue_data bfqqd[BFQ_MAX_ACTUATORS];
> 	...
> }
> 
> Thinking aloud here. That may actually make the code more complicated.

I see your point, but, yes, this change would entail one more
indirection when accessing queues from io contexts.

Apart from this, I have applied all of your other suggestions here.

Thanks,
Paolo

> 
>> 
>> 	unsigned int requests;	/* Number of requests this process has in flight */
>> };
> 
> -- 
> Damien Le Moal
> Western Digital Research