RE: [RFC PATCH] mtd: tests: Fix eraseblock read speed miscalculation for lower partition sizes

[Amit Kumar Kumar Mahapatra <akumarma@xilinx.com>]

Hello Miquel,


> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Thursday, February 3, 2022 10:26 PM
> To: Amit Kumar Kumar Mahapatra <akumarma@xilinx.com>
> Cc: richard@nod.at; vigneshr@ti.com; linux-mtd@lists.infradead.org; linux-
> kernel@vger.kernel.org; git <git@xilinx.com>; David Oberhollenzer
> <david.oberhollenzer@sigma-star.at>
> Subject: Re: [RFC PATCH] mtd: tests: Fix eraseblock read speed miscalculation
> for lower partition sizes
> 
> Hi Amit,
> 
> +Cc: David, who's maintaining the tools. Please keep him in the
> recipients list!
> 
> amit.kumar-mahapatra@xilinx.com wrote on Thu, 3 Feb 2022 18:54:34
> +0530:
> 
> > While calculating speed during  mtd_speedtest, the time interval
> > (i.e., start - finish) is rounded off to the nearest milliseconds by
> > ignoring the fractional part. This leads to miscalculation of speed.
> > The miscalculation is more visible while running speed test on small
> > partition sizes(i.e., when partition size is equal to eraseblock size
> > or twice the eraseblock size) at higher spi frequencies.
> >
> > For e.g., while calculating eraseblock read speed for a mtd partition
> > with size equal to the eraseblock size(i.e., 64KiB) the eraseblock
> > read time interval comes out to be 966490 nanosecond. This is then
> > converted to millisecond(i.e., 0.966 msec.). The integer part (i.e., 0
> > msec) of the value is considered and the fractional part (i.e., 0.966)
> > is ignored,for calculating the eraseblock read speed. So the reported
> > eraseblock read speed is 0 KiB/s, which is incorrect.
> >
> > There are two approaches to fix this issue.
> >
> > First approach will be to keep the time interval in millisecond. and
> > round up the integer value, with this approach the 0.966msec time
> > interval in the above example will be rounded up to 1msec and this
> > value is used for calculating the speed. Downside of this approach is
> > that the reported speed is still not accurate.
> >
> > Second approach will be to convert the time interval to microseconds
> > instead of milliseconds, with this approach the 966490 nanosecond time
> > interval in the above example will be converted t0 966.490usec and
> > this value is used for calculating the speed. As compared to the
> > current implementation and the suggested First approach, this approach
> > will report a more accurate speed. Downside of this approach is that,
> > in future if the mtd size is too large then the u64 variable, that
> > holds the number of bytes, might overflow.
> >
> > In this patch we have gone with the second approach as this reports a
> > more accurate speed. With this approach the eraseblock read speed in
> > the above example comes out to be 132505 KiB/s when the spi clock is
> > configured at 150Mhz.
> >
> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
> mahapatra@xilinx.com>
> > ---
> > BRANCH: mtd/next
> > ---
> >  drivers/mtd/tests/speedtest.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/tests/speedtest.c
> > b/drivers/mtd/tests/speedtest.c index 93e76648f676..2b76e7750c68
> > 100644
> > --- a/drivers/mtd/tests/speedtest.c
> > +++ b/drivers/mtd/tests/speedtest.c
> > @@ -161,13 +161,13 @@ static inline void stop_timing(void)  static
> > long calc_speed(void)  {
> >  	uint64_t k;
> > -	long ms;
> > +	long us;
> 
> Should this be an explicit 64-bit value? And unsigned?
> unsigned long long int or uint64_t? I believe we are now 1000x closer
> to the 4GiB limit so we might need to enlarge this variable.

I will change it to uint64_t.

Regards,
Amit
> 
> >
> > -	ms = ktime_ms_delta(finish, start);
> > -	if (ms == 0)
> > +	us = ktime_us_delta(finish, start);
> > +	if (us == 0)
> >  		return 0;
> > -	k = (uint64_t)goodebcnt * (mtd->erasesize / 1024) * 1000;
> > -	do_div(k, ms);
> > +	k = (uint64_t)goodebcnt * (mtd->erasesize / 1024) * 1000000;
> > +	do_div(k, us);
> >  	return k;
> >  }
> >
> 
> Otherwise lgtm!
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> 
> Thanks,
> Miquèl