Page MenuHomeFreeBSD

aesni(4): improve session lookup performance
AbandonedPublic

Authored by cem on May 11 2018, 1:35 PM.
Referenced Files
Unknown Object (File)
Dec 20 2023, 1:16 AM
Unknown Object (File)
Dec 13 2023, 9:22 AM
Unknown Object (File)
Oct 4 2023, 2:18 AM
Unknown Object (File)
Sep 24 2023, 9:52 AM
Unknown Object (File)
Jul 2 2023, 8:37 AM
Unknown Object (File)
Jun 17 2023, 5:54 PM
Unknown Object (File)
May 13 2023, 3:52 AM
Unknown Object (File)
Apr 22 2023, 4:13 AM

Details

Summary

Replace the TAILQ used to store the aesni sessions with a RB tree.
The queue is now only used to store free sessions.

Each time aesni_process is called, we potentially iterate through the whole session list in order to find the session pointer.
It is OK when using few sessions, but it is a performance killer when dealing with thousands of sessions, and that is the case with big IPSec configurations.

Test Plan

Using an IPSEC SAD with 2k entries, I have a performance drop up to 50% with IPSec AES-GCM-16.
With this patch, I see no noticeable performance change.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

I agree that the current system sucks and that it does not scale with multiple sessions. I'm not sure a binary tree is the right replacement, though. And probably session management should be lifted out into OCF (OpenCrypto Framework) so that all drivers can benefit from it without copy-pasting.

OCF has a number of shortcomings which John detailed here: https://lists.freebsd.org/pipermail/freebsd-arch/2018-January/018835.html

crypto/aesni/aesni.c
252

This replacement is wrong. It can result in use-after-free. The function must abort like it did before if there are any entries in sc->sessions (i.e., any active sessions). It cannot free active sessions.

In D15389#324522, @cem wrote:

I agree that the current system sucks and that it does not scale with multiple sessions. I'm not sure a binary tree is the right replacement, though. And probably session management should be lifted out into OCF (OpenCrypto Framework) so that all drivers can benefit from it without copy-pasting.

OCF has a number of shortcomings which John detailed here: https://lists.freebsd.org/pipermail/freebsd-arch/2018-January/018835.html

I do agree that a binary tree is not an optimal choice and the session management should be handled in OCF.
However, I think this patch still makes sense since the replacement is quite trivial and does not prevent any further refactoring. It is definitely better than a tail queue.

crypto/aesni/aesni.c
252

Yes my bad!

Nobody wants this? It is definitely better than the current code though.

Nobody wants this? It is definitely better than the current code though.

Sure, it improves performance (I haven't tested, but I'm quite willing to take your word for it) of a single OCF driver in the case that some consumer is allocating a lot of sessions (or maybe a reasonable number of sessions on a high core-count CPU). But that doesn't make it definitely better.

It adds complexity and perpetuates the problem that every OCF driver needs to copy and paste crappy session management. I think a general solution that moves the session management into OCF and out of drivers is a better way to fix this. You don't need the RB tree if sessions are just pointers.

In D15389#328341, @cem wrote:

Nobody wants this? It is definitely better than the current code though.

Sure, it improves performance (I haven't tested, but I'm quite willing to take your word for it) of a single OCF driver in the case that some consumer is allocating a lot of sessions (or maybe a reasonable number of sessions on a high core-count CPU). But that doesn't make it definitely better.

It adds complexity and perpetuates the problem that every OCF driver needs to copy and paste crappy session management. I think a general solution that moves the session management into OCF and out of drivers is a better way to fix this. You don't need the RB tree if sessions are just pointers.

So what is the plan here? Wait for a couple of months/years that somebody is willing to change the session management in crypto(4) and all crypto drivers?
And in the meanwhile suffer from poor performance when you use a lot of IPSec tunnels at the same time?

So what is the plan here? Wait for somebody willing to do the session management in OCF right?

Yes.

I agree w/ cem that we need a general solution to this. I'm fine w/ this patch, but I do question the wisdom in making this code aesni only. This code really needs to be turned into a library which any OCF driver can use, as this performance problem is not limited to aesni.

crypto/aesni/aesni.h
71

comments would be nice to know what the two uses of these are. Yes, you commented in the review, but people aren't going to dig into a review to understand this code.

cem commandeered this revision.EditedJul 11 2018, 10:50 PM
cem edited reviewers, added: emeric.poupon_stormshield.eu; removed: cem.