Skip to content
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

WIP: Libipsec improvements #1284

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Thermi
Copy link
Contributor

@Thermi Thermi commented Sep 7, 2022

This branch contains some of the changes that Martin Willi did for libipsec to constrain the queue size of libipsec with a codel queue, as well as some changes from me to fix types, a small linking issue, and implementation of acquires for libipsec.

TODO:

  • bring Android stuff up to par for codel-queue
  • (optional) use Tobias' code (?)
  • rate limit acquires
  • performance test
  • integrate/rewrite codel queue for RFC 8289/RFC 8290
  • squash commits where relevant
  • (final) code review

@Thermi Thermi changed the title Libipsec improvements WIP: Libipsec improvements Sep 8, 2022
@tobiasbrunner
Copy link
Member

Regarding the CoDel queue: I've asked Martin and as far as he remembers, he did some (successful) experimenting at the time, but never any full blown tests (stress or otherwise) or formal measurements. Did you do any kind of testing/benchmarking? The patches would also need some reviewing. And it might be interesting to check RFC 8289 to see if anything could be improved (as a later improvement, RFC 8290 could be an option, but that would probably require quite some work).

Regarding the acquires: I've implement asynchronous relaying of acquires from libipsec to the daemon a long time ago (libipsec-trap branch). What's missing from that is a call in the processor. But I've wanted to implement some rate limiting first, so we don't trigger an acquire for every matching packet. I think whoever was interested in this at the time disappeared, so this was never completed.

@Thermi
Copy link
Contributor Author

Thermi commented Sep 16, 2022

I did not do any performance comparisons simply because the overhead caused by the crypto is so absurdly high in comparison. There are plans in the making to improve the performance generally and also make it more efficient but up to now having it simply not break in corner cases is the better way forward right now.

Yes, the patches need some review too. But the general acceptance is more important right now than the exact implementation.

I used your implementation I think as basis. I did not implement any rate limiting yet. I could of course do something like the kernel does. That should be a good enough example implementation to basically copy from.

There might be more improvements coming, I intend to get them all into upstream here.

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.

None yet

3 participants