Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add full-cycle speed test #1391

Merged
merged 6 commits into from
Feb 17, 2023
Merged

Add full-cycle speed test #1391

merged 6 commits into from
Feb 17, 2023

Conversation

baentsch
Copy link
Member

@baentsch baentsch commented Feb 16, 2023

Running ./tests/speed_sig Falcon-1024 with this new code reliably reproduces #1390 and could unearth similar such errors in the future -- when now added to CI: Feedback as to whether we should do this welcome: Would complete this PR with KEM-equivalent and github CI tests then.

  • [no] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • [no] Does this PR change the the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@baentsch baentsch mentioned this pull request Feb 16, 2023
@baentsch baentsch marked this pull request as ready for review February 16, 2023 12:52
@baentsch
Copy link
Member Author

Failures mostly "as expected" (triggered by the Falcon-1024 bug). @Martyrshot : Can you see a reason why this code SIGSEGVs on the ARM emulator? It works perfectly on the M1 (??).

@baentsch baentsch requested a review from Martyrshot February 16, 2023 13:02
@Martyrshot
Copy link
Member

My first thought is that maybe an optimized implementation is being used when it shouldn't be. I wonder if the new falcon implementation checks feature support differently than the previous version did.

@baentsch
Copy link
Member Author

@Martyrshot Sorry, I wasn't clear in my question: It's only the new "fullcycletest" (keygen-sign-verify) that fails on ARM emulator -- and that for all algorithms (see test log), not just falcon.

@Martyrshot
Copy link
Member

@baentsch Can I get access to that environment? Do these happen only when fullcycle is true? Or do these illegal instructions happen regardless?

@baentsch
Copy link
Member Author

baentsch commented Feb 16, 2023

@baentsch Can I get access to that environment?

Most definitely: It's "simply" a set of docker images. All code is visible here. I'm right now trying to reproduce myself:
Edit/Add: First results: After further simplifying things (now also building the code in the armhf emulation container docker run --rm -v pwd:/projects/liboqs -it openquantumsafe/ci-debian-buster-armhf:latest /bin/bash), it is definitely the code generated for "speed testing" that causes the problem: KAT tests work just fine:

root@456df10ca3d1:/projects/liboqs/build# ./tests/speed_kem Kyber768
Configuration info
==================
Target platform:  armv7l-Linux-5.16.14-051614-generic
Compiler:         gcc (8.3.0)
Compile options:  [-Wa,--noexecstack;-O3;-fomit-frame-pointer;-fdata-sections;-ffunction-sections;-Wl,--gc-sections;-Wbad-function-cast]
OQS version:      0.8.0-dev
Git commit:       b61585893dbc9147e4647ac6335ad91212ff40cc
OpenSSL enabled:  Yes (OpenSSL 1.1.1d  10 Sep 2019)
AES:              OpenSSL
SHA-2:            OpenSSL
SHA-3:            C
OQS build flags:  OQS_OPT_TARGET=auto CMAKE_BUILD_TYPE=Release 
CPU exts compile-time: 

Speed test
==========
Started at 2023-02-16 16:20:17
Operation                            | Iterations | Total time (s) | Time (us): mean | pop. stdev | CPU cycles: mean          | pop. stdev
------------------------------------ | ----------:| --------------:| ---------------:| ----------:| -------------------------:| ----------:
Kyber768                             |            |                |                 |            |                           |           
Illegal instruction (core dumped)
root@456df10ca3d1:/projects/liboqs/build# arch
armv7l

Ideas welcome...

@thomwiggers
Copy link
Contributor

See also PQClean/PQClean#477

@Martyrshot
Copy link
Member

So I guess the first thing to point out is the emulator is using armv7, where as the m1 is armv8. I wonder if there is some data width issues somewhere that doesn't happen on armv8 since it's 64 bit, but does happen on armv7 because it's 32 bit?

@baentsch baentsch force-pushed the mb-fullcyclespeedtest branch from 2846b16 to 9c159b0 Compare February 17, 2023 06:45
@baentsch
Copy link
Member Author

if there is some data width issues somewhere

Maybe -- but definitely not in the code added as part of this PR: Just confirmed that the speed_kem/sig programs always crash on this platform -- in other words, it seems like repeated API calls fail there. Created #1393 to track -- in case anyone has a keen interest in that old platform.

@baentsch baentsch merged commit 20fadb8 into main Feb 17, 2023
@baentsch baentsch deleted the mb-fullcyclespeedtest branch February 17, 2023 09:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants