[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 4 Apr 2017 15:28:59 +0000
From: Bart Van Assche <Bart.VanAssche@...disk.com>
To: "paolo.valente@...aro.org" <paolo.valente@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"fchecconi@...il.com" <fchecconi@...il.com>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"axboe@...nel.dk" <axboe@...nel.dk>,
"avanzini.arianna@...il.com" <avanzini.arianna@...il.com>,
"broonie@...nel.org" <broonie@...nel.org>,
"tj@...nel.org" <tj@...nel.org>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>
Subject: Re: [PATCH V2 04/16] block, bfq: modify the peak-rate estimator
On Tue, 2017-04-04 at 12:42 +0200, Paolo Valente wrote:
> > Il giorno 31 mar 2017, alle ore 17:31, Bart Van Assche <bart.vanassche@...disk.com> ha scritto:
> >
> > On Fri, 2017-03-31 at 14:47 +0200, Paolo Valente wrote:
> > > + 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.
>
> The instruction above the assignment you highlight stores in
> delta_ktime the difference between 'now' and the last budget start.
> The latter may have happened at most about 100 ms before 'now'. So
> there should be no overflow issue.
Hello Paolo,
Please double check the following code: if (delta_usecs < 1000 || delta_usecs >= LONG_MAX)
Since delta_usecs is a 32-bit variable and LONG_MAX a 64-bit constant on 64-bit systems
I'm not sure that code will do what it is intended to do.
Thanks,
Bart.
Powered by blists - more mailing lists