Bug 1910745

Upstream iproute v5.12 has the same issue.

Actually, this is not an issue. As you can see from lib/utils_math.c, iproute2 uses units from the International System of Units (SI), and kbps is not among them.
Closing it, feel free to reopen should you find out this is incorrect. Also, if you spot incorrect units used in iproute man pages, please open a bz and highlight them to me, so I can fix them. Thanks!

(In reply to Andrea Claudi from comment #2)
> Actually, this is not an issue. As you can see from lib/utils_math.c,
> iproute2 uses units from the International System of Units (SI), and kbps is
> not among them.
> Closing it, feel free to reopen should you find out this is incorrect. Also,
> if you spot incorrect units used in iproute man pages, please open a bz and
> highlight them to me, so I can fix them. Thanks!

Hi Andrea. I'm confused how is this "NOTABUG"? Did you mean to close this as WONTFIX?

I believe this is a bug because
a) The error message is still misleading, how is a user supposed to understand that there's an issue with the burst parameter from the error message 'Illegal "buffer"'?
b) You are mentioning 'kbps' but I am using the 'burst' value with 'kb' for the burst parameter (s. description) and this is what causes the error. By tc manpage
"       SIZES  Amounts of data. Can be specified as a floating point number followed by an optional unit:

              b or a bare number
                     Bytes.

              kbit   Kilobits

              kb or k
                     Kilobytes

              mbit   Megabits

              mb or m
                     Megabytes

              gbit   Gigabits

              gb or g
                     Gigabytes

              TC stores sizes internally as 32-bit unsigned integer in byte, so we can specify a max size of 4294967295 bytes.
" - therefore, I expect 'kb' to work.

You are saying 'kbps' is not an SI symbol, but in the tc manpage I read:
"       RATES  Bandwidths or rates.  These parameters accept a floating point number, possibly followed by either a unit (both SI and IEC
              units  supported),  or a float followed by a '%' character to specify the rate as a percentage of the device's speed (e.g.
              5%, 99.5%). Warning: specifying the rate as a percentage means a fraction of the current speed; if the speed changes,  the
              value will not be recalculated.

              bit or a bare number
                     Bits per second

              kbit   Kilobits per second

              mbit   Megabits per second

              gbit   Gigabits per second

              tbit   Terabits per second

              bps    Bytes per second

              kbps   Kilobytes per second

              mbps   Megabytes per second

              gbps   Gigabytes per second

              tbps   Terabytes per second

              To specify in IEC units, replace the SI prefix (k-, m-, g-, t-) with IEC prefix (ki-, mi-, gi- and ti-) respectively.
"
Do you mean to say this "To specify the IEC units ...." is incorrect and should be fixed?

I checked lib/utils_math.c and it doesn't list 'kbps', but it does list 'KBps', are you implying the burst parameter should be used with 'KBps' instead of 'kb'?
"
/* See http://physics.nist.gov/cuu/Units/binary.html */
static const struct rate_suffix {
	const char *name;
	double scale;
} suffixes[] = {
	{ "bit",	1. },
	{ "Kibit",	1024. },
	{ "kbit",	1000. },
	{ "mibit",	1024.*1024. },
	{ "mbit",	1000000. },
	{ "gibit",	1024.*1024.*1024. },
	{ "gbit",	1000000000. },
	{ "tibit",	1024.*1024.*1024.*1024. },
	{ "tbit",	1000000000000. },
	{ "Bps",	8. },
	{ "KiBps",	8.*1024. },
	{ "KBps",	8000. },
	{ "MiBps",	8.*1024*1024. },
	{ "MBps",	8000000. },
	{ "GiBps",	8.*1024.*1024.*1024. },
	{ "GBps",	8000000000. },
	{ "TiBps",	8.*1024.*1024.*1024.*1024. },
	{ "TBps",	8000000000000. },
	{ NULL }
};"

