HFSC Based Script

[zcecc22]

As discussed with Moeller, please find this pull request with my HFSC script.

I made some changes to functions.sh:

• Turned AUTOFLOW on. From my experience this improves performance.
• Added an ethtool section to setup the underlying interface.
• Split the function get_flows for use in the FLOW classifier which uses the "divisor" instead of "flows".
• Added missing modules.

[moeller0]

Hi Vincent, hi Toke,

On Jul 26, 2015, at 20:48 , Vincent Frentzel notifications@github.com wrote:

As discussed with Moeller, please find this pull request with my HFSC script.

I made some changes to functions.sh:

� Turned AUTOFLOW on. From my experience this improves performance.

Do you have any data to somehow quantify the improvement?

� Added an ethtool section to setup the underlying interface.

Great, I wanted to add something like this for some time now.

� Split the function get_flows for use in the FLOW classifier which uses the "divisor" instead of "flows�.

No opinon.

� Added missing modules.

I wonder whether we should actually list and load the modules explicitly for and in each qos script, sort of to document what is required. We also should give a reasonable error message if modules are not available, but both of these are independent of this merge, but rather a bit longer on my todo list already ;)

You can view, comment on, or merge this pull request online at:

#1

Commit Summary

� Added nxt_routed_hfsc.qos script.

We wrapped logger into sqm_logger which helps in getting better output to stdout and also works on non-openwrt distributions (thanks to Toke). Also I have a small request to make, this script does not contain any comments, it would be nice if at least the clever parts were highlighted ;)

� Added license information.
File Changes

� M src/defaults.sh (2)
� M src/functions.sh (75)
� A src/nxt_routed_hfsc.qos (146)
� A src/nxt_routed_hfsc.qos.help (2)
Patch Links:

� https://github.com/tohojo/sqm-scripts/pull/1.patch
� https://github.com/tohojo/sqm-scripts/pull/1.diff

I am all for pulling this and the fixing the logger/sqm_logger mismatch in sqm_scripts after merging.

Best Regards
Sebastian

�
Reply to this email directly or view it on GitHub.

[tohojo]

I am certainly fine with adding this feature, but there's a couple of issues with this particular pull request:

1. Lack of Signed-off-by entry in the main commit.
2. Whitespace issues: Please indent by one tab character, and make sure your editor does not expand the tabs into spaces.
3. The use of 'logger' vs 'sqm_logger' as Sebastian mentioned.
4. Different changes are bunched in one commit. It would be better to split them out into logically separate commits, each with a separate descriptive commit message and a Signed-off-by line:
   • One for the changes to functions.sh.
   • One for changing the default for flows.
   • One for adding the hfsc script.

If you could please fix these issues and open a new pull request, I'll be happy to merge it :)

[tohojo]

Realised that indentation was fairly inconsistent as it was, so probably shouldn't fault you for adding a bit more inconsistency in there. :)

Should now be consistent at four spaces of indent everywhere. Please use that when you resubmit :)

[zcecc22]

Thanks for the info. I definitely like 4 spaces more... Going through the code I realized that my pull request missed the fact that some cleanup logic has been added in the stop-sqm (iptables rule teardown). This is fine for the current qos scripts, however mine have different rules (I also have some scrips without any iptable rules for use on transparent bridges).

As a way forward, my idea was to provide the start-sqm and stop-sqm functions as interface functions of the qos scripts to allow script specific implementations. Would that be fine?

[moeller0]

Hi Toke,

On Jul 29, 2015, at 01:34 , Toke H�iland-J�rgensen notifications@github.com wrote:

Realised that indentation was fairly inconsistent as it was, so probably shouldn't fault you for adding a bit more inconsistency in there. :)

Should now be consistent at four spaces of indent everywhere. Please use that when you resubmit :)

Great, I believe having a HFSC script as part of the default sqm-scripts is going to be quite helpful for testing. @Vincent would you mind if I modded your script after merging to also handle link layer adjustments?
Also I apologize for inconsistent indentation so far, I just noticed my editor was set to �fake half tabs�, which is the worst of all worlds. I now switched to 4 spaces, even though I believe the proper indentation to be 1 tab and the tab width to be 8 characters (how strongly I believe that is obvious from the fact that I did not notice how baroque my indentation has been until now ;) )

Best Regards
Sebastian

�
Reply to this email directly or view it on GitHub.

[zcecc22]

On a separate point... I would like to propose that the selection of the qdisc be removed from UCI in favor of defining it statically in each qos script. The reasons are as follow:

1. This was implemented initially in CeroWrt to allow for experimentation. For OpenWrt I would argue that it makes more sense to provide optimized scripts for a particular Qdisc and save the end user the effort of finding the right one.
2. The CeroWrt experimental Qdiscs are not in the mainline kernel (i.e. nfq_codel, efq_codel, etc...) therefore options for playing are quite limited (fq_codel, pie if custom kernel, cake in the future).
3. There is an added complexity to be implemented in the qos scripts to allow switching between the different Qdisc which makes the script harder to maintain and test. Going back to 1) I dont think this complexity adds much benefit.

From an overall overview I think that future activities should focus on taking as much of the learnings from Cero and build a stable, rock solid product, out of sqm-scripts. This means cleaning up the qos scripts and providing a set of options to the user which solve actual problems (ie. not provide a base for experimentation on Qdiscs). I would also recommend to remove some of the qos scripts which we feel are not adding much user value.

[zcecc22]

Hi Seb,

No issue on post merge modding. Just let me get the right script in first as I really want to give something that we will work everywhere. I did some playing but was not so satisfied with the performance on some links.

