Very Poor Multi-Threaded Performance of lib TCrypto Hashing Functions

[haxelion]

Hello,

We've been investigating performance issues when using the hashing function provided by the tcrypto library.
We have isolated this issue to the way the IPP cryptography primitives are being used: intel/cryptography-primitives#93

To take the specific example of SHA-256, when calling ippsHashMessage_rmf, ippsHashMethod_SHA256_TT is called every time:

https://github.com/intel/linux-sgx/blob/7385e10ce1106215d15f874a024ca224c7417eea/sdk/tlibcrypto/ipp/sgx_sha256_msg.cpp#L49-L68

This seems innocent enough as this code should just return a static structure with function pointers to the specific functions for that hashing primitive. However, the _TT methods support dynamic dispatching to the NI implementations of those hashing primitives: https://www.intel.com/content/www/us/en/docs/ipp-crypto/developer-guide-reference/2021-9/one-way-hash-primitives.html

Because of the way this was implemented, calling ippsHashMethod_SHA256_TT repeatedly, on a platform supporting SHA-NI, results in every call setting the method.hashUpdate global function pointer to the normal implementation function pointer and then to the NI one:

https://github.com/intel/cryptography-primitives/blob/59a3c2e80c8fccd0d37b7a58020671c5468ec49b/sources/ippcp/hash/sha256/pcphashmethod_sha256_tt.c#L49-L75

Since this structure is static and shared across all threads, calling this method from different threads causes the function pointer to keep changing for all threads involved with devastating consequences for the memory caches (on the pure ippcp sample without using SGX, we could see a massive memory bottleneck due to L1D and L3 cache misses using perf).

I'm unsure what is the correct fix for the libtcrypto functions. I have seen some internal code implementing CPU dispatching directly using ippcp internal functions: https://github.com/intel/linux-sgx/blob/7385e10ce1106215d15f874a024ca224c7417eea/sdk/tlibcrypto/ipp/ipp_disp/intel64/ippsHashMessage_rmf.c#L61-L76

For our use case, we will use ippcp directly, make sure to call ippsHashMethod_SHA256_TT only once and cache it.

[mikolajlubiak]

Hi,

I made an upstream intel/cryptography-primitives pull request with a fix: intel/cryptography-primitives#95

Have a nice day,
Mikołaj

[haxelion]

Hello Mikołaj,

Thank you very much for proposing a PR!
I'm very curious to know what Intel thinks about this issue but that PR is a step in the right direction.

Have a nice day!

[mikolajlubiak]

I'm very curious to know what Intel thinks about this issue but that PR is a step in the right direction.

I don't work at Intel, at least not yet ;) I had an interview and if they hire me I would be happy to work more on refactoring TCrypto.

[jbdelcuv]

I think that if Mikolaj's PR solves this performance issue, we would have to wait and integrate a newer version of the Cryptography Primitives Library when it becomes available.
Note that the dispatcher code mentioned above is generated with a tool that is part of the crypto library package, which is why we rather not modify it.

[haxelion]

@jbdelcuv, thank you very much for your answer.

Looking closer at the PR, Mikołaj PR actually only solve half of the issue (reduce the cache miss by half instead of eliminating it). This is because writing the same value over and over in the structure still causes cache lines to be invalidated. That's why Mikołaj only saw a 10% performance uplift while what we saw was very dramatic (see the IPP CP ticket, SGX performance were very similar). I can provide a minimal enclave demonstrating the issue inside SGX.

To solve the issue, the calls to *_TT (tick tock functions) need to be only done once or entirely eliminated from the TCrypto API.

I see two main solutions:

1. Implement that dispatcher code everywhere (maybe the tool can be tweaked to generate that dispatch code in TCrypto public API?)
2. Call *_TT function once when the library is initialized and populate a global structure which is then used by those public APIs.

We would be interested in contributing a fix but since it would require some efforts, we would like to make sure we agree on the solution first.

[jbdelcuv]

@haxelion, I agree with your assessment.
I read again your initial analysis as well as Mikolaj's PR and performance results and I understand why such update wouldn't address the issue.
As I mentioned earlier, I wouldn't attempt to update the tool that generates the dispatcher code. It appears to me it would have to handle hash functions in a especial way, which seems to be beyond what the tool is intended to do.
On the other hand, updating the tcrypto library seems a more straightforward route to me. As you suggested, calling the SHA1, SHA256, and SHA384 _TT functions in the library initialization routine, cache these global structures, and have the public APIs use them would avoid the issue in the first place.
I'd like to talk to other team members but your proposal makes sense to me. 😉