(In reply to smitterl from comment #3)
> Hi Andrea. I'm confused how is this "NOTABUG"? Did you mean to close this as
> WONTFIX?

Hi, I really mean NOTABUG since iproute works exactly like this since even before commit 26ab0b103c63 ("Make get_rates be table based and go up to terrabits."), where the table with suffixes above was introduced, back in 2004.
But...

> I believe this is a bug because
> a) The error message is still misleading, how is a user supposed to
> understand that there's an issue with the burst parameter from the error
> message 'Illegal "buffer"'?
> b) You are mentioning 'kbps' but I am using the 'burst' value with 'kb' for
> the burst parameter (s. description) and this is what causes the error. By
> tc manpage
> "       SIZES  Amounts of data. Can be specified as a floating point number
> followed by an optional unit:
> 
>               b or a bare number
>                      Bytes.
> 
>               kbit   Kilobits
> 
>               kb or k
>                      Kilobytes
> 
>               mbit   Megabits
> 
>               mb or m
>                      Megabytes
> 
>               gbit   Gigabits
> 
>               gb or g
>                      Gigabytes
> 
>               TC stores sizes internally as 32-bit unsigned integer in byte,
> so we can specify a max size of 4294967295 bytes.
> " - therefore, I expect 'kb' to work.
> 
> You are saying 'kbps' is not an SI symbol, but in the tc manpage I read:
> "       RATES  Bandwidths or rates.  These parameters accept a floating
> point number, possibly followed by either a unit (both SI and IEC
>               units  supported),  or a float followed by a '%' character to
> specify the rate as a percentage of the device's speed (e.g.
>               5%, 99.5%). Warning: specifying the rate as a percentage means
> a fraction of the current speed; if the speed changes,  the
>               value will not be recalculated.
> 
>               bit or a bare number
>                      Bits per second
> 
>               kbit   Kilobits per second
> 
>               mbit   Megabits per second
> 
>               gbit   Gigabits per second
> 
>               tbit   Terabits per second
> 
>               bps    Bytes per second
> 
>               kbps   Kilobytes per second
> 
>               mbps   Megabytes per second
> 
>               gbps   Gigabytes per second
> 
>               tbps   Terabytes per second
> 
>               To specify in IEC units, replace the SI prefix (k-, m-, g-,
> t-) with IEC prefix (ki-, mi-, gi- and ti-) respectively.
> "
> Do you mean to say this "To specify the IEC units ...." is incorrect and
> should be fixed?

I can see your point. Indeed, thanks for reopening, as we can use this to fix the tc manpage.
For the sake of clarity, may I change the title of this issue to "[tc-htb] misleading error message when setting burst max value"?

> I checked lib/utils_math.c and it doesn't list 'kbps', but it does list
> 'KBps', are you implying the burst parameter should be used with 'KBps'
> instead of 'kb'?

Yes, that's my point. As tc-htb uses this table, and 'kbps' is not listed here, I believe 'KBps' is the correct one for your command.
Thinking about that, I can try to improve this upstream and make this case-insensitive, do you believe this can help?

