Skip to content

Do not clear bulk markings when down-prioritizing #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 28, 2024
Merged

Do not clear bulk markings when down-prioritizing #17

merged 4 commits into from
Nov 28, 2024

Conversation

brada4
Copy link
Contributor

@brada4 brada4 commented Nov 19, 2024

first 500ms wrongly upgrades already bulk tcp connections

simplistic filter would be

ip dscp lt cs4 ip dscp ne cs1

optimize that to range with single payload extract

since we are extracting payload anyway also avoid forcing unchanged value on packets.

Comment text says "downprioritize", where in fact it does reverse cs1->cs0

simple code would be
  ip dscp lt cs4 ip dsccp ne cs1
@brada4
Copy link
Contributor Author

brada4 commented Nov 19, 2024

Constants used.

nft describe ip dscp
payload expression, datatype dscp (Differentiated Services Code Point) (basetype integer), 6 bits

pre-defined symbolic constants (in hexadecimal):
        cs0                             0x00
        cs1                             0x08
...
        cs4                             0x20
... 

@hudra0
Copy link
Owner

hudra0 commented Nov 21, 2024

You PR make sense, but:

While the hex range approach {0x01-0x07, 0x09-0x1f} should work correctly, I believe the initial PR using ip dscp < cs4 ip dscp != cs1 is more maintainable and self-documenting. Using symbolic names (cs4, cs1) clearly shows the intention of not upgrading bulk traffic, and makes future adjustments easier. Though the hex ranges might be marginally more efficient, the readability and maintainability benefits of the symbolic approach outweigh this small performance difference. Would you be open to reverting to the symbolic version?

@brada4
Copy link
Contributor Author

brada4 commented Nov 22, 2024

100% agree, go for it ;-)

@brada4 brada4 closed this Nov 22, 2024
@brada4
Copy link
Contributor Author

brada4 commented Nov 22, 2024

Edit: it is just 1st part needing ip dscp != cs1 added.

@brada4 brada4 reopened this Nov 22, 2024
Make rules intent more obvious
@brada4
Copy link
Contributor Author

brada4 commented Nov 22, 2024

@hudra0 ok , done as advised.

@brada4
Copy link
Contributor Author

brada4 commented Nov 22, 2024

Please squash PR as it has mess from my broken fork.

@hudra0 hudra0 merged commit 124fc46 into hudra0:main Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants