Re: [dpdk-dev] [PATCH 2/3] rte_sched: introduce reciprocal divide

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 2 Dec 2015 16:45:01 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> + *     * Neither the name of Intel Corporation nor the names of its
> 
> Why is Intel mentioned here, as according to this license header Intel is not the copyright holder?

Copy/paste from other code.


> > +#ifndef _RTE_RECIPROCAL_H_
> > +#define _RTE_RECIPROCAL_H_
> > +
> > +struct rte_reciprocal {
> > +	uint32_t m;
> > +	uint8_t sh1, sh2;
> > +};
> 
> The size of this structure is not a multiple of 32 bits. You seem to transfer this structure by value rather than by reference (the function rte_reciprocal_value() below returns an instance of this structure), I don't feel comfortable with the last 16 bits of the structure being left uninitialized, we should probably add some explicit pad field and initialize this structure explicitly to zero at init time?

Shouldn't matter for inline at all.

> 
> > +
> > +static inline uint32_t rte_reciprocal_divide(uint32_t a, struct rte_reciprocal
> > R)
> > +{
> > +	uint32_t t = (uint32_t)(((uint64_t)a * R.m) >> 32);
> > +
> > +	return (t + ((a - t) >> R.sh1)) >> R.sh2;
> > +}
> > +
> > +struct rte_reciprocal rte_reciprocal_value(uint32_t d);
> 
> Why 32-bit arithmetic? We had a lot of bugs in librte_sched library due to 32-bit arithmetic that were particularly difficult to track. Can we have this function rte_reciprocal_divide() return a 64-bit integer and replace any 32-bit arithmetic/conversion with 64-bit operations?

Doing reciprocal divide by multiply requires a 2x temporary. So if it
used 64 bit math, it would require a 128 bit multiply. 


> > +
> > +#endif /* _RTE_RECIPROCAL_H_ */
> > --
> > 2.1.4
> 
> As previously discussed, a simpler/faster alternative to floating point division is 64-bit multiplication followed by right shift, any particular reason why this approach was not considered?

That is what this is. It is a 64 bit multiply (a * R.m) followed by a right shift.
The only other stuff is related to round off and scaling.

I chose to use known working algorithm rather than writing and having to
do mathematical validation of any new code.