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...
> >
> > >
> > > + 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.
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
