Page MenuHomeFreeBSD

aesni(4): improve session lookup performance
AbandonedPublic

Authored by cem on May 11 2018, 1:35 PM.

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
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

cem added a reviewer: jhb.May 11 2018, 4:09 PM

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!

emeric.poupon_stormshield.eu marked 2 inline comments as done.

Remarks

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

cem added a comment.May 24 2018, 3:03 PM

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?

cem added a comment.May 25 2018, 7:21 AM

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

Yes.

jmg added a comment.May 27 2018, 9:25 PM

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.
cem abandoned this revision.Jul 11 2018, 10:50 PM