Skip to content

fix: avoid resetting method.hashUpdate#95

Closed
mikolajlubiak wants to merge 2 commits intointel:developfrom
mikolajlubiak:fix/resetting-hashUpdate-pointer
Closed

fix: avoid resetting method.hashUpdate#95
mikolajlubiak wants to merge 2 commits intointel:developfrom
mikolajlubiak:fix/resetting-hashUpdate-pointer

Conversation

@mikolajlubiak
Copy link

@mikolajlubiak mikolajlubiak commented Mar 3, 2025

This is done to avoid setting the variable twice, because in a multithreaded environment this destroys memory caches. Note that to make the code cleaner I used an early return in the special sha256_ni_hashUpdate case.

Compilation using the Intel® oneAPI DPC++/C++ compiler succeeded.
Code formatted according to the .clang-format rules.
Added a note in change log and incremented the version.

Fixes intel/confidential-computing.sgx#1073

@mikolajlubiak
fix: avoid resetting method.hashUpdate
7063284 
This is done to avoid setting the variable twice, because in a
multithreaded environment this destroys memory caches.
Note that to make the code cleaner I used an early return in the special `sha256_ni_hashUpdate` case.

Fixes intel/confidential-computing.sgx#1073

Signed-off-by: Mikołaj Lubiak <[email protected]>
@mikolajlubiak
Copy link
Author

mikolajlubiak commented Mar 3, 2025

To cite the issue:

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

@mikolajlubiak mikolajlubiak mentioned this pull request Mar 3, 2025
Open
@mikolajlubiak
Copy link
Author

mikolajlubiak commented Mar 4, 2025 •
edited
Loading

Did a couple of benchmark runs, and the new code saves about 1,000,000,000 cache references, 10 CPU seconds (10% decrease), and halves the instruction count, in the specific benchmark case. Numbers hold roughly the same proportions in all runs.
With fix:

❯ sudo perf stat -B -e cache-references,cache-misses,cycles,instructions,branches,faults,migrations ./benchmark 12 10000000
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 5652985402 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 5654593436 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6133624034 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6137065569 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6217676274 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6218518712 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6277306025 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6278381844 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6356617497 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6360215651 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6448754165 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6455433524 ns

 Performance counter stats for './benchmark 12 10000000':

     2,539,162,050      cache-references
            97,644      cache-misses                     #    0.00% of all cache refs
   303,215,955,461      cycles
    56,939,106,874      instructions                     #    0.19  insn per cycle
     5,594,461,691      branches
               162      faults
                 9      migrations

       6.457206156 seconds time elapsed

      73.972820000 seconds user
       0.005963000 seconds sys

Without fix:

❯ sudo perf stat -B -e cache-references,cache-misses,cycles,instructions,branches,faults,migrations ./benchmark 12 10000000
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6324792106 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6328368805 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6948123032 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6972823042 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6989185139 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7003213614 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7044943411 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7054015430 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7101390258 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7104537262 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7106029344 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7121662468 ns

 Performance counter stats for './benchmark 12 10000000':

     3,727,422,918      cache-references
           120,280      cache-misses                     #    0.00% of all cache refs
   339,299,104,432      cycles
   110,108,473,947      instructions                     #    0.32  insn per cycle
     5,839,178,741      branches
               164      faults
                10      migrations

       7.124216913 seconds time elapsed

      82.828447000 seconds user
       0.005983000 seconds sys

Benchmark code: https://gist.github.com/mikolajlubiak/4d4c2f3286c03455f0aca256300aa3fe (from haxelion)

@mikolajlubiak
Copy link
Author

mikolajlubiak commented Mar 4, 2025

Some other stats with the same benchmark:

With fix:

❯ sudo perf stat -B -e LLC-loads,LLC-load-misses,LLC-stores,LLC-store-misses ./benchmark 12 10000000
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6149230421 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6153150286 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6195275445 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6204341769 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6204890586 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6208547958 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6266405663 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6269321465 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6269740576 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6274113901 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6402036998 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6408649862 ns

 Performance counter stats for './benchmark 12 10000000':

       289,679,436      LLC-loads                                                               (50.00%)
            17,378      LLC-load-misses                  #    0.01% of all LL-cache accesses    (49.99%)
       256,823,565      LLC-stores                                                              (50.01%)
             2,493      LLC-store-misses                                                        (50.01%)

       6.410538773 seconds time elapsed

      74.650541000 seconds user
       0.000993000 seconds sys

Without fix:

❯ sudo perf stat -B -e LLC-loads,LLC-load-misses,LLC-stores,LLC-store-misses ./benchmark 12 10000000
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6249371647 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6254551471 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6717360964 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6739020889 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6950421995 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 6963391824 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7013288374 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7060327093 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7068903938 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7072578410 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7175201365 ns
SHA256 result: 6b4ab10d92373474a97d10639e667f6b734af012f9be29997f1befd1c7166199 in 7182094432 ns

 Performance counter stats for './benchmark 12 10000000':

       467,841,050      LLC-loads                                                               (50.00%)
            18,428      LLC-load-misses                  #    0.00% of all LL-cache accesses    (49.99%)
       275,681,401      LLC-stores                                                              (50.01%)
             2,539      LLC-store-misses                                                        (50.01%)

       7.183901829 seconds time elapsed

      82.007108000 seconds user
       0.014439000 seconds sys

