On Wed, 27 Jan 2021 12:02:23 +0100 Simon Horman wrote:
> > > +void psched_ppscfg_precompute(struct psched_pktrate *r,
> > > + u64 pktrate64)
> > > +{
> > > + memset(r, 0, sizeof(*r));
> > > + r->rate_pkts_ps = pktrate64;
> > > + r->mult = 1;
> > > + /* The deal here is to replace a divide by a reciprocal one
> > > + * in fast path (a reciprocal divide is a multiply and a shift)
> > > + *
> > > + * Normal formula would be :
> > > + * time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
> > > + *
> > > + * We compute mult/shift to use instead :
> > > + * time_in_ns = (len * mult) >> shift;
> > > + *
> > > + * We try to get the highest possible mult value for accuracy,
> > > + * but have to make sure no overflows will ever happen.
> > > + */
> > > + if (r->rate_pkts_ps > 0) {
> > > + u64 factor = NSEC_PER_SEC;
> > > +
> > > + for (;;) {
> > > + r->mult = div64_u64(factor, r->rate_pkts_ps);
> > > + if (r->mult & (1U << 31) || factor & (1ULL << 63))
> > > + break;
> > > + factor <<= 1;
> > > + r->shift++;
> >
> > Aren't there helpers somewhere for the reciprocal divide
> > pre-calculation?
>
> Now that you mention it, yes.
>
> Looking over reciprocal_divide() I don't think it a good fit here as it
> operates on 32bit values, whereas the packet rate is 64 bit.
>
> Packet rate could be changed to a 32 bit entity if we convince ourselves we
> don't want more than 2^32 - 1 packets per second (a plausible position
> IMHO) - but that leads us to a secondary issue.
>
> The code above is very similar to an existing (long existing)
> byte rate variant of this helper - psched_ratecfg_precompute().
> And I do think we want to:
> a) Support 64-bit byte rates. Indeed such support seems to have
> been added to support 25G use-cases
> b) Calculate byte and packet rates the same way
>
> So I feel less and less that reciprocal_divide() is a good fit.
> But perhaps I am mistaken.
>
> In the meantime I will take a look to see if a helper common function can
> be made to do (64 bit) reciprocal divides for the packet and byte rate
> use-cases. I.e. the common code in psched_ppscfg_precompute() and
> psched_ratecfg_precompute().
No strong feelings, I'll just ask to document the reasoning in the
commit message or the comment above.
