From: "yukuai (C)" <yukuai3@huawei.com> To: Jan Kara <jack@suse.cz> Cc: <paolo.valente@linaro.org>, <axboe@kernel.dk>, <tj@kernel.org>, <linux-block@vger.kernel.org>, <cgroups@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <yi.zhang@huawei.com> Subject: Re: [PATCH -next 7/8] block, bfq: cleanup bfq_bfqq_update_budg_for_activation() Date: Thu, 19 May 2022 21:21:25 +0800 [thread overview] Message-ID: <6de25fae-7d36-c31c-a045-4f1668ef4ee5@huawei.com> (raw) In-Reply-To: <20220519111856.wvk4oetm7odnkg3w@quack3.lan> 在 2022/05/19 19:18, Jan Kara 写道: > On Sat 14-05-22 17:05:21, Yu Kuai wrote: >> It will only be called from bfq_bfqq_handle_idle_busy_switch() in >> specific code branch, there is no need to precaculate >> 'bfqq_wants_to_preempt' each time bfq_bfqq_handle_idle_busy_switch() >> is caleld. >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > Please see below: > >> @@ -1816,14 +1807,6 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, >> (bfqq->bic || RQ_BIC(rq)->stably_merged) && >> (*interactive || soft_rt))); >> >> - /* >> - * Using the last flag, update budget and check whether bfqq >> - * may want to preempt the in-service queue. >> - */ >> - bfqq_wants_to_preempt = >> - bfq_bfqq_update_budg_for_activation(bfqd, bfqq, >> - arrived_in_time); >> - >> /* >> * If bfqq happened to be activated in a burst, but has been >> * idle for much more than an interactive queue, then we > ... >> @@ -1918,7 +1900,7 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, >> * (2) this switch of bfqq to busy changes the scenario. >> */ >> if (bfqd->in_service_queue && >> - ((bfqq_wants_to_preempt && >> + ((bfq_bfqq_update_budg_for_activation(bfqd, bfqq) && >> bfqq->wr_coeff >= bfqd->in_service_queue->wr_coeff) || >> bfq_bfqq_higher_class_or_weight(bfqq, bfqd->in_service_queue) || >> !bfq_better_to_idle(bfqd->in_service_queue)) && > > So these changes are actually wrong because > bfq_bfqq_update_budg_for_activation() relies on > bfq_bfqq_non_blocking_wait_rq() but bfq_add_bfqq_busy() clears that. And > bfq_add_bfqq_busy() is called between the place where > bfq_bfqq_update_budg_for_activation() was called previously and now so your > patch breaks this logic. Hi, You are right, thanks for the explanation, I'll remove this patch and the next patch in next version. Kuai > > Honza >
WARNING: multiple messages have this Message-ID (diff)
From: "yukuai (C)" <yukuai3@huawei.com> To: Jan Kara <jack@suse.cz> Cc: paolo.valente@linaro.org, axboe@kernel.dk, tj@kernel.org, linux-block@vger.kernel.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, yi.zhang@huawei.com Subject: Re: [PATCH -next 7/8] block, bfq: cleanup bfq_bfqq_update_budg_for_activation() Date: Thu, 19 May 2022 21:21:25 +0800 [thread overview] Message-ID: <6de25fae-7d36-c31c-a045-4f1668ef4ee5@huawei.com> (raw) In-Reply-To: <20220519111856.wvk4oetm7odnkg3w@quack3.lan> ÔÚ 2022/05/19 19:18, Jan Kara дµÀ: > On Sat 14-05-22 17:05:21, Yu Kuai wrote: >> It will only be called from bfq_bfqq_handle_idle_busy_switch() in >> specific code branch, there is no need to precaculate >> 'bfqq_wants_to_preempt' each time bfq_bfqq_handle_idle_busy_switch() >> is caleld. >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > Please see below: > >> @@ -1816,14 +1807,6 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, >> (bfqq->bic || RQ_BIC(rq)->stably_merged) && >> (*interactive || soft_rt))); >> >> - /* >> - * Using the last flag, update budget and check whether bfqq >> - * may want to preempt the in-service queue. >> - */ >> - bfqq_wants_to_preempt = >> - bfq_bfqq_update_budg_for_activation(bfqd, bfqq, >> - arrived_in_time); >> - >> /* >> * If bfqq happened to be activated in a burst, but has been >> * idle for much more than an interactive queue, then we > ... >> @@ -1918,7 +1900,7 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, >> * (2) this switch of bfqq to busy changes the scenario. >> */ >> if (bfqd->in_service_queue && >> - ((bfqq_wants_to_preempt && >> + ((bfq_bfqq_update_budg_for_activation(bfqd, bfqq) && >> bfqq->wr_coeff >= bfqd->in_service_queue->wr_coeff) || >> bfq_bfqq_higher_class_or_weight(bfqq, bfqd->in_service_queue) || >> !bfq_better_to_idle(bfqd->in_service_queue)) && > > So these changes are actually wrong because > bfq_bfqq_update_budg_for_activation() relies on > bfq_bfqq_non_blocking_wait_rq() but bfq_add_bfqq_busy() clears that. And > bfq_add_bfqq_busy() is called between the place where > bfq_bfqq_update_budg_for_activation() was called previously and now so your > patch breaks this logic. Hi, You are right, thanks for the explanation, I'll remove this patch and the next patch in next version. Kuai > > Honza >
next prev parent reply other threads:[~2022-05-19 13:21 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-14 9:05 [PATCH -next 0/8] multiple cleanup patches for bfq Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-14 9:05 ` [PATCH -next 1/8] block, bfq: cleanup bfq_weights_tree add/remove apis Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 10:59 ` Jan Kara 2022-05-14 9:05 ` [PATCH -next 2/8] block, bfq: cleanup __bfq_weights_tree_remove() Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:02 ` Jan Kara 2022-05-14 9:05 ` [PATCH -next 3/8] block, bfq: factor out code to update 'active_entities' Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:04 ` Jan Kara 2022-05-19 11:04 ` Jan Kara 2022-05-14 9:05 ` [PATCH -next 4/8] block, bfq: don't declare 'bfqd' as type 'void *' in bfq_group Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:01 ` Jan Kara 2022-05-14 9:05 ` [PATCH -next 5/8] block, bfq: cleanup bfq_activate_requeue_entity() Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:06 ` Jan Kara 2022-05-14 9:05 ` [PATCH -next 6/8] block, bfq: remove dead code for updating 'rq_in_driver' Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:06 ` Jan Kara 2022-05-19 11:06 ` Jan Kara 2022-05-14 9:05 ` [PATCH -next 7/8] block, bfq: cleanup bfq_bfqq_update_budg_for_activation() Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:18 ` Jan Kara 2022-05-19 11:18 ` Jan Kara 2022-05-19 13:21 ` yukuai (C) [this message] 2022-05-19 13:21 ` yukuai (C) 2022-05-14 9:05 ` [PATCH -next 8/8] block, bfq: cleanup bfq_bfqq_handle_idle_busy_switch() Yu Kuai 2022-05-14 9:05 ` Yu Kuai 2022-05-19 11:23 ` Jan Kara 2022-05-19 11:23 ` Jan Kara
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6de25fae-7d36-c31c-a045-4f1668ef4ee5@huawei.com \ --to=yukuai3@huawei.com \ --cc=axboe@kernel.dk \ --cc=cgroups@vger.kernel.org \ --cc=jack@suse.cz \ --cc=linux-block@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=paolo.valente@linaro.org \ --cc=tj@kernel.org \ --cc=yi.zhang@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.