@haxelion
Copy link

haxelion commented Mar 11, 2025

@mikolajlubiak sorry I was AFK last week.
Thank you very much for looking into it and making this PR!

It is unfortunate that the IPP CP API and the TCrypto API doesn't mesh well and forces us to call the ippsHashMethod_SHA256_TT function each time but the only way around it would be a deeper refactoring of TCrypto.

I think that change can be broadly applied to all ippsHashMethod_SHA*_TT functions since they would all contain the same dynamic switching to the SHA-NI implementation.

@mikolajlubiak
Copy link
Author

mikolajlubiak commented Mar 11, 2025

It is unfortunate that the IPP CP API and the TCrypto API doesn't mesh well and forces us to call the ippsHashMethod_SHA256_TT function each time but the only way around it would be a deeper refactoring of TCrypto.

Yes, although this PR improves the performance a bit its still really poor.

I think that change can be broadly applied to all ippsHashMethod_SHA*_TT functions since they would all contain the same dynamic switching to the SHA-NI implementation.

You are right. I wanted to do that, but unfortunately had no free time.

@stanciuadrian
Copy link
Contributor

stanciuadrian commented Mar 11, 2025

Thanks @mikolajlubiak and @haxelion for your valuable contribution and feedback.
The performance results are really useful to understand what you see on your side.

We'll check it internally and get back to you with an answer.

@iMartyan
Copy link
Contributor

iMartyan commented Dec 10, 2025

Hello everyone,
Thank you for your patience regarding this issue. We have developed a fix that addresses the reported problem, which you can review here.

Summary of the fix: We've implemented a static volatile variable that, combined with compiler optimizations, prevents most re-initializations from occurring. However, we've identified that a race condition remains possible during the first initialization in multithreaded environments. While all operations are atomic, this makes repeated calls to the method functions potentially unsafe.

Recommendation: To ensure thread safety and optimal performance, we strongly recommend caching and reusing the returned pointer rather than making repeated function calls.

We would appreciate your feedback on this approach. Please review the implementation and let us know if you have any concerns before we proceed with closing this PR.

@mikolajlubiak
Copy link
Author

mikolajlubiak commented Jan 2, 2026

Hello everyone, Thank you for your patience regarding this issue. We have developed a fix that addresses the reported problem, which you can review here.

Summary of the fix: We've implemented a static volatile variable that, combined with compiler optimizations, prevents most re-initializations from occurring. However, we've identified that a race condition remains possible during the first initialization in multithreaded environments. While all operations are atomic, this makes repeated calls to the method functions potentially unsafe.

Recommendation: To ensure thread safety and optimal performance, we strongly recommend caching and reusing the returned pointer rather than making repeated function calls.

We would appreciate your feedback on this approach. Please review the implementation and let us know if you have any concerns before we proceed with closing this PR.

Hi @iMartyan,

The fix looks good. Mine only got rid of cache invalidation while yours actually fixed the multithreading issue.

I have a couple of questions though.

  1. CP_PREVENT_REORDER() on non x86 platforms is just a compiler barrier? Is that enough for actual thread safety, or does IPP-Crypto only run on x86?
  2. Can IsFeatureEnabled() ever change during runtime? If CPU features can't change, couldn't we just do the if/else once and set hashUpdate to the right value from the start?
  3. Do you know why my approach didn't fix the multithreading issue? I don't quite understand why it didn't fix the multithreading issue as well.

Thanks in advance,
Mikołaj

@iMartyan
Copy link
Contributor

iMartyan commented Jan 5, 2026 •
edited
Loading

Hi @mikolajlubiak,

Please find answers below:

  1. Yes, this macro is just a compiler barrier to prevent reordering of instructions. Without that compiler could change the logic of isInitialized. Still, it does not provide a complete thread safety
  2. Library provides functionality to manually set used CPU features, so yes, they might change during runtime. You may refer to SetCpuFeatures, this might be useful for testing purposes.
  3. In you approach function pointers might be set simultaneously by different threads. While on most systems this operation is atomic, it is still a race condition and cannot be considered thread safe. While current fix doesn't completely cover this issue, now this might happen only when several threads start to initialize the structure, later it is protected with isInitialized logic.

Thanks again for looking into this matter, we really do appreciate it!

Best regards,
Ivan

@paveldyakov
Copy link
Contributor

paveldyakov commented Jan 13, 2026

Hi @mikolajlubiak

According to our CONTRIBUTING.md guide, I am closing the current PR as the changes has been introduced in the library.

Thank you for your contribution!

Please let us know if you have any more questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Very Poor Multi-Threaded Performance of lib TCrypto Hashing Functions

5 participants

@mikolajlubiak @haxelion @stanciuadrian @iMartyan @paveldyakov