[moeller0]

Hi Vincent, hi Toke

On Jul 29, 2015, at 10:24 , Vincent Frentzel notifications@github.com wrote:

On a separate point... I would like to propose that the selection of the qdisc be removed from UCI in favor of defining it statically in each qos script. The reasons are as follow:

Great minds think alike; Toke and I �solved� this yesterday ;), sqm-scripts will populate a directory with the set of leaf qdiscs that are a) available on a given system, and that b) are in the set sqm-scripts supports. No more ns2_codel unless you actually have that qdisc on your system...

1. This was implemented initially in CeroWrt to allow for experimentation. For OpenWrt I would argue that it makes more sense to provide optimized scripts for a particular Qdisc and save the end user the effort of finding the right one.

With this I disagree; I think we should retain the ability to change leaf qdiscs (not the first level shapers like htb, hfsc, and friends though) within reason (piece_of_cake for example only uses on cake instance as shaper and leaf, so it will ignore the qdisc variable and set it to cake). This allows to actually test the qdiscs against each other which is vital to catch regressions (and yes we cal ready caught regressions in htb due to this). That said I agree that our defaults should result in a generically well-working setup.

2. The CeroWrt experimental Qdiscs are not in the mainline kernel (i.e. nfq_codel, efq_codel, etc...) therefore options for playing are quite limited (fq_codel, pie if custom kernel, cake in the future).

I hope this is solved now, and i agree that was highly confusing for (casual) users.

3. There is an added complexity to be implemented in the qos scripts to allow switching between the different Qdisc which makes the script harder to maintain and test. Going back to 1) I dont think this complexity adds much benefit.

Well, we need this to be able to actually configure the detail parameters in the advanced options sections, unfortunately not all qdiscs have the same invocation for conceptually identical options, so we need some impedance matching.

From an overall overview I think that future activities should focus on taking as much of the learnings from Cero and build a stable, rock solid product, out of sqm-scripts. This means cleaning up the qos scripts and providing a set of options to the user which solve actual problems (ie. not provide a base for experimentation on Qdiscs).

I agree, the default configuration should do exactly that (and if you note that it fails short please holler so we can fix this); I also think we should not remove the advanced options (hiding as we do now should be sufficient, well maybe we could hide them better ;) ). Expecting normal users to modify scripts will just lead to less experimentation and testing, compared to the status quo where we/I can ask for specific inputs into the advanced fields�
Also some of the complexity is there because reality is complex ;) the link layer adaptation for example is hard to automatically do right as it requires information about a link that is hard to come by automatically and quickly. So the option is to keep it simple and just ignore the link layer (leading to abysmal worst case behavior on ATM links) or expose the complexity to some degree. I opted for the second option.

I would also recommend to remove some of the qos scripts which we feel are not adding much user value.

We currently only have 5 in sqm-scripts, two 1-tier scripts one htb and one cake (which I hope to be the recommended one in the future once cake lans in openwrt), two multi-tier scripts one htb and one cake, as well as one wip script. We could remove that one, but a) I really like to tackle the tc filter hash issue with one, one of those days, and b) it is nice having source control working for me ;) (but if Toke and you want simple_pppoe.qos gone, I will just take it private until I actually get around to use it).
One of the things I really like about Toke�s design of sqm-scripts is that you can just drip a new script in and use all the nice helpers we came up with. Even though as far as I know you are the only person actually using this capability I believe it is worth keeping; )

Best Regards & sorry to mostly disagree
Sebastian

�
Reply to this email directly or view it on GitHub.

[tohojo]

I agree that we should continue to provide a qdisc selector in the GUI. With the recent changes to only show the available qdiscs that should not be too confusing. Note, however, that the (new) verify_qdiscs function can take a list of supported qdiscs as the second parameter, and will error out if the selected qdisc is not in that set. So I think it's reasonable for individual scripts to error out if the selected qdisc is not supported. We may want to think about how to present that fact to the user.

As for the API to the .qos scripts, I agree that hard-coding a particular teardown mechanism is probably not a good idea. Expecting each .qos script to define sqm_start and sqm_stop (and maybe having a default implementation in functions.sh) would probably be a good way to solve that. I'll look into refactoring that :)

[tohojo]

Okay, pushed the API change.

[moeller0]

On Jul 29, 2015, at 15:01 , Toke H�iland-J�rgensen notifications@github.com wrote:

I agree that we should continue to provide a qdisc selector in the GUI. With the recent changes to only show the available qdiscs that should not be too confusing. Note, however, that the (new) verify_qdiscs function can take a list of supported qdiscs as the second parameter, and will error out if the selected qdisc is not in that set. So I think it's reasonable for individual scripts to error out if the selected qdisc is not supported. We may want to think about how to present that fact to the user.

My hands on proposal is to dump this information into the log and tell the user in the GUI to read the log ;)

Best Regards
Sebastian

As for the API to the .qos scripts, I agree that hard-coding a particular teardown mechanism is probably not a good idea. Expecting each .qos script to define sqm_start and sqm_stop (and maybe having a default implementation in functions.sh) would probably be a good way to solve that. I'll look into refactoring that :)

�
Reply to this email directly or view it on GitHub.

[moeller0]

Hi Toke,

On Jul 29, 2015, at 15:27 , Toke H�iland-J�rgensen notifications@github.com wrote:

Okay, pushed the API change.

As always looks quite clean and nice; it will increase the entrance burden a bit for people wanting to write new scripts; then again as far as I know the only one that did, Vincent, most likely can deal with this change better than I can ;)

Best Regards
Sebastian

�
Reply to this email directly or view it on GitHub.