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