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