Bug 211730
| Summary: | pf uses 32bit value for bandwith with altq | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | rk | ||||
| Component: | kern | Assignee: | freebsd-pf (Nobody) <pf> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Many People | CC: | kp, mmokhi, ncrogers, pi, pkelsey | ||||
| Priority: | --- | Keywords: | patch | ||||
| Version: | 10.3-RELEASE | ||||||
| Hardware: | amd64 | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
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!). Created attachment 173509 [details] Patch Makes pf_ifspeed.baudrate uint64_t (In reply to Kristof Provost from comment #1) Does the attached patch fix it ? I don't have the hardware to test this, but I'm afraid the problem is a fair bit harder to fix than just that. It looks like the altq internals also use 32 bit values for bandwidth configuration, so at a minimum altq will have to be fixed too. This change will also change the ABI between kernel and user space, so it has to be handled carefully. Likely that will mean supporting two versions for the affected ioctl() commands for at least a full release. (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? (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 (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. (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). (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 ? 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.) (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 (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. 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. 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 pkelsey, can this be MFCd to stable/11? It'd be nice to be able to close this as 'fixed'. |
Hello, I need to shape 10G traffic, but I cant make bandwidth higher than 4.26 Gbit: pfctl shows: altq on int0 cbq bandwidth 4.26Gb tbrsize 36000 queue { default_nat.............. but in pf.conf is: altq on $int_if cbq bandwidth 8550Mb queue { default_nat.......... or altq on $int_if cbq bandwidth 10Gb queue { default_nat........ When I uses relative values (percent or nothing), only 1.25 Gbit is taken from NIC driver, but ifconfig gives this: media: Ethernet autoselect (10Gbase-SR <full-duplex,rxpause,txpause>) Thank you Radek