Skip to content

Mac kext: Replaces use of OSAtomic with standard C11 atomic operations#988

Merged
pmj merged 1 commit intomicrosoft:masterfrom
pmj:mac-kext-c11-atomics
Apr 2, 2019
Merged

Mac kext: Replaces use of OSAtomic with standard C11 atomic operations#988
pmj merged 1 commit intomicrosoft:masterfrom
pmj:mac-kext-c11-atomics

Conversation

@pmj
Copy link
Contributor

@pmj pmj commented Mar 31, 2019 •
edited
Loading

The OSAtomic APIs are not available in that form in user space, and so caused errors when attempting to call functions (ProviderMessaging_TrySendRequestAndWaitForResponse()!) that use them in the kext testing target. The similar user space equivalents are already deprecated in any case, so replacing the kext versions' use with the standardised C11 operations is the sensible choice.

@pmj
Mac kext: Replaces use of OSAtomic with standard C11 atomic operation
27d117d 
The OSAtomic APIs are not available in that form in user space, and so caused errors when linking the testable kext. The similar user space equivalents are already deprecated in any case, so replacing their use with the standardised C11 operations is the sensible choice.
@pmj pmj requested review from jeschu1 and wilbaker April 1, 2019 12:16
@nickgra
Copy link
Member

nickgra commented Apr 1, 2019

IIRC, the OSAtomics being transitioned to C11 atomics is what caused our problem with IOSharedDataQueue. While we believe that to be fixed, I'm going to run a large repo build on top of this PR just to be sure.

https://mseng.visualstudio.com/AzureDevOps/_build/results?buildId=9158349

@pmj
Copy link
Contributor Author

pmj commented Apr 1, 2019 •
edited
Loading

IIRC, the OSAtomics being transitioned to C11 atomics is what caused our problem with IOSharedDataQueue. While we believe that to be fixed, I'm going to run a large repo build on top of this PR just to be sure.

I'm pretty confident this change won't suffer from the same problem, but running the large repo build is definitely a good idea. The problem with IOSharedDataQueue was that the developer who ported that to C11 atomics for 10.13.6 got fancy with relaxing memory ordering to (I guess?) eke out a tiny extra bit of performance. They got it wrong though and relaxed it too much. I've not used the atomic_*_explicit() function variants here, just the atomic_*() which uses the default, sequentially consistent ordering, which is the most stringent and should be equivalent to the OSAtomic ordering guarantees.

@jrbriggs jrbriggs added this to the M151 milestone Apr 2, 2019
@pmj
Copy link
Contributor Author

pmj commented Apr 2, 2019

Looks like the large repo build completed without a hitch, so I'm going to go ahead and merge this.

@pmj pmj merged commit c55503f into microsoft:master Apr 2, 2019
@pmj pmj deleted the mac-kext-c11-atomics branch April 2, 2019 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@jeschu1 jeschu1 jeschu1 approved these changes

@wilbaker wilbaker wilbaker approved these changes

@nickgra nickgra nickgra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

domain: kext platform: macOS

Projects

None yet

Milestone

M151

Development

Successfully merging this pull request may close these issues.

5 participants

@pmj @nickgra @jeschu1 @wilbaker @jrbriggs