vrate_adj tracepoint traces vrate changes; however, it does so only
when busy_level is non-zero. busy_level turning to zero can sometimes
be as interesting an event. This patch also enables vrate_adj
tracepoint on other vrate related events - busy_level changes and
non-zero nr_lagging.
Signed-off-by: Tejun Heo <[email protected]>
---
Hello, Jens.
I've encountered vrate regulation issues while testing on a hard disk
machine. These are three patches to improve vrate adj visibility and
fix the issue.
Thanks.
block/blk-iocost.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1343,7 +1343,7 @@ static void ioc_timer_fn(struct timer_li
u32 ppm_wthr = MILLION - ioc->params.qos[QOS_WPPM];
u32 missed_ppm[2], rq_wait_pct;
u64 period_vtime;
- int i;
+ int prev_busy_level, i;
/* how were the latencies during the period? */
ioc_lat_stat(ioc, missed_ppm, &rq_wait_pct);
@@ -1531,6 +1531,7 @@ skip_surplus_transfers:
* and experiencing shortages but not surpluses, we're too stingy
* and should increase vtime rate.
*/
+ prev_busy_level = ioc->busy_level;
if (rq_wait_pct > RQ_WAIT_BUSY_PCT ||
missed_ppm[READ] > ppm_rthr ||
missed_ppm[WRITE] > ppm_wthr) {
@@ -1592,6 +1593,10 @@ skip_surplus_transfers:
atomic64_set(&ioc->vtime_rate, vrate);
ioc->inuse_margin_vtime = DIV64_U64_ROUND_UP(
ioc->period_us * vrate * INUSE_MARGIN_PCT, 100);
+ } else if (ioc->busy_level != prev_busy_level || nr_lagging) {
+ trace_iocost_ioc_vrate_adj(ioc, atomic64_read(&ioc->vtime_rate),
+ &missed_ppm, rq_wait_pct, nr_lagging,
+ nr_shortages, nr_surpluses);
}
ioc_refresh_params(ioc, false);
Some IOs may span multiple periods. As latencies are collected on
completion, the inbetween periods won't register them and may
incorrectly decide to increase vrate. nr_lagging tracks these IOs to
avoid those situations. Currently, whenever there are IOs which are
spanning from the previous period, busy_level is reset to 0 if
negative thus suppressing vrate increase.
This has the following two problems.
* When latency target percentiles aren't set, vrate adjustment should
only be governed by queue depth depletion; however, the current code
keeps nr_lagging active which pulls in latency results and can keep
down vrate unexpectedly.
* When lagging condition is detected, it resets the entire negative
busy_level. This turned out to be way too aggressive on some
devices which sometimes experience extended latencies on a small
subset of commands. In addition, a lagging IO will be accounted as
latency target miss on completion anyway and resetting busy_level
amplifies its impact unnecessarily.
This patch fixes the above two problems by disabling nr_lagging
counting when latency target percentiles aren't set and blocking vrate
increases when there are lagging IOs while leaving busy_level as-is.
Signed-off-by: Tejun Heo <[email protected]>
---
block/blk-iocost.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1407,7 +1407,8 @@ static void ioc_timer_fn(struct timer_li
* comparing vdone against period start. If lagging behind
* IOs from past periods, don't increase vrate.
*/
- if (!atomic_read(&iocg_to_blkg(iocg)->use_delay) &&
+ if ((ppm_rthr != MILLION || ppm_wthr != MILLION) &&
+ !atomic_read(&iocg_to_blkg(iocg)->use_delay) &&
time_after64(vtime, vdone) &&
time_after64(vtime, now.vnow -
MAX_LAGGING_PERIODS * period_vtime) &&
@@ -1537,21 +1538,23 @@ skip_surplus_transfers:
missed_ppm[WRITE] > ppm_wthr) {
ioc->busy_level = max(ioc->busy_level, 0);
ioc->busy_level++;
- } else if (nr_lagging) {
- ioc->busy_level = max(ioc->busy_level, 0);
- } else if (nr_shortages && !nr_surpluses &&
- rq_wait_pct <= RQ_WAIT_BUSY_PCT * UNBUSY_THR_PCT / 100 &&
+ } else if (rq_wait_pct <= RQ_WAIT_BUSY_PCT * UNBUSY_THR_PCT / 100 &&
missed_ppm[READ] <= ppm_rthr * UNBUSY_THR_PCT / 100 &&
missed_ppm[WRITE] <= ppm_wthr * UNBUSY_THR_PCT / 100) {
- ioc->busy_level = min(ioc->busy_level, 0);
- ioc->busy_level--;
+ /* take action iff there is contention */
+ if (nr_shortages && !nr_lagging) {
+ ioc->busy_level = min(ioc->busy_level, 0);
+ /* redistribute surpluses first */
+ if (!nr_surpluses)
+ ioc->busy_level--;
+ }
} else {
ioc->busy_level = 0;
}
ioc->busy_level = clamp(ioc->busy_level, -1000, 1000);
- if (ioc->busy_level) {
+ if (ioc->busy_level > 0 || (ioc->busy_level < 0 && !nr_lagging)) {
u64 vrate = atomic64_read(&ioc->vtime_rate);
u64 vrate_min = ioc->vrate_min, vrate_max = ioc->vrate_max;
The default hard disk param sets latency targets at 50ms. As the
default target percentiles are zero, these don't directly regulate
vrate; however, they're still used to calculate the period length -
100ms in this case.
This is excessively low. A SATA drive with QD32 saturated with random
IOs can easily reach avg completion latency of several hundred msecs.
A period duration which is substantially lower than avg completion
latency can lead to wildly fluctuating vrate.
Let's bump up the default latency targets to 250ms so that the period
duration is sufficiently long.
Signed-off-by: Tejun Heo <[email protected]>
---
block/blk-iocost.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -529,8 +529,8 @@ struct iocg_wake_ctx {
static const struct ioc_params autop[] = {
[AUTOP_HDD] = {
.qos = {
- [QOS_RLAT] = 50000, /* 50ms */
- [QOS_WLAT] = 50000,
+ [QOS_RLAT] = 250000, /* 250ms */
+ [QOS_WLAT] = 250000,
[QOS_MIN] = VRATE_MIN_PPM,
[QOS_MAX] = VRATE_MAX_PPM,
},
On 9/26/19 1:02 AM, Tejun Heo wrote:
> vrate_adj tracepoint traces vrate changes; however, it does so only
> when busy_level is non-zero. busy_level turning to zero can sometimes
> be as interesting an event. This patch also enables vrate_adj
> tracepoint on other vrate related events - busy_level changes and
> non-zero nr_lagging.
>
> Signed-off-by: Tejun Heo <[email protected]>
> ---
> Hello, Jens.
>
> I've encountered vrate regulation issues while testing on a hard disk
> machine. These are three patches to improve vrate adj visibility and
> fix the issue.
Applied 1-3 for 5.4, thanks Tejun.
--
Jens Axboe