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
