LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
To: Paolo Valente <paolo.valente@linaro.org>
Cc: linux-block <linux-block@vger.kernel.org>,
	Jens Axboe <axboe@kernel.dk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"'Paolo Valente' via bfq-iosched" <bfq-iosched@googlegroups.com>,
	Mark Brown <broonie@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH] block, bfq: keep peak_rate estimation within range 1..2^32-1
Date: Tue, 20 Mar 2018 10:41:24 +0300	[thread overview]
Message-ID: <38320074-d1e6-96bc-e832-e535efc07e81@yandex-team.ru> (raw)
In-Reply-To: <E9AC28A6-848D-420F-8DAB-8B1FC709BA38@linaro.org>

On 20.03.2018 06:00, Paolo Valente wrote:
> 
> 
>> Il giorno 19 mar 2018, alle ore 14:28, Konstantin Khlebnikov <khlebnikov@yandex-team.ru> ha scritto:
>>
>> On 19.03.2018 09:03, Paolo Valente wrote:
>>>> Il giorno 05 mar 2018, alle ore 04:48, Konstantin Khlebnikov <khlebnikov@yandex-team.ru> ha scritto:
>>>>
>>>> Rate should never overflow or become zero because it is used as divider.
>>>> This patch accumulates it with saturation.
>>>>
>>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>>> ---
>>>> block/bfq-iosched.c |    8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>>> index aeca22d91101..a236c8d541b5 100644
>>>> --- a/block/bfq-iosched.c
>>>> +++ b/block/bfq-iosched.c
>>>> @@ -2546,7 +2546,8 @@ static void bfq_reset_rate_computation(struct bfq_data *bfqd,
>>>>
>>>> static void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq)
>>>> {
>>>> -	u32 rate, weight, divisor;
>>>> +	u32 weight, divisor;
>>>> +	u64 rate;
>>>>
>>>> 	/*
>>>> 	 * For the convergence property to hold (see comments on
>>>> @@ -2634,9 +2635,10 @@ static void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq)
>>>> 	 */
>>>> 	bfqd->peak_rate *= divisor-1;
>>>> 	bfqd->peak_rate /= divisor;
>>>> -	rate /= divisor; /* smoothing constant alpha = 1/divisor */
>>>> +	do_div(rate, divisor);	/* smoothing constant alpha = 1/divisor */
>>>>
>>>> -	bfqd->peak_rate += rate;
>>>> +	/* rate should never overlow or become zero */
>>> It is bfqd->peak_rate that is used as a divider, and bfqd->peak_rate doesn't risk to be zero even if the variable 'rate' is zero here.
>>> So I guess the reason why you consider the possibility that bfqd->peak_rate becomes zero is because of an overflow when summing 'rate'. But, according to my calculations, this should be impossible with devices with sensible speeds.
>>> These are the reasons why I decided I could make it with a 32-bit variable, without any additional clamping. Did I make any mistake in my evaluation?
>>
>> According to Murphy's law this is inevitable..
>>
> 
> Yep.  Actually Murphy has been even clement this time, by making the
> failure occur to a kernel expert :)
> 
>> I've seen couple division by zero crashes in bfq_wr_duration.
>> Unfortunately logs weren't recorded.
>>
>>> Anyway, even if I made some mistake about the maximum possible value of the device rate, and the latter may be too high for bfqd->peak_rate to contain it, then I guess the right solution would not be to clamp the actual rate to U32_MAX, but to move bfqd->peak_rate to 64 bits. Or am I missing something else?
>>>>> +	bfqd->peak_rate = clamp_t(u64, rate + bfqd->peak_rate, 1, U32_MAX);
>>
>> 32-bit should be enough and better for division.
>> My patch makes sure it never overflows/underflows.
>> That's cheaper than full 64-bit/64-bit division.
>> Anyway 64-bit speed could overflow too. =)
>>
> 
> I see your point.  Still, if the mistake is not in sizing, then you
> bumped into some odd bug.  In this respect, I don't like much the idea
> of sweeping the dust under the carpet.  So, let me ask you for a
> little bit more help.  With your patch applied, and thus with no risk
> of crashes, what about adding, right before your clamp_t, something
> like:
> 
> if (!bfqd->peak_rate)
> 	pr_crit(<dump of all the variables involved in updating bfqd->peak_rate>);
> 
> Once the failure shows up (Murphy permitting), we might have hints to
> the bug causing it.
> 
> Apart from that, I have no problem with patches that make bfq more
> robust, even in a sort of black-box way.

This rate estimator is already works as a black-box with smoothing and
low-pass filter inside. It has limitations thus this is ok to declare
range of expected\sane results.

It would be nice to show estimated rate in sysfs to let user
diagnose whenever it is wrong for a long period of time.
Printing message all the time even ratelimited is a wrong idea.
If this should never happens - WARN_ONCE is more than enough.

I suspect crashes might be caused by bumpy timer which was affected by smi/nmi from mce.
I'll try to find evidence.

> 
> Thanks a lot,
> Paolo
> 
>>
>>>> 	update_thr_responsiveness_params(bfqd);
>>>>
>>>> reset_computation:
> 

  reply	other threads:[~2018-03-20  7:41 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-05 12:48 Konstantin Khlebnikov
2018-03-19  6:03 ` Paolo Valente
2018-03-19  6:28   ` Konstantin Khlebnikov
2018-03-20  3:00     ` Paolo Valente
2018-03-20  7:41       ` Konstantin Khlebnikov [this message]
2018-03-20 23:49         ` Paolo Valente
2018-03-26  8:01           ` Paolo Valente
2018-03-26 10:28             ` Konstantin Khlebnikov
2018-03-26 14:06               ` Paolo Valente
2018-03-26 14:18                 ` Konstantin Khlebnikov

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=38320074-d1e6-96bc-e832-e535efc07e81@yandex-team.ru \
    --to=khlebnikov@yandex-team.ru \
    --cc=axboe@kernel.dk \
    --cc=bfq-iosched@googlegroups.com \
    --cc=broonie@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paolo.valente@linaro.org \
    --cc=ulf.hansson@linaro.org \
    --subject='Re: [PATCH] block, bfq: keep peak_rate estimation within range 1..2^32-1' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).