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

Osx threadfix #23967

Closed
wants to merge 2 commits into from
Closed

Osx threadfix #23967

wants to merge 2 commits into from

Conversation

nhorman
Copy link
Contributor

@nhorman nhorman commented Mar 25, 2024

test build for debugging, as local operation on the development m1 doesn't reproduce failure

@nhorman nhorman marked this pull request as draft March 25, 2024 13:28
@nhorman nhorman marked this pull request as ready for review March 25, 2024 18:32
@nhorman nhorman added the 3.3.0 Planned eng view Include on 3.3.0 Engineering View Roadmap label Mar 25, 2024
@nhorman nhorman self-assigned this Mar 25, 2024
@nhorman nhorman requested review from mattcaswell and a team March 25, 2024 18:33
@nhorman nhorman added the severity: urgent Fixes an urgent issue (exempt from 24h grace period) label Mar 25, 2024
@slontis
Copy link
Member

slontis commented Mar 25, 2024

Is there some way of limiting the number of writes instead of doing this?

@nhorman
Copy link
Contributor Author

nhorman commented Mar 25, 2024

sure, but that runs a bit counter to the purpose of the test, they idea (in part) was to not only test the sanity of rw and rcu locks, but to provide a level of performance comparison.

What we could do is, instead of wrapping, when we hit MAX_ULLONG, just leave the counter there so that we never tripped over the wrapping case. Would that be acceptable?

@t-j-h
Copy link
Member

t-j-h commented Mar 26, 2024

If you are running over the end of the unsigned long long then you need to implement a "carry" into another variable (perhaps even another unsigned long long) each time you reach the wrapping point. That does sound like overkill for this context - but I also find the implementation here more than a little strange.

@t8m
Copy link
Member

t8m commented Mar 26, 2024

@nhorman I do not believe a wrap around 2^64 bit value is happening here. Or, the starting point was not a zero value. If the test run for 1000 seconds (which it is not running) it would mean doing 18 milions iterations in every nanosecond - that's obviously impossible. There must be some other issue in play.

@t8m t8m added triaged: bug The issue/pr is/fixes a bug branch: master Merge to master branch branch: 3.3 Merge to openssl-3.3 labels Mar 26, 2024
@kroeckx
Copy link
Member

kroeckx commented Mar 26, 2024

If we're worried for a wrap around at 2^64, why not clear the first bit?

Stochastic failures in the RCU test on MACOSX are occuring.  Due to beta
release, disabling this test on MACOSX until post 3.3 release
test/threadstest.c Outdated Show resolved Hide resolved
@nhorman
Copy link
Contributor Author

nhorman commented Mar 26, 2024

Based on OTC discussion, for the purposes of the beta release, since we are not yet using rcu, I've updated this PR to disable the rcu torture test on macosx. Will re-enable prior to introduction of usage post release

@t8m t8m added approval: review pending This pull request needs review by a committer tests: exempted The PR is exempt from requirements for testing labels Mar 26, 2024
@t8m
Copy link
Member

t8m commented Mar 26, 2024

OK with urgent

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I have one minor comment - but approved either way.

Agree urgent.

@@ -292,7 +293,6 @@ static int writer2_iterations = 0;
static uint64_t *writer_ptr = NULL;
static uint64_t global_ctr = 0;
static int rcu_torture_result = 1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This blank line removal looks odd. In my mind it is better with it.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels Mar 26, 2024
@mattcaswell
Copy link
Member

Have we raised an issue to remember to come back and fix this? If so what is the number?

@nhorman
Copy link
Contributor Author

nhorman commented Mar 26, 2024

merged to master and 3.3

@nhorman nhorman closed this Mar 26, 2024
openssl-machine pushed a commit that referenced this pull request Mar 26, 2024
Stochastic failures in the RCU test on MACOSX are occuring.  Due to beta
release, disabling this test on MACOSX until post 3.3 release

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23967)

(cherry picked from commit 1967539)
openssl-machine pushed a commit that referenced this pull request Mar 26, 2024
Stochastic failures in the RCU test on MACOSX are occuring.  Due to beta
release, disabling this test on MACOSX until post 3.3 release

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23967)
GamingFennex pushed a commit to GamingFennex/openssl that referenced this pull request Mar 27, 2024
Stochastic failures in the RCU test on MACOSX are occuring.  Due to beta
release, disabling this test on MACOSX until post 3.3 release

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#23967)

Fixing Format, Grammar, and Spelling.

- Changed many comment blocks to be the correct "/* */" format.
- Fixed general typos, mispellings, or incorrect grammar.

CLA: trivial
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.3.0 Planned eng view Include on 3.3.0 Engineering View Roadmap approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.3 Merge to openssl-3.3 severity: urgent Fixes an urgent issue (exempt from 24h grace period) tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants