It's impossible to create "pipe" with bandwidth higher than 2Gbits per second. Possible due to "signed" type of variable. # ipfw pipe 1 config bw 2700mbit/s ipfw: bandwidth too large
Over to maintainers.
(In reply to boba from comment #0) > It's impossible to create "pipe" with bandwidth higher than 2Gbits per > second. Possible due to "signed" type of variable. > > # ipfw pipe 1 config bw 2700mbit/s > ipfw: bandwidth too large I think you are right that its overflowing because of "signed" type. A simple change like this may fix the problem: Index: dummynet.c =================================================================== --- dummynet.c (revision 270969) +++ dummynet.c (working copy) @@ -546,7 +546,7 @@ if_name[namelen] = '\0'; *bandwidth = 0; } else { /* read bandwidth value */ - int bw; + uint32_t bw; char *end = NULL; bw = strtoul(arg, &end, 0);
How can we be sure this change to "unsigned" type does not brake anything else ? There might be some other places in dummynet code where this variable may be used as "signed". Unfortunately my C skills are not good enough to check this myself. Anyway. Even with this patch applied, this is not full solution to the problem, this just extends limits to 4Gbits per second.
(In reply to Hiren Panchasara from comment #2) > (In reply to boba from comment #0) > > It's impossible to create "pipe" with bandwidth higher than 2Gbits per > > second. Possible due to "signed" type of variable. > > > > # ipfw pipe 1 config bw 2700mbit/s > > ipfw: bandwidth too large > > I think you are right that its overflowing because of "signed" type. > > A simple change like this may fix the problem: > > Index: dummynet.c > =================================================================== > --- dummynet.c (revision 270969) > +++ dummynet.c (working copy) > @@ -546,7 +546,7 @@ > if_name[namelen] = '\0'; > *bandwidth = 0; > } else { /* read bandwidth value */ > - int bw; > + uint32_t bw; > char *end = NULL; > > bw = strtoul(arg, &end, 0); This patch will not work at all because of following check few lines after it: if (bw < 0) errx(EX_DATAERR, "bandwidth too large");
(In reply to boba from comment #4) > (In reply to Hiren Panchasara from comment #2) > > (In reply to boba from comment #0) > > > It's impossible to create "pipe" with bandwidth higher than 2Gbits per > > > second. Possible due to "signed" type of variable. > > > > > > # ipfw pipe 1 config bw 2700mbit/s > > > ipfw: bandwidth too large > > > > I think you are right that its overflowing because of "signed" type. > > > > A simple change like this may fix the problem: > > > > Index: dummynet.c > > =================================================================== > > --- dummynet.c (revision 270969) > > +++ dummynet.c (working copy) > > @@ -546,7 +546,7 @@ > > if_name[namelen] = '\0'; > > *bandwidth = 0; > > } else { /* read bandwidth value */ > > - int bw; > > + uint32_t bw; > > char *end = NULL; > > > > bw = strtoul(arg, &end, 0); > > This patch will not work at all because of following check few lines after > it: > > if (bw < 0) > errx(EX_DATAERR, "bandwidth too large"); This patch is incomplete. But the check above is needed exactly for the reason of catching overflowing values. Afaik, a couple of things need to be done to make the patch complete: 1) make sure whatever datatype we use for bandwidth is uniform across entire dummynet codebase. i.e. in struct dn_link inside netinet/ip_dummynet.h 2) It currently only handles Mb/s and b/s and not Gb/s while setting bandwidth. If someone else doesn't beat me to it, I'll try to spend some time next week on this.
(In reply to Hiren Panchasara from comment #5) > (In reply to boba from comment #4) > > (In reply to Hiren Panchasara from comment #2) > > > (In reply to boba from comment #0) > > > > It's impossible to create "pipe" with bandwidth higher than 2Gbits per > > > > second. Possible due to "signed" type of variable. > > > > > > > > # ipfw pipe 1 config bw 2700mbit/s > > > > ipfw: bandwidth too large > > > > > > I think you are right that its overflowing because of "signed" type. > > > > > > A simple change like this may fix the problem: > > > > > > Index: dummynet.c > > > =================================================================== > > > --- dummynet.c (revision 270969) > > > +++ dummynet.c (working copy) > > > @@ -546,7 +546,7 @@ > > > if_name[namelen] = '\0'; > > > *bandwidth = 0; > > > } else { /* read bandwidth value */ > > > - int bw; > > > + uint32_t bw; > > > char *end = NULL; > > > > > > bw = strtoul(arg, &end, 0); > > > > This patch will not work at all because of following check few lines after > > it: > > > > if (bw < 0) > > errx(EX_DATAERR, "bandwidth too large"); > > This patch is incomplete. But the check above is needed exactly for the > reason of catching overflowing values. > > Afaik, a couple of things need to be done to make the patch complete: > 1) make sure whatever datatype we use for bandwidth is uniform across entire > dummynet codebase. i.e. in struct dn_link inside netinet/ip_dummynet.h > > 2) It currently only handles Mb/s and b/s and not Gb/s while setting > bandwidth. > > If someone else doesn't beat me to it, I'll try to spend some time next week > on this. I think it is a waste of time to work on just extending the input range unless one revises the internals so that dummynet can do shaping with reasonable accuracy in the Gbit/s range. By that i mean the following: First of all change the internals of dummynet to opportunistically check the timer whenever there is traffic, rather than relying on a one-tick granularity. When dummynet was first implemented the timer was the 8254, reading it took forever, and 250-1000us granularity was adequate for the <10Mbit/s range it was meant to emulate. Second, the default parameters (1ms, 50 slots queue) limit the capacity of a pipe to some 600 Mbit/s with 1500-byte packets. Probably the code should print warnings if queue_capacity/tick is too far from the desired rate. Third, the bandwidth value is internally multiplied by other factor in the execution of the scheduling algorithms. If you bump the data type from 31 to 32 or 64 bits, you also need to check that the other computations do not overflow. In any case if you decide to go through this route please pass the code by me for review before committing. -- luigi
(In reply to rizzo from comment #6) > (In reply to Hiren Panchasara from comment #5) > > (In reply to boba from comment #4) > > > (In reply to Hiren Panchasara from comment #2) > > > > (In reply to boba from comment #0) > > > > > It's impossible to create "pipe" with bandwidth higher than 2Gbits per > > > > > second. Possible due to "signed" type of variable. > > > > > > > > > > # ipfw pipe 1 config bw 2700mbit/s > > > > > ipfw: bandwidth too large > > > > > > > > I think you are right that its overflowing because of "signed" type. > > > > > > > > A simple change like this may fix the problem: > > > > > > > > Index: dummynet.c > > > > =================================================================== > > > > --- dummynet.c (revision 270969) > > > > +++ dummynet.c (working copy) > > > > @@ -546,7 +546,7 @@ > > > > if_name[namelen] = '\0'; > > > > *bandwidth = 0; > > > > } else { /* read bandwidth value */ > > > > - int bw; > > > > + uint32_t bw; > > > > char *end = NULL; > > > > > > > > bw = strtoul(arg, &end, 0); > > > > > > This patch will not work at all because of following check few lines after > > > it: > > > > > > if (bw < 0) > > > errx(EX_DATAERR, "bandwidth too large"); > > > > This patch is incomplete. But the check above is needed exactly for the > > reason of catching overflowing values. > > > > Afaik, a couple of things need to be done to make the patch complete: > > 1) make sure whatever datatype we use for bandwidth is uniform across entire > > dummynet codebase. i.e. in struct dn_link inside netinet/ip_dummynet.h > > > > 2) It currently only handles Mb/s and b/s and not Gb/s while setting > > bandwidth. > > > > If someone else doesn't beat me to it, I'll try to spend some time next week > > on this. > > I think it is a waste of time to work on just extending the input range > unless one revises the internals so that dummynet can do shaping > with reasonable accuracy in the Gbit/s range. Thanks a lot for stopping me before I waste much time on it and come up with a wrong fix. Appreciate all the details you provided. I do not have bandwidth to look into a proper fix right now. cheers, Hiren
> By that i mean the following: > First of all change the internals of dummynet to opportunistically > check the timer whenever there is traffic, rather than relying on > a one-tick granularity. When dummynet was first implemented the timer > was the 8254, reading it took forever, and 250-1000us granularity was > adequate for the <10Mbit/s range it was meant to emulate. > > Second, the default parameters (1ms, 50 slots queue) limit the capacity of > a pipe to some 600 Mbit/s with 1500-byte packets. Probably the code should > print warnings if queue_capacity/tick is too far from the desired rate. > > Third, the bandwidth value is internally multiplied by other factor > in the execution of the scheduling algorithms. If you bump the data type > from 31 to 32 or 64 bits, you also need to check that the other computations > do not overflow. > > In any case if you decide to go through this route please pass the code > by me for review before committing. -- luigi As I understood, this new framework in FreeBSD now can handle 10GB of traffic (http://info.iet.unipi.it/~luigi/netmap/) and supports pipes and dummynet. Is it production-stable ? Is there some sort of "how-to" on how to use it ? boba.
batch change: For bugs that match the following - Status Is In progress AND - Untouched since 2018-01-01. AND - Affects Base System OR Documentation DO: Reset to open status. Note: I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
This problem with dummynet still exists. Here is a recent mailing list discussion with some ideas: http://docs.freebsd.org/cgi/mid.cgi?a1c0ac18-1290-a109-1c46-c8d6047cd8c1
Another user report via IRC was isolated to this issue. They are on 11.3, but all versions through to head are affected ^Triage: Update Version to earliest currently supported FreeBSD branch
pfSense has fixed this to allow up to 4Gbit's. See: https://github.com/pfsense/FreeBSD-src/commit/2085b4c32205d4b41c4cdc810db1b9531881c824#diff-8357b1b1701d22d67098d5b7219e9c6f And the issue to it: https://redmine.pfsense.org/issues/7979
See Also: Luiz Souza via pfSense issue #7979 [1], where the issue was apparently fixed: https://redmine.pfsense.org/issues/7979#note-8 @Luiz Can you provide addition detail on this downstream resolution please
See Also: https://lists.freebsd.org/pipermail/freebsd-net/2019-September/054314.html
Created attachment 210384 [details] Make the bandwidth internal storage an unsigned int in IPFW dummynet Patch from pfSense[1] adjusted and applied to current FreeBSD-Head. [1] https://github.com/pfsense/FreeBSD-src/commit/2085b4c32205d4b41c4cdc810db1b9531881c824#diff-8357b1b1701d22d67098d5b7219e9c6f.patch
While increasing the limit from 2 Gbps to 4 Gbps is certainly an improvement, it's not a complete solution to the the problem.
I remember a discussion (probably mailing list) that tackled this problem, and one of the questions that was raised was ABI compatibility. There were 2 proposals: 1. Add new uint64_t var which would be used in higher bandwidth configs 2. Use current variable non-linearly: up to (I'm making an example) 1Gb/s it's linear, and then it's some other curve. I'm sorry I can't find it in lists archives so we could have a proper reference. I might be wrong about the exact proposals, so don't trust me blindly, but the overall idea of the two proposals is correct.
Thank you for the patch Dani
There is an commit about this subject. I think it's fixed. https://github.com/freebsd/freebsd-src/commit/20ffd88ed54fe3fd098ac30bd221275b2a14f52c#diff-21e8f70f9e33c0576b6fc6942f438cb51afb26aabeb66494549685f5ef7e5f3f
Is bumping the limit from 2 to 4 Gbit/s really a solution? Wouldn't it have been better to use a 32 bit approximation using a float type?
(In reply to Franco Fichtner from comment #20) It does not fully solve the problem, no. It was committed because it was easy, and doubling the limit is not nothing.
(In reply to Kristof Provost from comment #21) Close this pending discussion on list (or in a new issue) to solve this in the broader/general case? The issue is resolved 'as reported' (no longer a signed integer limit) and has been around since 2014.
(In reply to Kubilay Kocak from comment #22) That works for me. I have no plans to work on increasing this limit, but ae@ is working on a new and improved dummynet version which will no doubt also address this problem. As far as I know there's no timeline for that, but anyone interested in working on this problem should coordinate with him.
(In reply to Kristof Provost from comment #23) Appreciate the detail Kristof. If we're aware of any breadcrumbs available (mailing list, reviews, or elsewhere) for Andreys plans or wip, please add them here. ^Triage: Assign to committer resolving via base 20ffd88ed54f pending MFC
There's not much public detail about the new dummynet work. This is the only public reference I could find: https://reviews.freebsd.org/D28967#649493