linux-kernel - Re: [PATCH BUGFIX/IMPROVEMENT 6/6] block, bfq: merge bursts of newly-created queues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a4874bb3-006c-85d9-5015-12443baa1e87@kernel.dk>
Date:   Tue, 26 Jan 2021 09:15:27 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Paolo Valente <paolo.valente@...aro.org>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jan Kara <jack@...e.cz>
Subject: Re: [PATCH BUGFIX/IMPROVEMENT 6/6] block, bfq: merge bursts of
 newly-created queues

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