> Il giorno 26 gen 2021, alle ore 17:15, Jens Axboe <axboe@xxxxxxxxx> ha scritto:
>
> On 1/26/21 3:51 AM, Paolo Valente wrote:
>> @@ -2809,6 +2853,12 @@ void bfq_release_process_ref(struct bfq_data *bfqd, struct bfq_queue *bfqq)
>> bfqq != bfqd->in_service_queue)
>> bfq_del_bfqq_busy(bfqd, bfqq, false);
>>
>> + if (bfqq->entity.parent &&
>> + bfqq->entity.parent->last_bfqq_created == bfqq)
>> + bfqq->entity.parent->last_bfqq_created = NULL;
>> + else if (bfqq->bfqd && bfqq->bfqd->last_bfqq_created == bfqq)
>> + bfqq->bfqd->last_bfqq_created = NULL;
>> +
>> bfq_put_queue(bfqq);
>> }
>>
>> @@ -2905,6 +2955,13 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic,
>> */
>> new_bfqq->pid = -1;
>> bfqq->bic = NULL;
>> +
>> + if (bfqq->entity.parent &&
>> + bfqq->entity.parent->last_bfqq_created == bfqq)
>> + bfqq->entity.parent->last_bfqq_created = new_bfqq;
>> + else if (bfqq->bfqd && bfqq->bfqd->last_bfqq_created == bfqq)
>> + bfqq->bfqd->last_bfqq_created = new_bfqq;
>> +
>> bfq_release_process_ref(bfqd, bfqq);
>> }
>
> Almost identical code constructs makes it seem like this should have a
> helper instead.
>
Right, sorry. Improved in V2.
>> @@ -5033,6 +5090,12 @@ void bfq_put_queue(struct bfq_queue *bfqq)
>> bfqg_and_blkg_put(bfqg);
>> }
>>
>> +static void bfq_put_stable_ref(struct bfq_queue *bfqq)
>> +{
>> + bfqq->stable_ref--;
>> + bfq_put_queue(bfqq);
>> +}
>> +
>> static void bfq_put_cooperator(struct bfq_queue *bfqq)
>> {
>> struct bfq_queue *__bfqq, *next;
>> @@ -5089,6 +5152,17 @@ static void bfq_exit_icq(struct io_cq *icq)
>> {
>> struct bfq_io_cq *bic = icq_to_bic(icq);
>>
>> + if (bic->stable_merge_bfqq) {
>> + unsigned long flags;
>> + struct bfq_data *bfqd = bic->stable_merge_bfqq->bfqd;
>> +
>> + if (bfqd)
>> + spin_lock_irqsave(&bfqd->lock, flags);
>> + bfq_put_stable_ref(bic->stable_merge_bfqq);
>> + if (bfqd)
>> + spin_unlock_irqrestore(&bfqd->lock, flags);
>> + }
>> +
>
> Construct like this are really painful. Just do:
>
> if (bfqd) {
> unsigned long flags;
>
> spin_lock_irqsave(&bfqd->lock, flags);
> bfq_put_stable_ref(bic->stable_merge_bfqq);
> spin_unlock_irqrestore(&bfqd->lock, flags);
> } else {
> bfq_put_stable_ref(bic->stable_merge_bfqq);
> }
>
> which is also less likely to cause code analyzer false warnings.
Done, thanks.
> Outside
> of that, it needs a comment on why it's ok NOT to grab the lock when
> bfqd is zero, because that seems counter-intuitive and more a case of
> "well we can't grab a lock for something we don't have". Maybe it's
> because bfqd is no longer visible at this point, and it's ok,
yes
> but it's
> definitely not clear just looking at this patch.
Right, the reason is already reported a few lines above, but not
repeated in this function. I'll repeat it.
> Even with that, is the
> bfqq visible? Should the ref be atomic, and locking happen further down
> instead?
>
Since the scheduler is gone, no pending I/O is expected to still
reference bfqq. I'll write this too in V2.
As I stated in my reply to another comments of yours, I'll submit the
V2 soon, unless I receive a reply before.
Thanks.
Paolo
> --
> Jens Axboe
>
