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

Fix GH-13437: FPM: ERROR: scoreboard: failed to lock (already locked) #15805

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Sep 8, 2024

This changes locking for scoreboard to reduce contention between readers and adds retries for acquiring scoreboard for read.

@bukka bukka changed the base branch from master to PHP-8.2 September 8, 2024 14:59
@bukka bukka force-pushed the fpm_scoreboard_locking_fix branch from 216d268 to 0f647dc Compare September 8, 2024 14:59
@bukka
Copy link
Member Author

bukka commented Sep 8, 2024

This is an attempt to fix GH-13437 which seems to work in my testing. However it might be a bit too much for the actual bug fix - specifically reduction of reader contention might not be necessary here because I didn't see that much reader contention in my testing. There is mostly writer contention so it should be enough to just add retries but the question is impact on performance. I'm still not decided whether to target 8.2 at the end. It will need more testing though. And also need to do some clean up as I initially started just with atomics so there are some left overs in the code.

@bukka
Copy link
Member Author

bukka commented Sep 8, 2024

Btw there are few more issues with locking procs and optimizing atomics that I plan to look later too.

@bukka bukka mentioned this pull request Oct 23, 2024
@bukka bukka force-pushed the fpm_scoreboard_locking_fix branch from 0f647dc to a584cc8 Compare November 23, 2024 13:26
@bukka bukka changed the base branch from PHP-8.2 to PHP-8.3 November 23, 2024 13:26
bukka added a commit to bukka/php-src that referenced this pull request Nov 23, 2024
This changes locking for scoreboard to reduce contention between readers
and adds retries for acquiring scoreboard for read.

Closes phpGH-15805
@bukka bukka force-pushed the fpm_scoreboard_locking_fix branch from a584cc8 to c3366e2 Compare November 23, 2024 13:27
@bukka bukka marked this pull request as ready for review November 23, 2024 13:27
@bukka
Copy link
Member Author

bukka commented Nov 23, 2024

I have done some further testing and checking and think it's ok for review. I might do even more testing next week so I changed the base to 8.3 as 8.2 is pretty close to the end of active support and this is potentially risky even considering the new regression rule. I can also control better quick fixes to 8.3 if needed in case it needs changes as quickly as possible.

I also added hid some debug logs as they are too noisy even for debug level and they go also to external logger (fcgi stderr) for those triggered in the child. I plan to improve this in master (filtering for external logger and introduction of new trace log level for FPM).

@bukka bukka marked this pull request as draft November 27, 2024 19:08
@bukka
Copy link
Member Author

bukka commented Nov 27, 2024

I was checking the failures in tests and there are related so need to debug them first and then do even more testing as my initial testing completely missed it.

This changes locking for scoreboard to reduce contention between readers
and adds retries for acquiring scoreboard for read.

Closes phpGH-15805
@bukka bukka force-pushed the fpm_scoreboard_locking_fix branch from c3366e2 to 6201bc5 Compare December 7, 2024 18:33
@bukka bukka marked this pull request as ready for review December 7, 2024 19:34
@bukka
Copy link
Member Author

bukka commented Dec 7, 2024

Found the issue and fixed it so all should be hopefully ready now. Will do a bit more testing next week and if I don't find anything, I will merge it.

@bukka bukka closed this in 3490ac0 Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants