From: Bart Van Assche <Bart.VanAssche@sandisk.com>
To: "tj@kernel.org" <tj@kernel.org>,
        "paolo.valente@linaro.org" <paolo.valente@linaro.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>
CC: "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "fchecconi@gmail.com" <fchecconi@gmail.com>,
        "avanzini.arianna@gmail.com" <avanzini.arianna@gmail.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "broonie@kernel.org" <broonie@kernel.org>
Subject: Re: [PATCH V2 04/16] block, bfq: modify the peak-rate estimator
Thread-Topic: [PATCH V2 04/16] block, bfq: modify the peak-rate estimator
Thread-Index: AQHSqjPmqK7Jhq4010+VKTKsNo1cqQ==
Date: Fri, 31 Mar 2017 15:31:44 +0000
Message-ID: <1490974279.2587.5.camel@sandisk.com>
References: <20170331124743.3530-1-paolo.valente@linaro.org>
         <20170331124743.3530-5-paolo.valente@linaro.org>
In-Reply-To: <20170331124743.3530-5-paolo.valente@linaro.org>
Accept-Language: en-US, nl-NL
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <1877D44A2B72FA4F816ADEB88ACBC9D2@sandisk.com>
MIME-Version: 1.0
WDCIPOUTBOUND: EOP-TRUE
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2017 15:31:49.4328
 (UTC)
X-MS-Exchange-CrossTenant-Id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=b61c8803-16f3-4c35-9b17-6f65f441df86;Ip=[74.221.232.55];Helo=[sacsmgep15.sandisk.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR04MB388
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v2VFW2XE027968
Content-Length: 2187
Lines: 50

On Fri, 2017-03-31 at 14:47 +0200, Paolo Valente wrote:
> -static bool bfq_update_peak_rate(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> -??????????????????????????????? bool compensate)
> +static bool bfq_bfqq_is_slow(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> +??????????????????????????????? bool compensate, enum bfqq_expiration reason,
> +??????????????????????????????? unsigned long *delta_ms)
> ?{
> -???????u64 bw, usecs, expected, timeout;
> -???????ktime_t delta;
> -???????int update = 0;
> +???????ktime_t delta_ktime;
> +???????u32 delta_usecs;
> +???????bool slow = BFQQ_SEEKY(bfqq); /* if delta too short, use seekyness */
> ?
> -???????if (!bfq_bfqq_sync(bfqq) || bfq_bfqq_budget_new(bfqq))
> +???????if (!bfq_bfqq_sync(bfqq))
> ????????????????return false;
> ?
> ????????if (compensate)
> -???????????????delta = bfqd->last_idling_start;
> +???????????????delta_ktime = bfqd->last_idling_start;
> ????????else
> -???????????????delta = ktime_get();
> -???????delta = ktime_sub(delta, bfqd->last_budget_start);
> -???????usecs = ktime_to_us(delta);
> -
> -???????/* Don't trust short/unrealistic values. */
> -???????if (usecs < 100 || usecs >= LONG_MAX)
> -???????????????return false;
> -
> -???????/*
> -??????? * Calculate the bandwidth for the last slice.? We use a 64 bit
> -??????? * value to store the peak rate, in sectors per usec in fixed
> -??????? * point math.? We do so to have enough precision in the estimate
> -??????? * and to avoid overflows.
> -??????? */
> -???????bw = (u64)bfqq->entity.service << BFQ_RATE_SHIFT;
> -???????do_div(bw, (unsigned long)usecs);
> +???????????????delta_ktime = ktime_get();
> +???????delta_ktime = ktime_sub(delta_ktime, bfqd->last_budget_start);
> +???????delta_usecs = ktime_to_us(delta_ktime);
> +

This patch changes the type of the variable in which the result of ktime_to_us()
is stored from u64 into u32 and next compares that result with LONG_MAX. Since
ktime_to_us() returns a signed 64-bit number, are you sure you want to store that
result in a 32-bit variable? If ktime_to_us() would e.g. return 0xffffffff00000100
or 0x100000100 then the assignment will truncate these numbers to 0x100.

Bart.