Subject: Re: timeout issue in scsi drivers: mstohz()
To: David Laight <david@l8s.co.uk>
From: Manuel Bouyer <bouyer@antioche.eu.org>
List: tech-kern
Date: 04/04/2002 21:53:50
On Wed, Apr 03, 2002 at 10:17:31PM +0100, David Laight wrote:
> > +/*
> > + * macro to convert from milliseconds to hz without integer overflow
> > + * Default version using only 32bits arithmetics.
> > + * 64bit port can define 64bit version in their <machine/param.h>
> > + * 100000 is safe for hz < 20000
> > + */
> > +#ifndef mstohz
> > +#define mstohz(ms) \
> > +	(__predict_false((ms) >= 100000)) ? \
> > +	    (((ms) / 1000 + 1) * hz) : \
> > +	    (((ms) * hz) / 1000)
> > +#endif
> > +#endif /* _KERNEL */
> 
> For systems with restricted constants, replacing 100000
> with (say) 0x10000 is probably worthwhile.

Good point, I've remplaced it with 0x20000 (131072)

> 
> Should the values be rounded up ever?
> 
> If the values can be rounded down, dividing by 1024 will
> be a benefit on systems that don't have a fast divide.

I don't think this is safe, however. mstohz() may be used for something else
than SCSI.
And for some device the timeout may come from device specs.

> 
> Also any software divide will be faster if unsigned
> arithmetic is used.  IIRC 1000u is enough to force this,
> otherwise use ((ms) + 0u) * hz / 1000u
> 
> EEEK - you've added so many superfluous parentheses it is
> impossibly to spot you've missed the most important set!
> 
> look what (fixed_delay + mstohz( ms )) evaluates to!
> try:
> #define mstohz(ms) \
> 	(__predict_false((ms) >= 0x10000u) ? \
> 	    ((ms) + 0u) / 1000u * hz : \
> 	    ((ms) + 0u) * hz / 1000u)

Ops, good point.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
--