Re: [PATCH V11 1/8] block, bfq: split sync bfq_queues on a per-actuator basis

[Damien Le Moal]

On 2022/12/21 19:27, Paolo Valente wrote:
> 
> 
>> Il giorno 21 dic 2022, alle ore 01:46, Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> 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