Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Avoid taking a write lock in RAND_get_rand_method() #20929

Closed
wants to merge 1 commit into from

Conversation

mattcaswell
Copy link
Member

The function RAND_get_rand_method() is called every time RAND_bytes() or RAND_priv_bytes() is called. We were obtaining a write lock in order to find the default random method - even though we rarely write. We change this to a read lock and only fallback to a write lock if we need to.

Partial fix for #20286

The function RAND_get_rand_method() is called every time RAND_bytes() or
RAND_priv_bytes() is called. We were obtaining a write lock in order to
find the default random method - even though we rarely write. We change
this to a read lock and only fallback to a write lock if we need to.

Partial fix for openssl#20286
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels May 10, 2023
@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels May 10, 2023
@hlandau hlandau removed the approval: otc review pending This pull request needs review by an OTC member label May 10, 2023
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 11, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 12, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@mattcaswell mattcaswell added the hold: need otc decision The OTC needs to make a decision label May 12, 2023
@arapov arapov linked an issue May 22, 2023 that may be closed by this pull request
@mattcaswell
Copy link
Member Author

mattcaswell commented May 23, 2023

I have run some performance tests for this using the randbytes performance test app in openssl/tools#146.

This basically runs RAND_bytes() repeatedly in a loop for varying numbers of threads (1, 10, 100, 500, 1000) across different OpenSSL versions. Versions tested were 1.1.1, 3.0, 3.1, master, master + #20929 (i.e. this PR), master + #20970, master + #20929 + #20970.

The numbers are timings in us for a set of 100 RAND_bytes calls. Smaller numbers are better.

1 10 100 500 1000
1.1.1 154.7 27.999 47.5211 80.53202 79.96716
3.0 108.62 40.443 43.4487 365.16734 438.25929
3.1 133.35 37.101 70.9483 242.17826 292.9062
master 83.16 37.68 205.1193 253.5632 295.03424
master + #20929 136.6 41.323 42.1758 244.75912 279.15844
master + #20970 123.34 30.259 43.5035 194.67536 181.63062
master + #20929 + #20970 121.16 21.57 10.848 11.13552 10.81915

There are some anomalies in here. The timings for 100 threads in 3.1 and master seem unexpectedly high.

Having master with just this PR shows an improvement but still not to 1.1.1 levels. Having master and #20970 also show slightly better and marked improvement. Having both this PR and #20970 seems to have a dramatic improvement (better than 1.1.1 even).

@beldmit
Copy link
Member

beldmit commented May 23, 2023

I have no idea how 10 concurrent threads can give better timing than 1. Could you please explain?

@t8m
Copy link
Member

t8m commented May 24, 2023

I have no idea how 10 concurrent threads can give better timing than 1. Could you please explain?

If you have more CPU cores they definitely can.

@mattcaswell
Copy link
Member Author

mattcaswell commented May 24, 2023

I have no idea how 10 concurrent threads can give better timing than 1. Could you please explain?

The timings are calculated based on the total time taken divided by the number of RAND_bytes() calls done. Each thread adds a fixed number of RAND_bytes() calls to be done. Based on the number of cores in the machine with 10 threads you can get more RAND_bytes() calls done completely in parallel to each other - so the total time doesn't go up by much, but the number of RAND_bytes() calls made goes up. Once you exceed the core count in the machine that effect goes away. At least that is my guess as to what is happening.

@t8m
Copy link
Member

t8m commented May 30, 2023

OTC: as there is little benefit without #20970, master only.

@t8m t8m removed hold: need otc decision The OTC needs to make a decision branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels May 30, 2023
@mattcaswell
Copy link
Member Author

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request May 30, 2023
The function RAND_get_rand_method() is called every time RAND_bytes() or
RAND_priv_bytes() is called. We were obtaining a write lock in order to
find the default random method - even though we rarely write. We change
this to a read lock and only fallback to a write lock if we need to.

Partial fix for #20286

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20929)
MrE-Fog pushed a commit to MrE-Fog/opensslxxixx that referenced this pull request Jun 4, 2023
The function RAND_get_rand_method() is called every time RAND_bytes() or
RAND_priv_bytes() is called. We were obtaining a write lock in order to
find the default random method - even though we rarely write. We change
this to a read lock and only fallback to a write lock if we need to.

Partial fix for #20286

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#20929)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.0 performance degraded due to locking
6 participants