(In reply to Andrea Claudi from comment #5)
> (In reply to smitterl from comment #3)
> > Hi Andrea. I'm confused how is this "NOTABUG"? Did you mean to close this as
> > WONTFIX?
> 
> Hi, I really mean NOTABUG since iproute works exactly like this since even
> before commit 26ab0b103c63 ("Make get_rates be table based and go up to
> terrabits."), where the table with suffixes above was introduced, back in
> 2004.
> But...
> 
[...]
TBH, "works exactly like this since ..." - this means there is no regression, IMO, but not that there is not a bug
> 
> I can see your point. Indeed, thanks for reopening, as we can use this to
> fix the tc manpage.
> For the sake of clarity, may I change the title of this issue to "[tc-htb]
> misleading error message when setting burst max value"?
Makes sense if you don't plan to support abbreviations usage for the 'burst' value.

[...]
> Yes, that's my point. As tc-htb uses this table, and 'kbps' is not listed
> here, I believe 'KBps' is the correct one for your command.
> Thinking about that, I can try to improve this upstream and make this
> case-insensitive, do you believe this can help?
I don't think so. I believe this would introduce inconsistencies if done only for 'KBps' and introduce more - unnecessary - code paths for testing - even if done for all. I think we can safely assume that 'kbps' is synonymous with 'KBps' - at least it seems to be this way for 'ceil' and 'rate' parameters. I don't have access to the ISO document but if I read this correctly wikipedia uses yet other abbreviations but mentioning 'often abbreviated "kbps"' - https://en.wikipedia.org/wiki/Data-rate_units#Kilobit_per_second

To sum up:
Neither 'kbps' nor 'kb' can be used for the SIZE Amount 'burst'. By manpage, 'kb' should be used. In any case the error message reports 'Illegal "buffer"' which doesn't tell the user what was wrong in the cli call (was it ceil, rate, burst, classid,...?).

Therefore, IMO
a. minimally the error message should explicitly state that the value for 'burst' is wrong, not 'buffer', so that the user has a way to correct the value
b. ideally, either 'kb' or 'kbps' should be supported by the buffer, by consistency with current unit definitions in manpage, I'd expect 'kb' to work
c. if 'buffer' has to be entered in bytes not supporting abbreviation, then the tc-htb manpage might want to mention that explicitly

(In reply to smitterl from comment #6)
> TBH, "works exactly like this since ..." - this means there is no
> regression, IMO, but not that there is not a bug

Sorry, I wasn't very clear with my previous comment.
If you look at tc/q_htb.c source code, you can see that ceil and rate are parsed using the get_rate64() or get_percent_rate64() functions, while the burst parameter is parsed using the get_size_and_cell() function.
They are treated differently by design from the beginning, that's what I tried to say. I looked again at the "tc htb" manpage (man tc htb), and indeed it is specified over there, too:

... htb rate rate [ ceil rate ] burst bytes

'rate' and 'ceil' are rate, while for burst you should specify bytes. And that...

> > I can see your point. Indeed, thanks for reopening, as we can use this to
> > fix the tc manpage.
> > For the sake of clarity, may I change the title of this issue to "[tc-htb]
> > misleading error message when setting burst max value"?
> Makes sense if you don't plan to support abbreviations usage for the 'burst'
> value.
> 
> [...]

Makes my previous point about changing the manpage moot. tc clearly specifies you should use bytes for burst, and that's the reason for the error.

> [...] I think we can safely assume that 'kbps' is synonymous
> with 'KBps' - at least it seems to be this way for 'ceil' and 'rate'
> parameters.

That's correct, it works already like that for rates.

> To sum up:
> Neither 'kbps' nor 'kb' can be used for the SIZE Amount 'burst'. By manpage,
> 'kb' should be used. 

I do not agree, that's true for SIZE but htb deal with 'burst' in bytes and do not expect you to use a unit.
Thus this is a "VALUE" according to the main tc man page, do you agree?

> In any case the error message reports 'Illegal
> "buffer"' which doesn't tell the user what was wrong in the cli call (was it
> ceil, rate, burst, classid,...?).

According to the source code, "buffer" and "maxburst" can be used instead of "burst", but I agree with you, the error message is misleading and should be fixed.

> Therefore, IMO
> a. minimally the error message should explicitly state that the value for
> 'burst' is wrong, not 'buffer', so that the user has a way to correct the
> value
> b. ideally, either 'kb' or 'kbps' should be supported by the buffer, by
> consistency with current unit definitions in manpage, I'd expect 'kb' to work
> c. if 'buffer' has to be entered in bytes not supporting abbreviation, then
> the tc-htb manpage might want to mention that explicitly

According to the above, I'll go with a., as "tc htb" manpage already mention explicitly that "burst" should be entered in bytes.

Is that ok with you?

(In reply to Andrea Claudi from comment #7)
> (In reply to smitterl from comment #6)
> > TBH, "works exactly like this since ..." - this means there is no
> > regression, IMO, but not that there is not a bug
> 
> Sorry, I wasn't very clear with my previous comment.
> If you look at tc/q_htb.c source code, you can see that ceil and rate are
> parsed using the get_rate64() or get_percent_rate64() functions, while the
> burst parameter is parsed using the get_size_and_cell() function.
> They are treated differently by design from the beginning, that's what I
> tried to say. I looked again at the "tc htb" manpage (man tc htb), and
> indeed it is specified over there, too:
> 
> ... htb rate rate [ ceil rate ] burst bytes
> 
> 'rate' and 'ceil' are rate, while for burst you should specify bytes. And
> that...
> 
> > > I can see your point. Indeed, thanks for reopening, as we can use this to
> > > fix the tc manpage.
> > > For the sake of clarity, may I change the title of this issue to "[tc-htb]
> > > misleading error message when setting burst max value"?
> > Makes sense if you don't plan to support abbreviations usage for the 'burst'
> > value.
> > 
> > [...]
> 
> Makes my previous point about changing the manpage moot. tc clearly
> specifies you should use bytes for burst, and that's the reason for the
> error.
> 
> > [...] I think we can safely assume that 'kbps' is synonymous
> > with 'KBps' - at least it seems to be this way for 'ceil' and 'rate'
> > parameters.
> 
> That's correct, it works already like that for rates.
> 
> > To sum up:
> > Neither 'kbps' nor 'kb' can be used for the SIZE Amount 'burst'. By manpage,
> > 'kb' should be used. 
> 
> I do not agree, that's true for SIZE but htb deal with 'burst' in bytes and
> do not expect you to use a unit.
> Thus this is a "VALUE" according to the main tc man page, do you agree?
> 
I think this is just a misunderstanding, I agree with you.
> > In any case the error message reports 'Illegal
> > "buffer"' which doesn't tell the user what was wrong in the cli call (was it
> > ceil, rate, burst, classid,...?).
> 
> According to the source code, "buffer" and "maxburst" can be used instead of
> "burst", but I agree with you, the error message is misleading and should be
> fixed.
> 
> > Therefore, IMO
> > a. minimally the error message should explicitly state that the value for
> > 'burst' is wrong, not 'buffer', so that the user has a way to correct the
> > value
> > b. ideally, either 'kb' or 'kbps' should be supported by the buffer, by
> > consistency with current unit definitions in manpage, I'd expect 'kb' to work
> > c. if 'buffer' has to be entered in bytes not supporting abbreviation, then
> > the tc-htb manpage might want to mention that explicitly
> 
> According to the above, I'll go with a., as "tc htb" manpage already mention
> explicitly that "burst" should be entered in bytes.
> 
> Is that ok with you?

Sure.

Patch submitted upstream: https://patchwork.kernel.org/project/netdevbpf/patch/2cf5c0b12a53f37fee1f7af9ccc3761cbda6c030.1620297647.git.aclaudi@redhat.com/

Mingyu, can you please consider to set itm and qa_ack+ on this? This is a simple error message fix, so it should be easy to verify.

Thank you

[23:13:13@dell-per740-80 ~]1# rpm -q iproute iproute-tc
iproute-5.12.0-3.el8.x86_64
iproute-tc-5.12.0-3.el8.x86_64


[23:12:53@dell-per740-80 ~]0# tc qdisc add dev lo root handle 1: htb
[23:12:56@dell-per740-80 ~]0# tc class add dev lo parent 1: classid 1:1 htb rate 4294967kbps ceil 4294967kbps burst 4294967kb
Illegal "burst"
Usage: ... qdisc add ... htb [default N] [r2q N]
                      [direct_qlen P] [offload]
 default  minor id of class to which unclassified packets are sent {0}
 r2q      DRR quantums are computed as rate in Bps/r2q {10}
 debug    string of 16 numbers each 0-3 {0}

 direct_qlen  Limit of the direct queue {in packets}
 offload  enable hardware offload
... class add ... htb rate R1 [burst B1] [mpu B] [overhead O]
                      [prio P] [slot S] [pslot PS]
                      [ceil R2] [cburst B2] [mtu MTU] [quantum Q]
 rate     rate allocated to this class (class can still borrow)
 burst    max bytes burst which can be accumulated during idle period {computed}
 mpu      minimum packet size used in rate computations
 overhead per-packet size overhead used in rate computations
 linklay  adapting to a linklayer e.g. atm
 ceil     definite upper class rate (no borrows) {rate}
 cburst   burst but for ceil {computed}
 mtu      max packet size we create rate map for {1600}
 prio     priority of leaf; lower are served first {0}
 quantum  how much bytes to serve from leaf at once {use r2q}

TC HTB version 3.3

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (iproute bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2021:4389