[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

Kristof Provost  changed:

   What|Removed |Added

 CC||pkel...@freebsd.org

--- Comment #14 from Kristof Provost  ---
pkelsey, can this be MFCd to stable/11? It'd be nice to be able to close this
as 'fixed'.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #13 from commit-h...@freebsd.org ---
A commit references this bug:

Author: pkelsey
Date: Wed Aug 22 19:38:52 UTC 2018
New revision: 338209
URL: https://svnweb.freebsd.org/changeset/base/338209

Log:
  Extended pf(4) ioctl interface and pfctl(8) to allow bandwidths of
  2^32 bps or greater to be used.  Prior to this, bandwidth parameters
  would simply wrap at the 2^32 boundary.  The computations in the HFSC
  scheduler and token bucket regulator have been modified to operate
  correctly up to at least 100 Gbps.  No other algorithms have been
  examined or modified for correct operation above 2^32 bps (some may
  have existing computation resolution or overflow issues at rates below
  that threshold).  pfctl(8) will now limit non-HFSC bandwidth
  parameters to 2^32 - 1 before passing them to the kernel.

  The extensions to the pf(4) ioctl interface have been made in a
  backwards-compatible way by versioning affected data structures,
  supporting all versions in the kernel, and implementing macros that
  will cause existing code that consumes that interface to use version 0
  without source modifications.  If version 0 consumers of the interface
  are used against a new kernel that has had bandwidth parameters of
  2^32 or greater configured by updated tools, such bandwidth parameters
  will be reported as 2^32 - 1 bps by those old consumers.

  All in-tree consumers of the pf(4) interface have been updated.  To
  update out-of-tree consumers to the latest version of the interface,
  define PFIOC_USE_LATEST ahead of any includes and use the code of
  pfctl(8) as a guide for the ioctls of interest.

  PR:   211730
  Reviewed by:  jmallett, kp, loos
  MFC after:2 weeks
  Relnotes: yes
  Sponsored by: RG Nets
  Differential Revision:https://reviews.freebsd.org/D16782

Changes:
  head/sbin/ipfw/altq.c
  head/sbin/pfctl/parse.y
  head/sbin/pfctl/pfctl.c
  head/sbin/pfctl/pfctl_altq.c
  head/sbin/pfctl/pfctl_parser.h
  head/sbin/pfctl/pfctl_qstats.c
  head/sys/net/altq/altq.h
  head/sys/net/altq/altq_cbq.c
  head/sys/net/altq/altq_cbq.h
  head/sys/net/altq/altq_codel.c
  head/sys/net/altq/altq_codel.h
  head/sys/net/altq/altq_fairq.c
  head/sys/net/altq/altq_fairq.h
  head/sys/net/altq/altq_hfsc.c
  head/sys/net/altq/altq_hfsc.h
  head/sys/net/altq/altq_priq.c
  head/sys/net/altq/altq_priq.h
  head/sys/net/altq/altq_subr.c
  head/sys/net/altq/altq_var.h
  head/sys/net/pfvar.h
  head/sys/netpfil/pf/pf_altq.h
  head/sys/netpfil/pf/pf_ioctl.c
  head/usr.sbin/bsnmpd/modules/snmp_pf/pf_snmp.c

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #12 from ncrog...@gmail.com ---
I ran into this same problem myself today on a 10G ixgbe(4) interface running
10.1-RELEASE... Did anyone manage to get a working patch going? Looks like
activity towards a resolution has died off. Even though ALTQ is unlikely to
support speeds higher than 4Gb/s on most hardware, it would be nice to know
that we can configure a 10G link speed. Currently if you set an interface altq
to 10G it confusingly becomes around 1.4Gb without warning due to the 32bit
unsigned integer.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #11 from Mahdi Mokhtari  ---
(In reply to rk from comment #10)
Hi.
Thanks for feedback on this patch.
> Interessant thing:
The result you saw makes sense, cause (as kristof pointed too) I have to change
some other structs too.
I'll update patch ASAP.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #10 from r...@starnet.cz ---
(In reply to Mahdi Mokhtari from comment #2)
Hello Mahdi,

I have recompiled kernel, but still the same:

in pf.conf:

altq on em0 cbq bandwidth 10Gb queue { default_nat,.

queue default_nat bandwidth 8000Mb cbq (borrow, red, default)

And with pfctl -nvf /etc/pf.conf
altq on em0 cbq bandwidth 1.41Gb tbrsize 36000 queue { default_nat
queue root_em0 bandwidth 1.41Gb priority 0 cbq( wrr root )

Interessant thing:

If I use in pf.conf this:
altq on em0 cbq bandwidth 5000Mb queue { default_nat, \

with pfctl I see:
altq on em0 cbq bandwidth 705.03Mb tbrsize 36000 queue { default_nat

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

Mark Linimon  changed:

   What|Removed |Added

   Keywords||patch
 CC|freebsd-am...@freebsd.org   |

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #9 from Kristof Provost  ---
That's a good question. I'd have to find a decent example somewhere myself.

The ioctl codes actually encode the size of the struct, so perhaps there's an
alternate approach.

A good first step would be to find all of the places that need to be fixed. A
patch which doesn't take the backwards compatibility into account would also be
a good thing. 

I still haven't even figured out which ioctl actually sets the altq
configuration. (The DIOCGIFSPEED call just reads the interface link speed.)

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #8 from Mahdi Mokhtari  ---
(In reply to Mahdi Mokhtari from comment #7)
Also, What about structs?
Should we use Macros around/inside them of old/new versions?
Or we should redefine new structs too ?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #7 from Mahdi Mokhtari  ---
(In reply to Kristof Provost from comment #6)
Aha :)
I see.
So you suggest we solve it by adding new IOCTL command.
Okay, lemme do a try.
When i did it, I'll upload a patch for review (if you have time).

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #6 from Kristof Provost  ---
(In reply to Mahdi Mokhtari from comment #4)
The problem with the ABI is that we can't rely on user space and kernel space
running the same code versions. If someone were to update the kernel, but not
the user space code (I don't think we support the reverse) they'd disagree
about the size of the bandwidth fields and things would break.

It'll likely be best to have two versions of the ioctl() command, one which
implements the old 32-bit behaviour (on the same ID as before!), and a new one
which implements the new 64-bit values.

That'd have to be supported for a bit, but hopefully it can be removed
eventually.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #5 from r...@starnet.cz ---
(In reply to Mahdi Mokhtari from comment #4)
Hello,

I have hardware but its production box - so when you say that you have working
patch, I can test it.

Radek

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

--- Comment #4 from Mahdi Mokhtari  ---
(In reply to Kristof Provost from comment #3)
Ah, yes i see most of pf_altq.h is int32.
And i guess we should change it too.

About your point on kernel ABI and ioctl(), i didn't get your point.
Do you mean we should make other version of DIOCGIFSPEED?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"

[Bug 211730] pf uses 32bit value for bandwith with altq

[bugzilla-noreply]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211730

Kristof Provost  changed:

   What|Removed |Added

 CC||k...@freebsd.org
   Assignee|freebsd-b...@freebsd.org|freebsd-pf@FreeBSD.org

--- Comment #1 from Kristof Provost  ---
The root cause here is that pf uses 32-bit values for bandwidth values.

Relative values don't help. DIOCGIFSPEED also uses 32-bit values (which in the
kernel are simply assigned from a 64-bit value!).

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"