Re: [PATCH BUGFIX/IMPROVEMENT 6/6] block, bfq: merge bursts of newly-created queues
From: Paolo Valente
Date: Thu Feb 25 2021 - 12:25:45 EST
>
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
>