aesni(4): improve session lookup performance
AbandonedPublic
Actions

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

Details

Reviewers

jmg
jhb
emeric.poupon_stormshield.eu

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

emeric.poupon_stormshield.eu created this revision.May 11 2018, 1:35 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 11 2018, 1:35 PM

valentin.vergez_stormshield.eu added a subscriber: valentin.vergez_stormshield.eu.May 11 2018, 1:42 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
253	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.

emeric.poupon_stormshield.eu added inline comments.May 14 2018, 7:21 AM

crypto/aesni/aesni.c
253	Yes my bad!

Remarks

Remove bad comment

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

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.

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?

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.

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.