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

[Paolo Valente]

> 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
>