Bug 194453 - dummynet(4): pipe config bw parameter limited to 2Gbits/s (signed integer limit)
Summary: dummynet(4): pipe config bw parameter limited to 2Gbits/s (signed integer limit)
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.3-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-net mailing list
URL:
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2014-10-18 19:15 UTC by boba
Modified: 2020-01-25 03:33 UTC (History)
8 users (show)

See Also:
koobs: maintainer-feedback? (loos)
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments
Make the bandwidth internal storage an unsigned int in IPFW dummynet (3.10 KB, patch)
2020-01-02 09:55 UTC, Dani
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description boba 2014-10-18 19:15:09 UTC
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
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2014-10-20 01:38:51 UTC
Over to maintainers.
Comment 2 Hiren Panchasara freebsd_committer 2014-10-20 20:54:20 UTC
(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);
Comment 3 boba 2014-10-21 00:59:32 UTC
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.
Comment 4 boba 2014-10-24 20:18:10 UTC
(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");
Comment 5 Hiren Panchasara freebsd_committer 2014-10-24 20:27:08 UTC
(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.
Comment 6 rizzo 2014-10-24 22:15:29 UTC
(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
Comment 7 Hiren Panchasara freebsd_committer 2014-10-24 22:36:29 UTC
(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
Comment 8 boba 2014-10-26 18:11:53 UTC
> 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.
Comment 9 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:50:04 UTC
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.
Comment 10 Andriy Gapon freebsd_committer 2019-10-22 13:30:49 UTC
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
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2020-01-02 09:17:25 UTC
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
Comment 13 Kubilay Kocak freebsd_committer freebsd_triage 2020-01-02 09:21:33 UTC
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
Comment 15 Dani 2020-01-02 09:55:16 UTC
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
Comment 16 Andriy Gapon freebsd_committer 2020-01-02 12:54:54 UTC
While increasing the limit from 2 Gbps to 4 Gbps is certainly an improvement, it's not a complete solution to the the problem.
Comment 17 Goran Mekić 2020-01-02 13:05:19 UTC
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.
Comment 18 Kubilay Kocak freebsd_committer freebsd_triage 2020-01-25 03:33:13 UTC
Thank you for the patch Dani