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

Add !$OMP critical directives around some GetNewUnit/Open*File calls #2554

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

andrew-platt
Copy link
Collaborator

@andrew-platt andrew-platt commented Dec 10, 2024

Ready to merge.

Feature or improvement description
We have had issues with unit numbers when opening files for many years when OpenMP is used. The root issue was that the GetNewUnit call was separate from the actual file opening. This could lead to race conditions when multiple threads hit a GetNewUnit before the retrieved unit numbers were used to open the corresponding files.

As a temporary solution, I added !$OMP critical(filename) directives around some of the GetNewUnit/Open*File call pairs. Some logic restructuring was required in a few places since a return cannot take place in one of these directives (note: the only places this occurs is where GetNewUnit error handling was previously handled correctly, however GetNewUnit only returns an ErrID_Severe, so the logic in there now kind of ignores all error handling from GetNewUnit). This solution is not really ideal.

The ideal solution would be more along the lines of what was started with #2538, but to do that correctly would break some backwards compatibility. It is therefore better to put that solution where GetNewUnit is embedded within the Open*File subroutines into 4.0.0 instead.

Related issue, if one exists
#2510 (Probably others as well)
#2538 -- first attempt at fixing this in 3.5.5

Impacted areas of the software
There should be no real impact to end users.

Additional supporting information
The !$OMP critical directive supports labels, so for all uses added here, the label filename has been added. Without this, an !$OMP critical around a call to a routine that includes the new directive would simply hang - the inner (unnamed) !$OMP critical would not be able to start until the outer finishes, which it couldn't do. I'm mostly being lazy here as I don't want to sort through all the code to find instances of !$OMP critical to make sure no nested calls occured.

Test results, if applicable
No results should change.

@andrew-platt andrew-platt added this to the v3.5.5 milestone Dec 10, 2024
@andrew-platt andrew-platt self-assigned this Dec 10, 2024
@andrew-platt andrew-platt force-pushed the f/OMP_critical branch 2 times, most recently from f921a97 to 98054c2 Compare December 10, 2024 22:02
Copy link
Collaborator

@deslaughter deslaughter left a comment

Choose a reason for hiding this comment

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

Looks good to me

@andrew-platt andrew-platt merged commit 64e00f4 into OpenFAST:rc-3.5.5 Dec 16, 2024
38 checks passed
@andrew-platt andrew-platt deleted the f/OMP_critical branch December 18, 2024 21:11
@andrew-platt andrew-platt mentioned this pull request Dec 19, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants