aesni(4): improve session lookup performance

[cem]

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.

[cem]

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.

[emeric.poupon_stormshield.eu]

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.

[emeric.poupon_stormshield.eu]

crypto/aesni/aesni.c

252

Yes my bad!

[emeric.poupon_stormshield.eu]

Remarks

[emeric.poupon_stormshield.eu]

Remove bad comment

[emeric.poupon_stormshield.eu]

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

[cem]

In D15389#328317, @emeric.poupon_stormshield.eu 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.

[emeric.poupon_stormshield.eu]

In D15389#328341, @cem wrote:

In D15389#328317, @emeric.poupon_stormshield.eu 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?

[cem]

In D15389#328653, @emeric.poupon_stormshield.eu wrote:

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

Yes.

[jmg]

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]

https://reviews.freebsd.org/D16215 (+ https://reviews.freebsd.org/D16216 for IPsec and https://reviews.freebsd.org/D16219 for aesni(4) ) solves the problem generally.