Bug 211730 - pf uses 32bit value for bandwith with altq
Summary: pf uses 32bit value for bandwith with altq
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.3-RELEASE
Hardware: amd64 Any
: --- Affects Many People
Assignee: freebsd-pf mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-08-10 13:22 UTC by rk
Modified: 2018-10-19 21:33 UTC (History)
5 users (show)

See Also:


Attachments
Patch Makes pf_ifspeed.baudrate uint64_t (819 bytes, patch)
2016-08-10 14:13 UTC, Mahdi Mokhtari
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rk 2016-08-10 13:22:38 UTC
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
Comment 1 Kristof Provost freebsd_committer 2016-08-10 13:33:49 UTC
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!).
Comment 2 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-08-10 14:13:14 UTC
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 ?
Comment 3 Kristof Provost freebsd_committer 2016-08-10 14:20:23 UTC
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.
Comment 4 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-08-10 14:37:35 UTC
(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?
Comment 5 rk 2016-08-10 16:58:52 UTC
(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
Comment 6 Kristof Provost freebsd_committer 2016-08-10 17:41:43 UTC
(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.
Comment 7 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-08-10 18:48:54 UTC
(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).
Comment 8 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-08-10 18:53:31 UTC
(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 ?
Comment 9 Kristof Provost freebsd_committer 2016-08-10 19:05:59 UTC
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.)
Comment 10 rk 2016-08-11 20:35:15 UTC
(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
Comment 11 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-08-11 20:44:50 UTC
(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.
Comment 12 ncrogers 2016-12-13 23:42:06 UTC
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.
Comment 13 commit-hook freebsd_committer 2018-08-22 19:39:27 UTC
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
Comment 14 Kristof Provost freebsd_committer 2018-10-19 21:33:06 UTC
pkelsey, can this be MFCd to stable/11? It'd be nice to be able to close this as 'fixed'.