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.
> @@ -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. 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, but it's
definitely not clear just looking at this patch. Even with that, is the
bfqq visible? Should the ref be atomic, and locking happen further down
instead?
--
Jens Axboe
> Il giorno 26 gen 2021, alle ore 17:15, Jens Axboe <[email protected]> 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
>