Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: 5+Kr5QDbZOVzqeCCdUny31M46PcybKp3C3/0LrA5b1axvis4IOK5EfgF4B2dQHt9AiZUMEDO53
 Xf+EoTlbSAZHFawHK+eIUxioUaTOSdvVV0ZPZtUm3OSyCvBzRYUZrYnTxjnxiSHwgNRvatvyx3
 9uRyql1bAWSuZB3lyaTRb1gz8e/0DTFyHLHf8Jrf5ckX2QHdFZbrAR2l+lG5PI7nlL6awp/oSz
 A/9ELoAWEFYhHat4zc+avw4nHfw9hlV6L51yrGYqZqngUUwjTHjerKo3hp0YfN2VjD9HT8PgFj
 kTI=
IronPort-SDR: vX2ChglINhH0mecxs3MYad1L0xrfHKrgL1MM7Z+cpMKuqbuIgFHaj+gUeOwTZuAKiXg1za54Lq
 Ft3OihGslgZpMN6L800my0Of2CN+TSf1OaDgvOFZwC7HJ459x3MNddEmoFufta7y9LfjIrnDAd
 BOagjzcDNFpmABkVsasmkTCBfEHMqD8hMmpfmlHvVRGJDPaYZ2olCkksAVFKIF9Ztg4qDhtuPm
 YhuG3PSvQea4k6KI+LbHnEJlaTqZ1ST++dvdZgm+pC4ZARvUEMHxdaET7A+97PMFnBSePjNwIr
 b44=
WDCIronportException: Internal
Message-ID: <017f3dbd-4586-490a-2f21-948586391515@opensource.wdc.com>
Date:   Wed, 21 Dec 2022 21:27:39 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH V11 1/8] block, bfq: split sync bfq_queues on a
 per-actuator basis
Content-Language: en-US
To:     Paolo Valente <paolo.valente@linaro.org>
Cc:     Jens Axboe <axboe@kernel.dk>,
        linux-block <linux-block@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Arie van der Hoeven <arie.vanderhoeven@seagate.com>,
        Rory Chen <rory.c.chen@seagate.com>,
        Glen Valante <glen.valante@linaro.org>,
        Gabriele Felici <felicigb@gmail.com>,
        Carmine Zaccagnino <carmine@carminezacc.com>
References: <20221220095013.55803-1-paolo.valente@linaro.org>
 <20221220095013.55803-2-paolo.valente@linaro.org>
 <8a49432d-642f-cd58-8e4d-2b320aef5edd@opensource.wdc.com>
 <D429FA01-9510-41FF-AD85-F74EB8AD171F@linaro.org>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <D429FA01-9510-41FF-AD85-F74EB8AD171F@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/12/21 19:27, Paolo Valente wrote:
> 
> 
>> Il giorno 21 dic 2022, alle ore 01:46, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto:
>>
>>
>> [...]
>>
>>> -static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync)
>>> +static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync,
>>> +			      unsigned int actuator_idx)
>>> {
>>> -	struct bfq_queue *bfqq = bic_to_bfqq(bic, is_sync);
>>> +	struct bfq_queue *bfqq = bic_to_bfqq(bic, is_sync, actuator_idx);
>>> 	struct bfq_data *bfqd;
>>>
>>> 	if (bfqq)
>>
>> With your current bic_to_bfqq() implementation, you will *never* get NULL as a
>> return value.
> 
> I'm afraid this is not true.  A bic is associated with a sync and an
> async queue, or with both.  So, in the hunk above, bic_to_bfqq returns
> NULL if:
> - either the bic is associated with a sync queue, but is_sync happens to be false;
> - or the bic is associate with an async queue, but is_sync happens to be true.
> 
> Of course, with these patches, the associations move from "with a
> sync/async queue" to "with a set of sync/async queues, one per
> actuator".

My bad... The bic->bfqq[][actuator_idx] is an array of pointers... I was reading
it as "&bic->bfqq[1][actuator_idx]". So please ignore. Apologies for the noise.

> 
>> So why is this if necessary ?
>>> 		bfqd = bfqq->bfqd; /* NULL if scheduler already exited */
>>>
>>> 	if (bfqq && bfqd) {
>>> -		unsigned long flags;
>>> -
>>> -		spin_lock_irqsave(&bfqd->lock, flags);
>>> 		bfqq->bic = NULL;
>>> 		bfq_exit_bfqq(bfqd, bfqq);
>>> -		bic_set_bfqq(bic, NULL, is_sync);
>>> -		spin_unlock_irqrestore(&bfqd->lock, flags);
>>> +		bic_set_bfqq(bic, NULL, is_sync, actuator_idx);
>>> 	}
>>> }
>>>
>>> static void bfq_exit_icq(struct io_cq *icq)
>>> {
>>> 	struct bfq_io_cq *bic = icq_to_bic(icq);
>>> +	struct bfq_data *bfqd = bic_to_bfqd(bic);
>>> +	unsigned long flags;
>>> +	unsigned int act_idx;
>>> +	/*
>>> +	 * If bfqd and thus bfqd->num_actuators is not available any
>>> +	 * longer, then cycle over all possible per-actuator bfqqs in
>>> +	 * next loop. We rely on bic being zeroed on creation, and
>>> +	 * therefore on its unused per-actuator fields being NULL.
>>> +	 */
>>> +	unsigned int num_actuators = BFQ_MAX_ACTUATORS;
>>>
>>> -	if (bic->stable_merge_bfqq) {
>>> -		struct bfq_data *bfqd = bic->stable_merge_bfqq->bfqd;
>>> +	/*
>>> +	 * bfqd is NULL if scheduler already exited, and in that case
>>> +	 * this is the last time these queues are accessed.
>>> +	 */
>>> +	if (bfqd) {
>>
>> Same here. bfqd can never be NULL. Or I am really missing something... Lots of
>> other places like this where checking bic_to_bfqd() seems unnecessary.
> 
> As written in the comment above, bfqd is NULL if the scheduler already
> exited.  That is, bic->icq.q->elevator->elevator_data == NULL.  This
> is an event I have checked several years ago, probably while porting
> cfq to bfq.  If boundary conditions changed later, and nobody realized
> that this was not true any longer, then bfqd would never be NULL as
> you say.  At any rate, I guess that such a change would then belong to
> a separate patch series.
> 
> Thanks,
> Paolo

-- 
Damien Le Moal
Western Digital Research