From: Stephen Hemminger <stephen@networkplumber.org>
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/3] rte_sched: introduce reciprocal divide
Date: Wed, 2 Dec 2015 14:05:45 -0800 [thread overview]
Message-ID: <20151202140545.4e3c739b@xeon-e3> (raw)
In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D8912647925BB6@IRSMSX108.ger.corp.intel.com>
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.
next prev parent reply other threads:[~2015-12-02 22:05 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-29 18:46 [dpdk-dev] [PATCH 0/3] sched: patches for 2.2 Stephen Hemminger
2015-11-29 18:46 ` [dpdk-dev] [PATCH 1/3] rte_sched: keep track of RED drops Stephen Hemminger
2015-11-29 22:12 ` Thomas Monjalon
2015-11-30 17:47 ` [dpdk-dev] [PATCH] rte_sched: drop deprecation notice for RED statistics Stephen Hemminger
2015-11-29 18:46 ` [dpdk-dev] [PATCH 2/3] rte_sched: introduce reciprocal divide Stephen Hemminger
2015-12-02 16:45 ` Dumitrescu, Cristian
2015-12-02 16:57 ` Hannes Frederic Sowa
2015-12-02 22:05 ` Stephen Hemminger [this message]
2015-11-29 18:46 ` [dpdk-dev] [PATCH 3/3] rte_sched: eliminate floating point in calculating byte clock Stephen Hemminger
2015-12-02 16:48 ` Dumitrescu, Cristian
2015-12-02 22:08 ` Stephen Hemminger
2016-03-04 15:00 ` [dpdk-dev] [PATCH 0/3] sched: patches for 2.2 Thomas Monjalon
2016-03-08 7:49 ` Dumitrescu, Cristian
2016-03-08 16:33 ` Stephen Hemminger
2016-03-08 19:53 ` Dumitrescu, Cristian
2016-03-08 20:40 ` Stephen Hemminger
2016-03-10 18:41 ` Dumitrescu, Cristian
2016-03-10 18:44 ` Stephen Hemminger
2016-03-10 18:51 ` Dumitrescu, Cristian
2016-03-13 22:25 ` Thomas Monjalon
2016-03-13 22:47 ` Dumitrescu, Cristian
2016-03-13 23:09 ` Thomas Monjalon
2016-03-14 14:40 ` Dumitrescu, Cristian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151202140545.4e3c739b@xeon-e3 \
--to=stephen@networkplumber.org \
--cc=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).