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:

linux-sgx/sdk/tlibcrypto/ipp/sgx_sha256_msg.cpp

Lines 49 to 68 in 7385e10

	sgx_status_t sgx_sha256_msg(const uint8_t *p_src, uint32_t src_len, sgx_sha256_hash_t *p_hash)
	{
	if ((p_src == NULL) || (p_hash == NULL))
	{
	return SGX_ERROR_INVALID_PARAMETER;
	}
	fips_self_test_hash256();
	IppStatus ipp_ret = ippStsNoErr;
	ipp_ret = ippsHashMessage_rmf((const Ipp8u *) p_src, src_len, (Ipp8u *)p_hash, ippsHashMethod_SHA256_TT());
	switch (ipp_ret)
	{
	case ippStsNoErr: return SGX_SUCCESS;
	case ippStsMemAllocErr: return SGX_ERROR_OUT_OF_MEMORY;
	case ippStsNullPtrErr:
	case ippStsLengthErr: return SGX_ERROR_INVALID_PARAMETER;
	default: return SGX_ERROR_UNEXPECTED;
	}
	}

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:

linux-sgx/sdk/tlibcrypto/ipp/ipp_disp/intel64/ippsHashMessage_rmf.c

Lines 61 to 76 in 7385e10

	IPPFUN(IppStatus, sgx_disp_ippsHashMessage_rmf,(const Ipp8u* pMsg, int len, Ipp8u* pMD, const IppsHashMethod* pMethod))
	{
	Ipp64u _features;
	_features = ippcpGetEnabledCpuFeatures();
	if( AVX3I_FEATURES == ( _features & AVX3I_FEATURES )) {
	return k1_ippsHashMessage_rmf( pMsg, len, pMD, pMethod );
	} else
	if( ippCPUID_AVX2 == ( _features & ippCPUID_AVX2 )) {
	return l9_ippsHashMessage_rmf( pMsg, len, pMD, pMethod );
	} else
	if( ippCPUID_SSE42 == ( _features & ippCPUID_SSE42 )) {
	return y8_ippsHashMessage_rmf( pMsg, len, pMD, pMethod );
	} else
	return ippStsCpuNotSupportedErr;
	}

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. 😉