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

[release/6.0] Fixed the yp_spin_count_unit to be a factor of the original_spin_count_unit rather than a continually increasing value #68881

Merged
merged 2 commits into from
May 5, 2022

Conversation

mrsharm
Copy link
Member

@mrsharm mrsharm commented May 5, 2022

Backport of #68879 to release/6.0.

Customer Impact

Reported by Cosmos; the associated bug can be found here: #68839

Details

This PR is to address #68839 where it's clear that SetYieldProcessorScalingFactor is called multiple times thereby increasing the value of yp_spin_count_unit and as a result the spin counts are monotonically increasing each time SetYieldProcessorScalingFactor is called; this function was previously assumed to be called only once but that changed with the following PR: #55295.

The fix is to store the original spin count unit and use that value to set the yp_spin_count_unit rather than adjusting the value based on the last yp_spin_count_unit.

Testing

Tested on a simple console app that has Server GC enabled and made sure the spin lock count is constrained. The repro code as a simple console app is:

var ALLOC_SIZE = 10_000;

while (true)
{
    Thread.SpinWait(10000);
    byte[] buffer = new byte[ALLOC_SIZE];
    buffer = new byte[ALLOC_SIZE];
    buffer = new byte[ALLOC_SIZE];
}

Thread.SpinWait eventually calls SetYieldProcessorScalingFactor; the result of the testing indicated that the yp_spin_count wasn't monotonically increasing like before.

Risk

This PR mitigates a high-risk issue and reduces the risk of the spin count growing by constraining max count.

…t_unit rather than a continually increasing value
@ghost
Copy link

ghost commented May 5, 2022

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Summary

This PR is to address #68839 where it's clear that SetYieldProcessorScalingFactor is called multiple times thereby increasing the value of yp_spin_count_unit and as a result the spin counts are monotonically increasing each time SetYieldProcessorScalingFactor is called; this function was previously assumed to be called only once but that changed with the following PR: #55295.

The fix is to store the original spin count unit and use that value to set the yp_spin_count_unit rather than adjusting the value based on the last yp_spin_count_unit.

NOTE: This change was added to main: #68879 - this PR is the cherry pick of that change into 6.0.

Author: mrsharm
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Please fill in the servicing template, and get a handful of code reviews. We can then take for consideration in 6.0.x

@mrsharm mrsharm changed the title Fixed the yp_spin_count_unit to be a factor of the original_spin_count_unit rather than a continually increasing value [release/6.0] Fixed the yp_spin_count_unit to be a factor of the original_spin_count_unit rather than a continually increasing value May 5, 2022
@JulieLeeMSFT JulieLeeMSFT added the Servicing-consider Issue for next servicing release review label May 5, 2022
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.x milestone May 5, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 5, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.6 May 5, 2022
@carlossanlop carlossanlop merged commit e41228d into dotnet:release/6.0 May 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-GC-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants