On Tue, May 16, 2017 at 18:19 +0000, Carl Mascott wrote:
> At the end is the patch I mentioned against pftop.c v1.37, using a guard
> digit.
> WARNING: Untested. I couldn't even try to compile it.
>
> At this point I don't see anything wrong with your patch, Mike.
>
>
> --------------------------------------------
> On Mon, 5/15/17, Mike Belopuhov <m...@belopuhov.com> wrote:
>
> Subject: Re: pf queue definition: bandwidth resolution problem
> To: "Carl Mascott" <cmasc...@yahoo.com>
> Cc: "Theo Buehler" <t...@math.ethz.ch>, tech@openbsd.org
> Date: Monday, May 15, 2017, 3:28 PM
>
> On Sun, May 14, 2017 at 21:08 +0000, Carl
> Mascott wrote:
> > OK, I was indeed missing
> something. Thanks, Theo and Mike.
> >
> > I think I see another very minor problem,
> though.
> > Let the pf BW be 9,999,999.
> > Shift right 3 digits (divide by 1000) :
> yields 9,999K.
> > (If we were doing
> floating point arithmetic, would yield 9,999.999K.)
> > You (Mike) don't round this,
> presumably to avoid overflow to 10,000 (5 digits).
> > However, since (i < 3) it could have
> been rounded and scaled again, to 10M.
> >
> Slightly more accurate but not a big deal.
> >
>
> You're right, initially I had a "<=
> 9999" there but then I got
> sidetracked
> with a hypothetical problem that right now seems
> like my imagination. I've double checked
> numbers and indeed,
> it should be "if
> (rtmp <= 9999)".
>
> >
> >
> --------------------------------------------
> > On Sun, 5/14/17, Theo Buehler <t...@math.ethz.ch>
> wrote:
> >
> > Subject:
> Re: pf queue definition: bandwidth resolution problem
> > To: tech@openbsd.org
> > Date: Sunday, May 14, 2017, 4:35 PM
> >
> > On Sun, May 14,
> 2017 at 08:29:18PM +0000, Carl
> >
> Mascott wrote:
> > > It looks to me
> like you
> > are rounding on each
> iteration of the for-loop:
> > >
> > > + for (i = 0;
> > rate > 9999 && i <= 3;
> i++) {
> > > + rtmp = rate
> / 1000;
> > > + if (rtmp
> < 9999)
> >
> >
> This is only true in the last
> >
> iteration.
> >
> >
> > + rtmp
> > += (rate
> % 1000) / 500;
> > > +
> > rate = rtmp;
> > >
> + }
> > >
> >
> > Am I missing
> > something?
> >
> >
> >
>
> --- pftop.c.old 2017-05-14 09:51:58.620737200 -0400
> +++ pftop.c 2017-05-16 14:01:08.184811800 -0400
> @@ -1608,7 +1608,7 @@
> void
> print_queue_node(struct pfctl_queue_node *node)
> {
> - u_int rate;
> + u_int64_t rate;
> int i;
> double interval, pps, bps;
> static const char unit[] = " KMG";
> @@ -1624,9 +1624,22 @@
> // XXX: missing min, max, burst
> tb_start();
> rate = node->qs.linkshare.m2.absolute;
> - for (i = 0; rate >= 1000 && i <= 3; i++)
> - rate /= 1000;
> - tbprintf("%u%c", rate, unit[i]);
> + rate *= 10UL;
> + for (i = 0; rate > 99999UL && i <= 3; i++)
> + rate /= 1000UL;
> + if (rate % 10UL >= 5UL) {
> + rate += 10UL; /* LSD incorrect, don't care */
> + if (rate > 99999UL) {
> + if (i < 3) {
> + rate /= 1000UL;
> + i++;
> + }
> + else
> + rate -= 10UL;
> + }
> + }
> + rate /= 10UL;
> + tbprintf("%u%c", (unsigned) rate, unit[i]);
> print_fld_tb(FLD_BANDW);
>
> if (node->qstats.valid && node->qstats_last.valid)
Thanks for the diff! But I must say this is a bit of an overkill (-:
I've checked in my amended diff just now. Thanks for reporting and
helping with the fix!
