From: Jan Kara <jack@suse.cz> To: Yu Kuai <yukuai3@huawei.com> Cc: jack@suse.cz, 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 8/8] block, bfq: cleanup bfq_bfqq_handle_idle_busy_switch() Date: Thu, 19 May 2022 13:23:09 +0200 [thread overview] Message-ID: <20220519112309.nhbulnc5iz424ohl@quack3.lan> (raw) In-Reply-To: <20220514090522.1669270-9-yukuai3@huawei.com> On Sat 14-05-22 17:05:22, Yu Kuai wrote: > 'wr_or_deserves_wr' is only used in bfq_update_bfqq_wr_on_rq_arrival(), > which is only called from bfq_bfqq_handle_idle_busy_switch() in specific > code branch, thus there is no need to precaculate 'wr_or_deserves_wr' > each time bfq_bfqq_handle_idle_busy_switch() is called. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> With this patch there's the same problem as with the previous one. Some of the variables passed to bfq_update_bfqq_wr_on_rq_arrival() (in_burst, soft_rt) would actually evaluate differently later in the function. Honza > --- > block/bfq-iosched.c | 110 +++++++++++++++++++++++++------------------- > 1 file changed, 62 insertions(+), 48 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 1e57d76c8dd3..cea8cb3f5ee2 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -1624,15 +1624,65 @@ static unsigned long bfq_smallest_from_now(void) > return jiffies - MAX_JIFFY_OFFSET; > } > > +/* > + * bfqq deserves to be weight-raised if: > + * - it is sync, > + * - it does not belong to a large burst, > + * - it has been idle for enough time or is soft real-time, > + * - is linked to a bfq_io_cq (it is not shared in any sense), > + * - has a default weight (otherwise we assume the user wanted > + * to control its weight explicitly) > + * > + * Merged bfq_queues are kept out of weight-raising > + * (low-latency) mechanisms. The reason is that these queues > + * are usually created for non-interactive and > + * non-soft-real-time tasks. Yet this is not the case for > + * stably-merged queues. These queues are merged just because > + * they are created shortly after each other. So they may > + * easily serve the I/O of an interactive or soft-real time > + * application, if the application happens to spawn multiple > + * processes. So let also stably-merged queued enjoy weight > + * raising. > + */ > +static bool bfqq_wr_or_deserves_wr(struct bfq_data *bfqd, > + struct bfq_queue *bfqq, > + struct request *rq, > + bool interactive, bool soft_rt) > +{ > + if (!bfqd->low_latency) > + return false; > + > + if (bfqq->wr_coeff > 1) > + return true; > + > + if (!bfq_bfqq_sync(bfqq)) > + return false; > + > + if (!bfqq->bic && !RQ_BIC(rq)->stably_merged) > + return false; > + > + if (!interactive && !soft_rt) > + return false; > + > + return true; > +} > + > static void bfq_update_bfqq_wr_on_rq_arrival(struct bfq_data *bfqd, > struct bfq_queue *bfqq, > unsigned int old_wr_coeff, > - bool wr_or_deserves_wr, > - bool interactive, > - bool in_burst, > - bool soft_rt) > -{ > - if (old_wr_coeff == 1 && wr_or_deserves_wr) { > + struct request *rq, > + bool interactive) > +{ > + bool in_burst = bfq_bfqq_in_large_burst(bfqq); > + bool soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 && > + !BFQQ_TOTALLY_SEEKY(bfqq) && > + !in_burst && > + time_is_before_jiffies(bfqq->soft_rt_next_start) && > + bfqq->dispatched == 0 && > + bfqq->entity.new_weight == 40; > + > + if (old_wr_coeff == 1 && > + bfqq_wr_or_deserves_wr(bfqd, bfqq, rq, interactive, soft_rt)) { > /* start a weight-raising period */ > if (interactive) { > bfqq->service_from_wr = 0; > @@ -1674,9 +1724,9 @@ static void bfq_update_bfqq_wr_on_rq_arrival(struct bfq_data *bfqd, > if (interactive) { /* update wr coeff and duration */ > bfqq->wr_coeff = bfqd->bfq_wr_coeff; > bfqq->wr_cur_max_time = bfq_wr_duration(bfqd); > - } else if (in_burst) > + } else if (in_burst) { > bfqq->wr_coeff = 1; > - else if (soft_rt) { > + } else if (soft_rt) { > /* > * The application is now or still meeting the > * requirements for being deemed soft rt. We > @@ -1768,44 +1818,11 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, > struct request *rq, > bool *interactive) > { > - bool soft_rt, in_burst, wr_or_deserves_wr, > - idle_for_long_time = bfq_bfqq_idle_for_long_time(bfqd, bfqq); > + bool in_burst = bfq_bfqq_in_large_burst(bfqq); > + bool idle_for_long_time = bfq_bfqq_idle_for_long_time(bfqd, bfqq); > > - /* > - * bfqq deserves to be weight-raised if: > - * - it is sync, > - * - it does not belong to a large burst, > - * - it has been idle for enough time or is soft real-time, > - * - is linked to a bfq_io_cq (it is not shared in any sense), > - * - has a default weight (otherwise we assume the user wanted > - * to control its weight explicitly) > - */ > - in_burst = bfq_bfqq_in_large_burst(bfqq); > - soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 && > - !BFQQ_TOTALLY_SEEKY(bfqq) && > - !in_burst && > - time_is_before_jiffies(bfqq->soft_rt_next_start) && > - bfqq->dispatched == 0 && > - bfqq->entity.new_weight == 40; > *interactive = !in_burst && idle_for_long_time && > bfqq->entity.new_weight == 40; > - /* > - * Merged bfq_queues are kept out of weight-raising > - * (low-latency) mechanisms. The reason is that these queues > - * are usually created for non-interactive and > - * non-soft-real-time tasks. Yet this is not the case for > - * stably-merged queues. These queues are merged just because > - * they are created shortly after each other. So they may > - * easily serve the I/O of an interactive or soft-real time > - * application, if the application happens to spawn multiple > - * processes. So let also stably-merged queued enjoy weight > - * raising. > - */ > - wr_or_deserves_wr = bfqd->low_latency && > - (bfqq->wr_coeff > 1 || > - (bfq_bfqq_sync(bfqq) && > - (bfqq->bic || RQ_BIC(rq)->stably_merged) && > - (*interactive || soft_rt))); > > /* > * If bfqq happened to be activated in a burst, but has been > @@ -1840,11 +1857,8 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, > if (time_is_before_jiffies(bfqq->split_time + > bfqd->bfq_wr_min_idle_time)) { > bfq_update_bfqq_wr_on_rq_arrival(bfqd, bfqq, > - old_wr_coeff, > - wr_or_deserves_wr, > - *interactive, > - in_burst, > - soft_rt); > + old_wr_coeff, rq, > + *interactive); > > if (old_wr_coeff != bfqq->wr_coeff) > bfqq->entity.prio_changed = 1; > -- > 2.31.1 > -- Jan Kara <jack@suse.com> SUSE Labs, CR
WARNING: multiple messages have this Message-ID (diff)
From: Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> To: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> Cc: jack-AlSwsSmVLrQ@public.gmane.org, paolo.valente-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org, tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, yi.zhang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org Subject: Re: [PATCH -next 8/8] block, bfq: cleanup bfq_bfqq_handle_idle_busy_switch() Date: Thu, 19 May 2022 13:23:09 +0200 [thread overview] Message-ID: <20220519112309.nhbulnc5iz424ohl@quack3.lan> (raw) In-Reply-To: <20220514090522.1669270-9-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> On Sat 14-05-22 17:05:22, Yu Kuai wrote: > 'wr_or_deserves_wr' is only used in bfq_update_bfqq_wr_on_rq_arrival(), > which is only called from bfq_bfqq_handle_idle_busy_switch() in specific > code branch, thus there is no need to precaculate 'wr_or_deserves_wr' > each time bfq_bfqq_handle_idle_busy_switch() is called. > > Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> With this patch there's the same problem as with the previous one. Some of the variables passed to bfq_update_bfqq_wr_on_rq_arrival() (in_burst, soft_rt) would actually evaluate differently later in the function. Honza > --- > block/bfq-iosched.c | 110 +++++++++++++++++++++++++------------------- > 1 file changed, 62 insertions(+), 48 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 1e57d76c8dd3..cea8cb3f5ee2 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -1624,15 +1624,65 @@ static unsigned long bfq_smallest_from_now(void) > return jiffies - MAX_JIFFY_OFFSET; > } > > +/* > + * bfqq deserves to be weight-raised if: > + * - it is sync, > + * - it does not belong to a large burst, > + * - it has been idle for enough time or is soft real-time, > + * - is linked to a bfq_io_cq (it is not shared in any sense), > + * - has a default weight (otherwise we assume the user wanted > + * to control its weight explicitly) > + * > + * Merged bfq_queues are kept out of weight-raising > + * (low-latency) mechanisms. The reason is that these queues > + * are usually created for non-interactive and > + * non-soft-real-time tasks. Yet this is not the case for > + * stably-merged queues. These queues are merged just because > + * they are created shortly after each other. So they may > + * easily serve the I/O of an interactive or soft-real time > + * application, if the application happens to spawn multiple > + * processes. So let also stably-merged queued enjoy weight > + * raising. > + */ > +static bool bfqq_wr_or_deserves_wr(struct bfq_data *bfqd, > + struct bfq_queue *bfqq, > + struct request *rq, > + bool interactive, bool soft_rt) > +{ > + if (!bfqd->low_latency) > + return false; > + > + if (bfqq->wr_coeff > 1) > + return true; > + > + if (!bfq_bfqq_sync(bfqq)) > + return false; > + > + if (!bfqq->bic && !RQ_BIC(rq)->stably_merged) > + return false; > + > + if (!interactive && !soft_rt) > + return false; > + > + return true; > +} > + > static void bfq_update_bfqq_wr_on_rq_arrival(struct bfq_data *bfqd, > struct bfq_queue *bfqq, > unsigned int old_wr_coeff, > - bool wr_or_deserves_wr, > - bool interactive, > - bool in_burst, > - bool soft_rt) > -{ > - if (old_wr_coeff == 1 && wr_or_deserves_wr) { > + struct request *rq, > + bool interactive) > +{ > + bool in_burst = bfq_bfqq_in_large_burst(bfqq); > + bool soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 && > + !BFQQ_TOTALLY_SEEKY(bfqq) && > + !in_burst && > + time_is_before_jiffies(bfqq->soft_rt_next_start) && > + bfqq->dispatched == 0 && > + bfqq->entity.new_weight == 40; > + > + if (old_wr_coeff == 1 && > + bfqq_wr_or_deserves_wr(bfqd, bfqq, rq, interactive, soft_rt)) { > /* start a weight-raising period */ > if (interactive) { > bfqq->service_from_wr = 0; > @@ -1674,9 +1724,9 @@ static void bfq_update_bfqq_wr_on_rq_arrival(struct bfq_data *bfqd, > if (interactive) { /* update wr coeff and duration */ > bfqq->wr_coeff = bfqd->bfq_wr_coeff; > bfqq->wr_cur_max_time = bfq_wr_duration(bfqd); > - } else if (in_burst) > + } else if (in_burst) { > bfqq->wr_coeff = 1; > - else if (soft_rt) { > + } else if (soft_rt) { > /* > * The application is now or still meeting the > * requirements for being deemed soft rt. We > @@ -1768,44 +1818,11 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, > struct request *rq, > bool *interactive) > { > - bool soft_rt, in_burst, wr_or_deserves_wr, > - idle_for_long_time = bfq_bfqq_idle_for_long_time(bfqd, bfqq); > + bool in_burst = bfq_bfqq_in_large_burst(bfqq); > + bool idle_for_long_time = bfq_bfqq_idle_for_long_time(bfqd, bfqq); > > - /* > - * bfqq deserves to be weight-raised if: > - * - it is sync, > - * - it does not belong to a large burst, > - * - it has been idle for enough time or is soft real-time, > - * - is linked to a bfq_io_cq (it is not shared in any sense), > - * - has a default weight (otherwise we assume the user wanted > - * to control its weight explicitly) > - */ > - in_burst = bfq_bfqq_in_large_burst(bfqq); > - soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 && > - !BFQQ_TOTALLY_SEEKY(bfqq) && > - !in_burst && > - time_is_before_jiffies(bfqq->soft_rt_next_start) && > - bfqq->dispatched == 0 && > - bfqq->entity.new_weight == 40; > *interactive = !in_burst && idle_for_long_time && > bfqq->entity.new_weight == 40; > - /* > - * Merged bfq_queues are kept out of weight-raising > - * (low-latency) mechanisms. The reason is that these queues > - * are usually created for non-interactive and > - * non-soft-real-time tasks. Yet this is not the case for > - * stably-merged queues. These queues are merged just because > - * they are created shortly after each other. So they may > - * easily serve the I/O of an interactive or soft-real time > - * application, if the application happens to spawn multiple > - * processes. So let also stably-merged queued enjoy weight > - * raising. > - */ > - wr_or_deserves_wr = bfqd->low_latency && > - (bfqq->wr_coeff > 1 || > - (bfq_bfqq_sync(bfqq) && > - (bfqq->bic || RQ_BIC(rq)->stably_merged) && > - (*interactive || soft_rt))); > > /* > * If bfqq happened to be activated in a burst, but has been > @@ -1840,11 +1857,8 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, > if (time_is_before_jiffies(bfqq->split_time + > bfqd->bfq_wr_min_idle_time)) { > bfq_update_bfqq_wr_on_rq_arrival(bfqd, bfqq, > - old_wr_coeff, > - wr_or_deserves_wr, > - *interactive, > - in_burst, > - soft_rt); > + old_wr_coeff, rq, > + *interactive); > > if (old_wr_coeff != bfqq->wr_coeff) > bfqq->entity.prio_changed = 1; > -- > 2.31.1 > -- Jan Kara <jack-IBi9RG/b67k@public.gmane.org> SUSE Labs, CR
next prev parent reply other threads:[~2022-05-19 11:23 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) 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 [this message] 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=20220519112309.nhbulnc5iz424ohl@quack3.lan \ --to=jack@suse.cz \ --cc=axboe@kernel.dk \ --cc=cgroups@vger.kernel.org \ --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 \ --cc=yukuai3@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.