Re: pf queue definition: bandwidth resolution problem
On Tue, May 16, 2017 at 18:19 +, 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 +, 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 "<=
> " 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 <= )".
>
> >
> >
>
> > 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 +, Carl
> >
> Mascott wrote:
> > > It looks to me
> like you
> > are rounding on each
> iteration of the for-loop:
> > >
> > > + for (i = 0;
> > rate > && i <= 3;
> i++) {
> > > + rtmp = rate
> / 1000;
> > > + if (rtmp
> < )
> >
> >
> 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 > 9UL && i <= 3; i++)
> + rate /= 1000UL;
> + if (rate % 10UL >= 5UL) {
> + rate += 10UL; /* LSD incorrect, don't care */
> + if (rate > 9UL) {
> + 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!
Re: pf queue definition: bandwidth resolution problem
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 +, 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 "<=
" 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 <= )".
>
>
------------
> 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 +, Carl
>
Mascott wrote:
> > It looks to me
like you
> are rounding on each
iteration of the for-loop:
> >
> > + for (i = 0;
> rate > && i <= 3;
i++) {
> > + rtmp = rate
/ 1000;
> > + if (rtmp
< )
>
>
This is only true in the last
>
iteration.
>
>
> + rtmp
> += (rate
% 1000) / 500;
> > +
> rate = rtmp;
> >
+ }
> >
>
> Am I missing
> something?
>
>
>
--- pftop.c.old2017-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 > 9UL && i <= 3; i++)
+ rate /= 1000UL;
+ if (rate % 10UL >= 5UL) {
+ rate += 10UL; /* LSD incorrect, don't care */
+ if (rate > 9UL) {
+ 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)
Re: pf queue definition: bandwidth resolution problem
On Mon, May 15, 2017 at 09:28:57PM +0200, Mike Belopuhov wrote: > On Sun, May 14, 2017 at 21:08 +, 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 "<= " 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 <= )". Ah, indeed.
Re: pf queue definition: bandwidth resolution problem
On Sun, May 14, 2017 at 21:08 +, 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 "<= " 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 <= )".
>
>
> 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 +, Carl
> Mascott wrote:
> > It looks to me like you
> are rounding on each iteration of the for-loop:
> >
> > + for (i = 0;
> rate > && i <= 3; i++) {
> > + rtmp = rate / 1000;
> > + if (rtmp < )
>
> This is only true in the last
> iteration.
>
> > + rtmp
> += (rate % 1000) / 500;
> > +
> rate = rtmp;
> > + }
> >
> > Am I missing
> something?
>
>
>
Re: pf queue definition: bandwidth resolution problem
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.
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 +, Carl
Mascott wrote:
> It looks to me like you
are rounding on each iteration of the for-loop:
>
> + for (i = 0;
rate > && i <= 3; i++) {
> + rtmp = rate / 1000;
> + if (rtmp < )
This is only true in the last
iteration.
> + rtmp
+= (rate % 1000) / 500;
> +
rate = rtmp;
> + }
>
> Am I missing
something?
Re: pf queue definition: bandwidth resolution problem
On Sun, May 14, 2017 at 20:29 +, Carl Mascott wrote:
> It looks to me like you are rounding on each iteration of the for-loop:
>
> +for (i = 0; rate > && i <= 3; i++) {
> +rtmp = rate / 1000;
> +if (rtmp < )
> +rtmp += (rate % 1000) / 500;
> +rate = rtmp;
> +}
>
> Am I missing something?
>
rtmp is less than only on the last iteration.
Perhaps you're referring to "rate / 1000" as rounding
but in fact it just throws those digits away.
> I'll post my patch on Tuesday.
>
> Yes, I understand that systat can display only 4 digits for BW.
> That's 5 digits with my guard digit, which is shifted out (and not displayed)
> at the end.
> So, with the guard digit, 6 digits is too many.
>
>
> ------------
> On Sun, 5/14/17, Mike Belopuhov <m...@belopuhov.com> wrote:
>
> Subject: Re: pf queue definition: bandwidth resolution problem
> To: "Carl Mascott" <cmasc...@yahoo.com>
> Cc: tech@openbsd.org, t...@openbsd.org
> Date: Sunday, May 14, 2017, 4:05 PM
>
> On Sun, May 14, 2017 at 19:48 +, Carl
> Mascott wrote:
> > I have a suggestion RE
> your pftop.c patch. You are rounding
> >
> multiple times, after each scale operation. This is known
> as
> > rounding the intermediate results of
> a calculation and degrades
> > accuracy.
> If you're not familiar with the issue do a Google
> search
> > on rounding intermediate.
> >
>
> I round
> only once, that's what I've explained in my
> mail.
>
> > I suggest assigning the
> pf rate to an unsigned long and multiplying by 10.
> > This makes the LSD a guard digit.
> > After all scaling, round once (if guard
> digit >= 5 then add 10).
> > Yes, this
> may require one more scaling operation if it rounds up to 6
> digits (including guard digit).
> > At the
> very end divide by 10.
> > Note: This is
> essentially fixed point arithmetic with one decimal
> digit.
> >
> > I modified
> pftop.c v1.37 to do this earlier today.
> >
> It was kind of tricky.
> > I need to see if
> it still looks OK on Tuesday (I'm busy Monday).
> > Let me know if you want a patch then.
> > I won't have actually tested it,
> though, so it's highly likely to have bugs.
> > You might be better off writing your own
> (and then perhaps comparing to mine).
> >
>
> > RE my email client: It's Yahoo
> webmail -- nothing I can do about it.
> >
>
> pftop in
> systat can only display 4 digits and 1 unit symbol
> so you can't display all digits of your
> bandwidth spec.
> But please by all means,
> send your diff and I'll take a look at it.
>
>
Re: pf queue definition: bandwidth resolution problem
On Sun, May 14, 2017 at 08:29:18PM +, Carl Mascott wrote:
> It looks to me like you are rounding on each iteration of the for-loop:
>
> +for (i = 0; rate > && i <= 3; i++) {
> +rtmp = rate / 1000;
> +if (rtmp < )
This is only true in the last iteration.
> +rtmp += (rate % 1000) / 500;
> +rate = rtmp;
> +}
>
> Am I missing something?
Re: pf queue definition: bandwidth resolution problem
It looks to me like you are rounding on each iteration of the for-loop:
+for (i = 0; rate > && i <= 3; i++) {
+rtmp = rate / 1000;
+if (rtmp < )
+rtmp += (rate % 1000) / 500;
+rate = rtmp;
+}
Am I missing something?
I'll post my patch on Tuesday.
Yes, I understand that systat can display only 4 digits for BW.
That's 5 digits with my guard digit, which is shifted out (and not displayed)
at the end.
So, with the guard digit, 6 digits is too many.
On Sun, 5/14/17, Mike Belopuhov <m...@belopuhov.com> wrote:
Subject: Re: pf queue definition: bandwidth resolution problem
To: "Carl Mascott" <cmasc...@yahoo.com>
Cc: tech@openbsd.org, t...@openbsd.org
Date: Sunday, May 14, 2017, 4:05 PM
On Sun, May 14, 2017 at 19:48 +, Carl
Mascott wrote:
> I have a suggestion RE
your pftop.c patch. You are rounding
>
multiple times, after each scale operation. This is known
as
> rounding the intermediate results of
a calculation and degrades
> accuracy.
If you're not familiar with the issue do a Google
search
> on rounding intermediate.
>
I round
only once, that's what I've explained in my
mail.
> I suggest assigning the
pf rate to an unsigned long and multiplying by 10.
> This makes the LSD a guard digit.
> After all scaling, round once (if guard
digit >= 5 then add 10).
> Yes, this
may require one more scaling operation if it rounds up to 6
digits (including guard digit).
> At the
very end divide by 10.
> Note: This is
essentially fixed point arithmetic with one decimal
digit.
>
> I modified
pftop.c v1.37 to do this earlier today.
>
It was kind of tricky.
> I need to see if
it still looks OK on Tuesday (I'm busy Monday).
> Let me know if you want a patch then.
> I won't have actually tested it,
though, so it's highly likely to have bugs.
> You might be better off writing your own
(and then perhaps comparing to mine).
>
> RE my email client: It's Yahoo
webmail -- nothing I can do about it.
>
pftop in
systat can only display 4 digits and 1 unit symbol
so you can't display all digits of your
bandwidth spec.
But please by all means,
send your diff and I'll take a look at it.
Re: pf queue definition: bandwidth resolution problem
On Sun, May 14, 2017 at 19:48 +, Carl Mascott wrote: > I have a suggestion RE your pftop.c patch. You are rounding > multiple times, after each scale operation. This is known as > rounding the intermediate results of a calculation and degrades > accuracy. If you're not familiar with the issue do a Google search > on rounding intermediate. > I round only once, that's what I've explained in my mail. > I suggest assigning the pf rate to an unsigned long and multiplying by 10. > This makes the LSD a guard digit. > After all scaling, round once (if guard digit >= 5 then add 10). > Yes, this may require one more scaling operation if it rounds up to 6 digits > (including guard digit). > At the very end divide by 10. > Note: This is essentially fixed point arithmetic with one decimal digit. > > I modified pftop.c v1.37 to do this earlier today. > It was kind of tricky. > I need to see if it still looks OK on Tuesday (I'm busy Monday). > Let me know if you want a patch then. > I won't have actually tested it, though, so it's highly likely to have bugs. > You might be better off writing your own (and then perhaps comparing to mine). > > RE my email client: It's Yahoo webmail -- nothing I can do about it. > pftop in systat can only display 4 digits and 1 unit symbol so you can't display all digits of your bandwidth spec. But please by all means, send your diff and I'll take a look at it.
Re: pf queue definition: bandwidth resolution problem
I have a suggestion RE your pftop.c patch.
You are rounding multiple times, after each scale operation.
This is known as rounding the intermediate results of a calculation and
degrades accuracy.
If you're not familiar with the issue do a Google search on rounding
intermediate.
I suggest assigning the pf rate to an unsigned long and multiplying by 10.
This makes the LSD a guard digit.
After all scaling, round once (if guard digit >= 5 then add 10).
Yes, this may require one more scaling operation if it rounds up to 6 digits
(including guard digit).
At the very end divide by 10.
Note: This is essentially fixed point arithmetic with one decimal digit.
I modified pftop.c v1.37 to do this earlier today.
It was kind of tricky.
I need to see if it still looks OK on Tuesday (I'm busy Monday).
Let me know if you want a patch then.
I won't have actually tested it, though, so it's highly likely to have bugs.
You might be better off writing your own (and then perhaps comparing to mine).
RE my email client: It's Yahoo webmail -- nothing I can do about it.
On Sat, 5/13/17, Mike Belopuhov <m...@belopuhov.com> wrote:
Subject: Re: pf queue definition: bandwidth resolution problem
To: "Carl Mascott" <cmasc...@yahoo.com>
Cc: tech@openbsd.org, t...@openbsd.org
Date: Saturday, May 13, 2017, 8:02 PM
Good call. This one is a bit more complicated
since we have 5 positions
to display and the
last one is sort of reserved for the unit specifier.
So ignoring the unit we can
display numbers from 1 to .
However,
when truncating numbers like that we have to properly
round
them. I've been taught to round
this way: [12499 / 1000] = 12K and
[56500 /
1000] = 57K. I.e. I compare the remainder to 500.
YMMV.
The edge case where
we have to avoid adding this 1 is when rounding
in a previous cycle would affect rounding in
the next, i.e. 1149
would be rounded to
12M if we would unconditionally add 1.
Does the diff below look good?
On Sat, May 13, 2017 at 21:28
+, Carl Mascott wrote:
> One more
thing:
> The BW column of "systat
queues" has the same truncation error.
> I'm guessing that "systat
queues" is running "pfctl -vsqueue"
periodically, but if that's not the case then the same
fix is needed in systat.
>
>
>
>
> On Sat, 5/13/17, Carl Mascott <cmasc...@yahoo.com>
wrote:
>
> Subject:
Re: pf queue definition: bandwidth resolution problem
> To: "Mike Belopuhov" <m...@belopuhov.com>
> Cc: m...@openbsd.org
> Date: Saturday, May 13, 2017, 4:55 PM
>
> I forgot to ask:
How will I know when there's
> a
snapshot with a fixed pfctl binary?
>
Any problem with dropping the new pfctl
> binary into my 6.1-stable (i386)
system?
>
> P.S.
I'm new to OpenBSD.
>
You should not use snapshot
binaries on -stable. In fact, newer pfctl
won't work at all with an old kernel.
And please fix your email
client as it mangles mails that you quote.
Index:
sbin/pfctl/pfctl_parser.c
===
RCS file:
/home/cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.309
diff
-u -p -r1.309 pfctl_parser.c
---
sbin/pfctl/pfctl_parser.c 26 Oct 2016 14:15:59
- 1.309
+++
sbin/pfctl/pfctl_parser.c 13 May 2017 19:18:19
-
@@ -1177,7 +1177,7 @@
print_bwspec(const char *prefix, struct
printf("%s%u%%", prefix,
bw->percent);
else if
(bw->absolute) {
rate =
bw->absolute;
- for (i = 0;
rate >= 1000 && i <= 3; i++)
+ for (i = 0; rate >= 1000
&& i <= 3 && (rate % 1000 == 0); i++)
rate /= 1000;
printf("%s%u%c",
prefix, rate, unit[i]);
}
Index: usr.bin/systat/pftop.c
===
RCS file:
/home/cvs/src/usr.bin/systat/pftop.c,v
retrieving revision 1.37
diff
-u -p -r1.37 pftop.c
---
usr.bin/systat/pftop.c 3 May 2017 14:01:29 -
1.37
+++ usr.bin/systat/pftop.c 13 May
2017 23:55:38 -
@@ -1608,7 +1608,7 @@
calc_pps(u_int64_t new_pkts, u_int64_t l
void
print_queue_node(struct
pfctl_queue_node *node)
{
- u_int rate;
+ u_int rate, rtmp;
int i;
double interval, pps, bps;
static const char unit[] = " KMG";
@@ -1624,8 +1624,12 @@ print_queue_node(struct
pfctl_queue_node
// XXX: missing
min, max, burst
tb_start();
rate =
node->qs.linkshare.m2.absolute;
- for (i = 0; rate
>= 1000 && i <= 3; i++)
- rate /= 1000;
+ for (i = 0; rate > && i
<= 3; i++) {
+ rtmp = rate /
1000;
+ if (rtmp < )
+ rtmp += (rate % 1000) /
500;
+ rate = rtmp;
+ }
tbprintf("%u%c", rate, unit[i]);
print_fld_tb(FLD_BANDW);
Re: pf queue definition: bandwidth resolution problem
On Sun, May 14, 2017 at 02:02:39AM +0200, Mike Belopuhov wrote: > Good call. This one is a bit more complicated since we have 5 positions > to display and the last one is sort of reserved for the unit specifier. > > So ignoring the unit we can display numbers from 1 to . > However, when truncating numbers like that we have to properly round > them. I've been taught to round this way: [12499 / 1000] = 12K and > [56500 / 1000] = 57K. I.e. I compare the remainder to 500. YMMV. > > The edge case where we have to avoid adding this 1 is when rounding > in a previous cycle would affect rounding in the next, i.e. 1149 > would be rounded to 12M if we would unconditionally add 1. > > Does the diff below look good? Is it intentional that numbers between 999,500 and 9,999,999 are now displayed as 1000K - K, then the display jumps to 10M in systat? It would be more conventional to display these as 1M-10M, but I suppose the additional granularity isn't bad. (something similar happens at the M/G boundary, of course) Other than that, the diff looks fine to me.
Re: pf queue definition: bandwidth resolution problem
Good call. This one is a bit more complicated since we have 5 positions
to display and the last one is sort of reserved for the unit specifier.
So ignoring the unit we can display numbers from 1 to .
However, when truncating numbers like that we have to properly round
them. I've been taught to round this way: [12499 / 1000] = 12K and
[56500 / 1000] = 57K. I.e. I compare the remainder to 500. YMMV.
The edge case where we have to avoid adding this 1 is when rounding
in a previous cycle would affect rounding in the next, i.e. 1149
would be rounded to 12M if we would unconditionally add 1.
Does the diff below look good?
On Sat, May 13, 2017 at 21:28 +, Carl Mascott wrote:
> One more thing:
> The BW column of "systat queues" has the same truncation error.
> I'm guessing that "systat queues" is running "pfctl -vsqueue" periodically,
> but if that's not the case then the same fix is needed in systat.
>
>
>
>
> On Sat, 5/13/17, Carl Mascott <cmasc...@yahoo.com> wrote:
>
> Subject: Re: pf queue definition: bandwidth resolution problem
> To: "Mike Belopuhov" <m...@belopuhov.com>
> Cc: m...@openbsd.org
> Date: Saturday, May 13, 2017, 4:55 PM
>
> I forgot to ask: How will I know when there's
> a snapshot with a fixed pfctl binary?
> Any problem with dropping the new pfctl
> binary into my 6.1-stable (i386) system?
>
> P.S. I'm new to OpenBSD.
>
You should not use snapshot binaries on -stable. In fact, newer pfctl
won't work at all with an old kernel.
And please fix your email client as it mangles mails that you quote.
Index: sbin/pfctl/pfctl_parser.c
===
RCS file: /home/cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.309
diff -u -p -r1.309 pfctl_parser.c
--- sbin/pfctl/pfctl_parser.c 26 Oct 2016 14:15:59 - 1.309
+++ sbin/pfctl/pfctl_parser.c 13 May 2017 19:18:19 -
@@ -1177,7 +1177,7 @@ print_bwspec(const char *prefix, struct
printf("%s%u%%", prefix, bw->percent);
else if (bw->absolute) {
rate = bw->absolute;
- for (i = 0; rate >= 1000 && i <= 3; i++)
+ for (i = 0; rate >= 1000 && i <= 3 && (rate % 1000 == 0); i++)
rate /= 1000;
printf("%s%u%c", prefix, rate, unit[i]);
}
Index: usr.bin/systat/pftop.c
===
RCS file: /home/cvs/src/usr.bin/systat/pftop.c,v
retrieving revision 1.37
diff -u -p -r1.37 pftop.c
--- usr.bin/systat/pftop.c 3 May 2017 14:01:29 - 1.37
+++ usr.bin/systat/pftop.c 13 May 2017 23:55:38 -
@@ -1608,7 +1608,7 @@ calc_pps(u_int64_t new_pkts, u_int64_t l
void
print_queue_node(struct pfctl_queue_node *node)
{
- u_int rate;
+ u_int rate, rtmp;
int i;
double interval, pps, bps;
static const char unit[] = " KMG";
@@ -1624,8 +1624,12 @@ print_queue_node(struct pfctl_queue_node
// XXX: missing min, max, burst
tb_start();
rate = node->qs.linkshare.m2.absolute;
- for (i = 0; rate >= 1000 && i <= 3; i++)
- rate /= 1000;
+ for (i = 0; rate > && i <= 3; i++) {
+ rtmp = rate / 1000;
+ if (rtmp < )
+ rtmp += (rate % 1000) / 500;
+ rate = rtmp;
+ }
tbprintf("%u%c", rate, unit[i]);
print_fld_tb(FLD_BANDW);
Re: pf queue definition: bandwidth resolution problem
One more thing: The BW column of "systat queues" has the same truncation error. I'm guessing that "systat queues" is running "pfctl -vsqueue" periodically, but if that's not the case then the same fix is needed in systat. On Sat, 5/13/17, Carl Mascott <cmasc...@yahoo.com> wrote: Subject: Re: pf queue definition: bandwidth resolution problem To: "Mike Belopuhov" <m...@belopuhov.com> Cc: m...@openbsd.org Date: Saturday, May 13, 2017, 4:55 PM I forgot to ask: How will I know when there's a snapshot with a fixed pfctl binary? Any problem with dropping the new pfctl binary into my 6.1-stable (i386) system? P.S. I'm new to OpenBSD. On Sat, 5/13/17, Carl Mascott <cmasc...@yahoo.com> wrote: Subject: Re: pf queue definition: bandwidth resolution problem To: "Mike Belopuhov" <m...@belopuhov.com> Cc: m...@openbsd.org, tech@openbsd.org Date: Saturday, May 13, 2017, 4:21 PM First, just to be safe, I did a bandwidth test with only one queue, max bandwidth 1999K. pf is fine: measured speed was about 2M. Just eyeballing it, I don't see anything wrong with your patch, but I have no way to test it: I'm not set up to build from source. If I understand correctly you have already tested it. In that case, I guess it's OK to commit it (or however the process works..). Gee, this was easy. Thanks! On Sat, 5/13/17, Mike Belopuhov <m...@belopuhov.com> wrote: Subject: Re: pf queue definition: bandwidth resolution problem To: "Carl Mascott" <cmasc...@yahoo.com> Cc: m...@openbsd.org, tech@openbsd.org Date: Saturday, May 13, 2017, 3:23 PM Ah, I see what you mean. Indeed, we have to make sure the remainder is 0 when we're displaying the bandwidth. I think the diff below is what we want. Works fine here, any OKs? On Sat, May 13, 2017 at 18:34 +, Carl Mascott wrote: > You missed the point. I didn't do any testing. I just looked at the output of "pfctl -squeue" (correction: In the original post I wrote "pfctl -srules") and noticed that the assigned queue bandwidths reported by pfctl were in some cases much different than the specified queue bandwidth parameters in pf.conf. To put it another way, the readback of defined bandwidth does not match the definition. > > From observation it looks like specified bandwidths in pf.conf with 4 or more digits followed by 'K' have the 3 LSD's dropped and 'K' changed to 'M' in the output of "pfctl -squeue." Example: 1800K in pf.conf becomes 1M in the output of "pfctl -squeue" -- a very significant difference. > > It remains to be determined whether the fault is in the setting of bandwidth by pf or in the reporting of bandwidth by pfctl. Actual tests with a simple queue, as you suggested, could determine whether pf is enforcing the correct bandwidth values. To make it easiest to see an error the optimum leaf queue max bandwidth to use is 1999K. Then see whether the measured bandwidth is ~2M or ~1M. > > When I have time I'll do a simple test. > > > > ------------ > On Sat, 5/13/17, Mike Belopuhov <m...@belopuhov.com> wrote: > > Subject: Re: pf queue definition: bandwidth resolution problem > To: "Carl Mascott" <cmasc...@yahoo.com> > Cc: m...@openbsd.org > Date: Saturday, May 13, 2017, 12:02 PM > > On Tue, May 09, 2017 at 19:47 +, Carl > Mascott wrote: > > Intel Atom D2500 > 1.66GHz > > OpenBSD i386 v6.1-stable > > > > I can't get pf > to give me the queue bandwidths that I specify in > pf.conf. > > > > > pf.conf: > > > > queue > rootq on $ext_if bandwidth 9M max 9M qlimit 100 > > queue qdef parent rootq > bandwidth 3650K default > > > queue qrtp parent rootq bandwidth 350K min 350K burst 700K > for 200ms > > queue qweb parent > rootq bandwidth 4M > > queue > qpri parent rootq bandwidth 900K min 50K burst 1800K for > 200ms > > queue qdns parent > rootq bandwidth 100K min 10K burst 200K for 1000ms > > > > output of pfctl > -srules: > > > > queue > rootq on bge0 bandwidth 9M, max 9M
Re: pf queue definition: bandwidth resolution problem
First, just to be safe, I did a bandwidth test with only one queue, max bandwidth 1999K. pf is fine: measured speed was about 2M. Just eyeballing it, I don't see anything wrong with your patch, but I have no way to test it: I'm not set up to build from source. If I understand correctly you have already tested it. In that case, I guess it's OK to commit it (or however the process works..). Gee, this was easy. Thanks! On Sat, 5/13/17, Mike Belopuhov <m...@belopuhov.com> wrote: Subject: Re: pf queue definition: bandwidth resolution problem To: "Carl Mascott" <cmasc...@yahoo.com> Cc: m...@openbsd.org, tech@openbsd.org Date: Saturday, May 13, 2017, 3:23 PM Ah, I see what you mean. Indeed, we have to make sure the remainder is 0 when we're displaying the bandwidth. I think the diff below is what we want. Works fine here, any OKs? On Sat, May 13, 2017 at 18:34 +, Carl Mascott wrote: > You missed the point. I didn't do any testing. I just looked at the output of "pfctl -squeue" (correction: In the original post I wrote "pfctl -srules") and noticed that the assigned queue bandwidths reported by pfctl were in some cases much different than the specified queue bandwidth parameters in pf.conf. To put it another way, the readback of defined bandwidth does not match the definition. > > From observation it looks like specified bandwidths in pf.conf with 4 or more digits followed by 'K' have the 3 LSD's dropped and 'K' changed to 'M' in the output of "pfctl -squeue." Example: 1800K in pf.conf becomes 1M in the output of "pfctl -squeue" -- a very significant difference. > > It remains to be determined whether the fault is in the setting of bandwidth by pf or in the reporting of bandwidth by pfctl. Actual tests with a simple queue, as you suggested, could determine whether pf is enforcing the correct bandwidth values. To make it easiest to see an error the optimum leaf queue max bandwidth to use is 1999K. Then see whether the measured bandwidth is ~2M or ~1M. > > When I have time I'll do a simple test. > > > > -------- > On Sat, 5/13/17, Mike Belopuhov <m...@belopuhov.com> wrote: > > Subject: Re: pf queue definition: bandwidth resolution problem > To: "Carl Mascott" <cmasc...@yahoo.com> > Cc: m...@openbsd.org > Date: Saturday, May 13, 2017, 12:02 PM > > On Tue, May 09, 2017 at 19:47 +, Carl > Mascott wrote: > > Intel Atom D2500 > 1.66GHz > > OpenBSD i386 v6.1-stable > > > > I can't get pf > to give me the queue bandwidths that I specify in > pf.conf. > > > > > pf.conf: > > > > queue > rootq on $ext_if bandwidth 9M max 9M qlimit 100 > > queue qdef parent rootq > bandwidth 3650K default > > > queue qrtp parent rootq bandwidth 350K min 350K burst 700K > for 200ms > > queue qweb parent > rootq bandwidth 4M > > queue > qpri parent rootq bandwidth 900K min 50K burst 1800K for > 200ms > > queue qdns parent > rootq bandwidth 100K min 10K burst 200K for 1000ms > > > > output of pfctl > -srules: > > > > queue > rootq on bge0 bandwidth 9M, max 9M qlimit 100 > > queue qdef parent rootq bandwidth 3M > default qlimit 50 > > queue qrtp parent > rootq bandwidth 350K, min 350K burst 700K for 200ms qlimit > 50 > > queue qweb parent rootq bandwidth 4M > qlimit 50 > > queue qpri parent rootq > bandwidth 900K, min 50K burst 1M for 200ms qlimit 50 > > queue qdns parent rootq bandwidth 100K, > min 10K burst 200K for 1000ms qlimit 50 > > > > > Discrepancies in the above: > > > > > defined actual > > > -- - > > qdef BW 3650K 3M > > qpri burst 1800K 1M > > > > It looks like for > anything specified as abcdK the result is aM, i.e., for any > bandwidth >= 1000K the resulting bandwidth is > truncated (not rounded) to M, where > = most significant digit. Any bandwidth > < 1000K works correctly. > > > > Is this a bug, a misfeature, or a > feature? > > Thanks! > > > Index: sbin/pfctl/pfctl_parser.c === RCS file: /home/cvs/src/sbin/pfctl/pfctl_parser.c,v retrieving revision 1.309 diff -u -p -r1.309 pfctl_parser.c --- sbin/pfctl/pfctl_parser.c 26 Oct 2016 14:15:59 -0
Re: pf queue definition: bandwidth resolution problem
Ah, I see what you mean. Indeed, we have to make sure the remainder
is 0 when we're displaying the bandwidth. I think the diff below is
what we want. Works fine here, any OKs?
On Sat, May 13, 2017 at 18:34 +, Carl Mascott wrote:
> You missed the point. I didn't do any testing. I just looked at the output of
> "pfctl -squeue" (correction: In the original post I wrote "pfctl -srules")
> and noticed that the assigned queue bandwidths reported by pfctl were in some
> cases much different than the specified queue bandwidth parameters in
> pf.conf. To put it another way, the readback of defined bandwidth does not
> match the definition.
>
> From observation it looks like specified bandwidths in pf.conf with 4 or more
> digits followed by 'K' have the 3 LSD's dropped and 'K' changed to 'M' in the
> output of "pfctl -squeue." Example: 1800K in pf.conf becomes 1M in the output
> of "pfctl -squeue" -- a very significant difference.
>
> It remains to be determined whether the fault is in the setting of bandwidth
> by pf or in the reporting of bandwidth by pfctl. Actual tests with a simple
> queue, as you suggested, could determine whether pf is enforcing the correct
> bandwidth values. To make it easiest to see an error the optimum leaf queue
> max bandwidth to use is 1999K. Then see whether the measured bandwidth is ~2M
> or ~1M.
>
> When I have time I'll do a simple test.
>
>
>
> ----------------
> On Sat, 5/13/17, Mike Belopuhov <m...@belopuhov.com> wrote:
>
> Subject: Re: pf queue definition: bandwidth resolution problem
> To: "Carl Mascott" <cmasc...@yahoo.com>
> Cc: m...@openbsd.org
> Date: Saturday, May 13, 2017, 12:02 PM
>
> On Tue, May 09, 2017 at 19:47 +, Carl
> Mascott wrote:
> > Intel Atom D2500
> 1.66GHz
> > OpenBSD i386 v6.1-stable
> >
> > I can't get pf
> to give me the queue bandwidths that I specify in
> pf.conf.
> >
> >
> pf.conf:
> >
> > queue
> rootq on $ext_if bandwidth 9M max 9M qlimit 100
> > queue qdef parent rootq
> bandwidth 3650K default
> >
> queue qrtp parent rootq bandwidth 350K min 350K burst 700K
> for 200ms
> > queue qweb parent
> rootq bandwidth 4M
> > queue
> qpri parent rootq bandwidth 900K min 50K burst 1800K for
> 200ms
> > queue qdns parent
> rootq bandwidth 100K min 10K burst 200K for 1000ms
> >
> > output of pfctl
> -srules:
> >
> > queue
> rootq on bge0 bandwidth 9M, max 9M qlimit 100
> > queue qdef parent rootq bandwidth 3M
> default qlimit 50
> > queue qrtp parent
> rootq bandwidth 350K, min 350K burst 700K for 200ms qlimit
> 50
> > queue qweb parent rootq bandwidth 4M
> qlimit 50
> > queue qpri parent rootq
> bandwidth 900K, min 50K burst 1M for 200ms qlimit 50
> > queue qdns parent rootq bandwidth 100K,
> min 10K burst 200K for 1000ms qlimit 50
> >
>
> > Discrepancies in the above:
> >
> >
> defined actual
> >
> -- -
> > qdef BW 3650K 3M
> > qpri burst 1800K 1M
> >
> > It looks like for
> anything specified as abcdK the result is aM, i.e., for any
> bandwidth >= 1000K the resulting bandwidth is
> truncated (not rounded) to M, where
> = most significant digit. Any bandwidth
> < 1000K works correctly.
> >
> > Is this a bug, a misfeature, or a
> feature?
> > Thanks!
> >
>
Index: sbin/pfctl/pfctl_parser.c
===
RCS file: /home/cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.309
diff -u -p -r1.309 pfctl_parser.c
--- sbin/pfctl/pfctl_parser.c 26 Oct 2016 14:15:59 - 1.309
+++ sbin/pfctl/pfctl_parser.c 13 May 2017 19:18:19 -
@@ -1177,7 +1177,7 @@ print_bwspec(const char *prefix, struct
printf("%s%u%%", prefix, bw->percent);
else if (bw->absolute) {
rate = bw->absolute;
- for (i = 0; rate >= 1000 && i <= 3; i++)
+ for (i = 0; rate >= 1000 && i <= 3 && (rate % 1000 == 0); i++)
rate /= 1000;
printf("%s%u%c", prefix, rate, unit[i]);
}
