Re: [kvm-unit-tests PATCH V4 4/5] lib/powerpc: Implement generic delay function for use in unit test

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

On Thu, 2016-08-18 at 12:24 +0200, Andrew Jones wrote:
> On Thu, Aug 18, 2016 at 02:39:07PM +1000, Suraj Jitindar Singh wrote:
> > 
> > On Wed, 2016-08-17 at 15:04 +0200, Andrew Jones wrote:
> > > 
> > > On Wed, Aug 17, 2016 at 04:48:57PM +1000, Suraj Jitindar Singh
> > > wrote:
> > > > 
> > > > +
> > > > +void delay(uint64_t cycles)
> > > > +{
> > > > +	uint64_t start = get_tb();
> > > > +	/*
> > > > +	 * Pretty unlikely unless your server has been on for,
> > > > or
> > > > you want to
> > > > +	 * delay for, over 1000 years, but still.
> > > > +	 */
> > > > +	assert(cycles < (UINT64_MAX - start));
> > > > +	while ((get_tb() - start) < cycles)
> > > I don't think the above assert is necessary. As long as the
> > > subtraction
> > > (get_tb() - start) produces a uint64_t, then the condition should
> > > always
> > > work - per C99. Maybe it should be written as (uint64_t)(get_tb()
> > > -
> > > start)
> > > to be 100% correct though.
> > This is to catch the case where the caller passes a ridiculously
> > large
> > cycles value (e.g. UINT64_MAX - 1) and/or start is sufficiently
> > large
> > that get_tb() - start will never be >= to cycles because the time-
> > base
> > counter will overflow and wrap around before that ever becomes
> > true.
> > This is super unlikely but will avoid an infinite loop in the event
> > someone does it.
> I understand that. What I'm saying is that with unsigned integer
> subtraction, per C99, you don't have to worry about it. Just try
> 
> #include <stdio.h>
> #include <stdint.h>
> int main(void)
> {
> 	uint64_t start = UINT64_MAX - 1;
> 	uint64_t tb = 5; // tb wrapped
> 	uint64_t cycles = 10;
> 
> 	if ((tb - start) < cycles)
> 		printf("works fine\n");
> 	return 0;
> }
> 
> to see that it works fine. As for guarding against ridiculously large
> cycles inputs, don't bother. How do you even define what's
> ridiculous?
> I'd say it's anything more than a minute...
Ok I understand what you're saying now. I'll bin the assert.
Thanks.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +		cpu_relax();
> > > > +}
> > > > +
> > > > +void udelay(uint64_t us)
> > > > +{
> > > > +	assert(us < (UINT64_MAX / tb_hz));
> > > Same comment here. I'm pretty sure (wrap around wraps my head, so
> > > I
> > > could be wrong) that the main concern is maintaining unsigned
> > > integer
> > > subtraction, which the C99 guarantees to wrap modulo 2^N, N being
> > > the
> > > number of bits of the unsigned integer.
> > This is to catch when the caller tries to sleep for > 36000000000us
> > (10
> > hrs), which I realise is highly unlikely. But in this case us *
> > tb_hz
> > will be too big to store in a u64. Thus this won't delay for the
> > intended time, hence the assert.
> If the caller does that, the we're doing him a favor by wrapping
> the input... More seriously, while I'm in favor of asserts for
> external inputs (DT reads, command line inputs), I think it's safe
> to expect unit test developers to just not do something like this,
> or to be able to debug it themselves when they do, without the aid
> of asserts pinpointing the mistake for them.
Makes sense, I'll ditch this assert as well.
> 
> Thanks,
> drew
Thanks