You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
This seems rather unfortunate to incur one gratuitous division per packet. Since the 64/65 encoding is happening independent of data being send it seems much more efficient to adjust the shaper rate by multiplying with (64/65) and flooring that result, an operation which can be performed in the tc front-end and would not require (additional) per-packet processing. Since I believe I have brought this up as comment to the original PTM patch I wonder why you choose to solve this in this expensive fashion? Especially since for most users the shaper rate number will be larger than the packet size which will cause less rounding error...
The reason will be displayed to describe this comment to others. Learn more.
On 4 Oct, 2016, at 11:30, moeller0 ***@***.*** wrote:
This seems rather unfortunate to incur one gratuitous division per packet.
Division by 64 is optimised by GCC into a right-shift by 6 bits. This is a single-cycle operation on almost all modern CPUs, except ARM where it is folded into the following addition and thus takes zero cycles.
- Jonathan Morton
The reason will be displayed to describe this comment to others. Learn more.
On Oct 4, 2016, at 13:21 , chromi ***@***.*** wrote:
> On 4 Oct, 2016, at 11:30, moeller0 ***@***.*** wrote:
>
> This seems rather unfortunate to incur one gratuitous division per packet.
Division by 64 is optimised by GCC into a right-shift by 6 bits. This is a single-cycle operation on almost all modern CPUs, except ARM where it is folded into the following addition and thus takes zero cycles.
```
That still makes it a gratuitous shift operation, at 1GHz you are still wasting a cycle (1 ns) and even on ARM your argument does not gold much water. If we look at the code:
```
out += (out / 64) + !!(out % 64);
Your argument seems to be that the “/“ on ARM does not cost more than the following “+” and hence i free. And that gcc will replace the “/“ with shifts and hence the accounting is free. My argument is that the whole line is gratuitous (or rather would be if you did floor(specified_rate \* 64 /65) in tc when called wih the PTM keyword). So at least argue about the cost of all the 5 operations in that line please.
Question how can I improve my communication with you so that we can actually have thoughtful civil discussions (even if we in the end might agree to disagree) instead of these rather peculiar exchanges we seem to run in every now and then. I could refrain from commenting but I am not sure you would prefer me proposing patches instead…
The reason will be displayed to describe this comment to others. Learn more.
On 4 Oct, 2016, at 15:13, moeller0 ***@***.*** wrote:
So at least argue about the cost of all the 5 operations in that line please.
Five operations which compile to four single-cycle instructions on ARM and MIPS - and _three_ on ARM64. PowerPC takes five instructions (but is likely to execute them in parallel), and AMD64 takes seven (but has the resources to cope).
```
https://godbolt.org/g/aXqrcl
```
I’m quite experienced in writing efficient code for RISC CPUs.
And we’re literally talking about four cycles (on the CPUs most used in consumer-grade routers) out of how many already incurred per packet? I have better places to spend my limited mental energy.
- Jonathan Morton
The reason will be displayed to describe this comment to others. Learn more.
Hi Jonathan,
On Oct 4, 2016, at 14:41 , chromi ***@***.*** wrote:
> On 4 Oct, 2016, at 15:13, moeller0 ***@***.*** wrote:
>
> So at least argue about the cost of all the 5 operations in that line please.
Five operations which compile to four single-cycle instructions on ARM and MIPS - and _three_ on ARM64. PowerPC takes five instructions (but is likely to execute them in parallel), and AMD64 takes seven (but has the resources to cope).
https://godbolt.org/g/aXqrcl
I’m quite experienced in writing efficient code for RISC CPUs.
```
Missing the forrest for the trees, I believe. I do not doubt your _ability_ to write efficient code I do doubt whether in this specific instance you actually _did_ solve the issue at hand most efficiently. The assembly link is cute but really is is making my point. All of this:
```
" tst r3, #2
bxeq lr
ands r3, r0, #63
add r0, r0, r0, lsr #6
movne r3, #1
add r0, r3, r0
"
can be avoided for each packet… Not I know s is only one conditional (which a branch predictor will get perfectly right) and just 5 more instructions. Compare this to my replacement code at that function:
“
"
Yes right no conditional and no instruction. I would venture that code optimization is rarely easier. (Yes, yes, the rate adjustment needs to go somewhere, but it will be executed rarely enough to truly not matter, assuming there are fewer rate changes than packets send).
And we’re literally talking about four cycles (on the CPUs most used in consumer-grade routers) out of how many already incurred per packet? I have better places to spend my limited mental energy.
```
Well, that is a different argument to be had, not the one we are having right now. In essence we are where we are now, because (I assume) you initially modeled the PTM rate overhead analogous to ATM’s and failed to realize yourself that these are not that similar and that PTM has a much simpler solution than ATM. Now it appears you have fully “entrenched” yourself and are not willing to “budge” one bit (in a sense we are replaying Dr. Seuss’s “the Zax”).
The ironic thing from my perspective is that you keep digging; you made up the argument about it only costing one instruction (instead of the up to 7 as you have shown now) instead of arguing about the “ gratuitous” bit that was the core of my argument.
```
The reason will be displayed to describe this comment to others. Learn more.
On 4 Oct, 2016, at 16:20, moeller0 ***@***.*** wrote:
and are not willing to “budge” one bit
You’re right, I’m not budging. But I have good reason for it.
We’re both stubborn about the things we care about. In my case, I care about writing and maintaining this code. To do so, I already have to juggle an incredible number of things in my head. Adding various exceptions to abstractions I’ve already carved out increases that number.
As does having arguments on the internet about it all.
In this case, the relevant abstractions are:
1: The shaper operates on two values, a rate and a size. The rate is the physical link rate. Overhead compensation is therefore done entirely on the size.
2: All of the code which applies overhead compensation is in one place - one of the smaller functions in the whole codebase.
What you are suggesting would add exceptions to both of those abstractions:
1: The rate is the physical link rate _except for_ when PTM compensation is active, when it is 64/65 of the physical link rate. Overhead compensation is therefore done partially on both components of the shaper equation.
2: Some of the code which applies overhead compensation is in a different place from all the rest. Indeed, you would prefer it be in userspace, a whole other _domain_. This doesn’t just complicate _my_ life, but that of anyone coming along afterwards and wanting to port it to another OS - which is not a merely theoretical possibility - since they can’t rely on only the sch_cake module code any more.
You asked earlier whether I had justifications for my decision, and stated that you trusted that I had some. Well, there they are. Be happy.
In the meantime, I really _do_ have more important things to spend my time and mental capacity on. Some of them are more relevant to performance than this, such as making sure my new implementation of triple-isolate actually works the way I think it does. Some of them are also relevant to your particular interests, such as finding a way to excise the “via-ethernet” wart, and vastly improving the documentation. But I can’t do any of that efficiently if I keep getting interrupted by arguments such as this.
- Jonathan Morton
The reason will be displayed to describe this comment to others. Learn more.
Jonathan,
On Oct 4, 2016, at 16:11 , chromi ***@***.*** wrote:
> On 4 Oct, 2016, at 16:20, moeller0 ***@***.*** wrote:
>
> and are not willing to “budge” one bit
You’re right, I’m not budging. But I have good reason for it.
We’re both stubborn about the things we care about. In my case, I care about writing and maintaining this code. To do so, I already have to juggle an incredible number of things in my head. Adding various exceptions to abstractions I’ve already carved out increases that number.
As does having arguments on the internet about it all.
```
Then do not discuss if it distracts you. Look, I have realized that cake is your “sandbox” and you fight hard to make all important decisions yourself, I get that. I also realize that cake is promising a lot of what the typical sqm-scripts users want. I am trying hard to recommend cake to others, but your attitude and demonstrated lack of love to detail and honestly your occasionally abrasive discussion style makes it really hard for me to do so. Not your problem I realize, but mine.
```
In this case, the relevant abstractions are:
1: The shaper operates on two values, a rate and a size. The rate is the physical link rate. Overhead compensation is therefore done entirely on the size.
```
In most circumstances the rate is decidedly not the “physical link rate” but a (largish) fraction thereof. So this theoretical purity you propose simply does not seem to exist in real life (think downstream shapers for example, or DOCISIS where there is no real way for the user to access the physical link rate, not that that matters that much in a TDM system). Also a number of ISPs started to use user-opaque shapers/policers at the BRAS/BNG level so even if the user knows the DSL sync rate it will help much in correctly configuring overhead accounting (note with theses shpers/policers the 64/65 reduction issue becomes moot since the fixed shaper by necessity is set lower than physical link rate * 64/65). So if cake assumes the shaper rate to be the physical link rate something is wrong with cake’s idea about its typical use-case and reality. But I assume cake is quite fine.
So in essence, as I see it: (shaper) rate is the value where the user codes the bandwidth sacrifice to the latency gods while overhead needs to reflect the physical reality (at least as seen from the UI) so that shaper rate can be honored. We already established that the shaper will work whether one increases the accounted packet size or reduces the rate one shapes against OR BOTH. And that in turn makes it a pure implementation decision where to do the accounting/compensation.
```
2: All of the code which applies overhead compensation is in one place - one of the smaller functions in the whole codebase.
```
Well, all I pointed out that there are two reasonable arguments why PTM’s 64/64 encoding does not fit well into the current setup, sure you can force a square wedge into a round hole with enough force, but please do not claim that this is an efficient and natural fit.
```
What you are suggesting would add exceptions to both of those abstractions:
1: The rate is the physical link rate _except for_ when PTM compensation is active, when it is 64/65 of the physical link rate. Overhead compensation is therefore done partially on both components of the shaper equation.
```
No, the relevant distinction to make IMHO is the type of overhead, per-packet overhead needs to be accounted on a per packet fashion since there is no precise way to avoid that, while generic “rate” can be dealt with much more efficiently at the part of the code that deals with rates. I do not know about you, but I absolutely detest the things AAL5’s nasty integer cell count rule makes us do to compensate for the overhead, but we have no other option, but for PTM we do.
```
2: Some of the code which applies overhead compensation is in a different place from all the rest.
```
Not necessarily, you could put the new function just below the current one. Or rename cake_overhead into cake_per_packet_overhead and also add a cake_rate_overhead. Is it just me or are you just king these things up on the fly in a quest to retroactively justify the line you drew into the sand?
```
Indeed, you would prefer it be in userspace, a whole other _domain_.
```
Well, I proposed that this could live in tc. mostly to demonstrate how rarely this adjustment code needs to be executed. I also alluded to the fact that with your inbuilt-rate estimator tc might not be the correct place. To repeat my main point is this compensation mostly is once per runtime and performing it for each packet seems wasteful.
```
This doesn’t just complicate _my_ life, but that of anyone coming along afterwards and wanting to port it to another OS - which is not a merely theoretical possibility - since they can’t rely on only the sch_cake module code any more.
```
I get popcorn and put a six pack in the fridge to se you apply this kind of logic once you try to get cake into mainline Linux… Also people most likely will need a user space tool to configure cake in another OS it is not that ythe PTM overhead calculation is the only thing cake needs tc for, now ist it?
```
You asked earlier whether I had justifications for my decision, and stated that you trusted that I had some. Well, there they are. Be happy.
```
Okay, thank you. If you had given “code organization” as initial rationale I would have shut up, since I happily accept that cake is your baby. But the arguments you offered why your way is better in solving the PTM encoding issue than my proposal are pretty weak and I find it almost insulting you tried these on me.
```
In the meantime, I really _do_ have more important things to spend my time and mental capacity on. Some of them are more relevant to performance than this, such as making sure my new implementation of triple-isolate actually works the way I think it does.
Some of them are also relevant to your particular interests, such as finding a way to excise the “via-ethernet” wart, and vastly improving the documentation. But I can’t do any of that efficiently if I keep getting interrupted by arguments such as this.
```
Well, I guess I simply keep recommending VDSL2 users reduce their shaper bandwidth to at least 100*64/65 = 98.4% of the sync rate and be done with the issue (this does not even require a patch to tc and allows the user to appreciate the rounding error if calculated with a few decimal places).
```
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems rather unfortunate to incur one gratuitous division per packet. Since the 64/65 encoding is happening independent of data being send it seems much more efficient to adjust the shaper rate by multiplying with (64/65) and flooring that result, an operation which can be performed in the tc front-end and would not require (additional) per-packet processing. Since I believe I have brought this up as comment to the original PTM patch I wonder why you choose to solve this in this expensive fashion? Especially since for most users the shaper rate number will be larger than the packet size which will cause less rounding error...
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9f8a71There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.