input, dec_sub: avoid unnecessary recursive locks and don't use recursive mutex #13818
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a public function, yet its access to ictx through get_bind_section is not locked. Fixes: 4614d43
This makes it easy to eyeball check the call hierarchy between public and private functions.
na-na-hi
force-pushed
the
remove-recursive-lock
branch
from
b611b84
to
80939cd
Compare
|
Download the artifacts for this pull request: |
kasper93
reviewed
Apr 7, 2024
The absense of a call hierarchy between public and private functions results in many unnecessary recursive locks: public functions require locks, which are also called by other public and private functions in this file. Fortunately, since the lock is private to this file, this situation can be avoided by establishing a call hierarchy: - Public functions must lock, and can only call private functions in this file - Private functions must not lock, and can only call private functions in this file - No function can call any public function in this file, the only exception being mp_input_wakeup and mp_input_parse_cmd. This arrangement ensures that there will be no locks more than necessary: All public function calls will lock only once, and never recursively.
Previous commits made sure that the lock will never be called for more than once for all public functions. Thus deadlock is impossible, so recursive mutex is unneeded and can be converted to a normal mutex.
These public functions should use locks to keep its usage consistent with input.c. Fixes: 024e0cd
92a9f11 added locking for dec_sub. At that time, because the lock was exposed to the outside world, a recursive mutex was used. However, this is no longer true after e9e883e, when the public locking functions were removed. This means that the lock is now private. Unlike input.c, dec_sub already enforces said call hierarchy, so combined with the aforementioned change, the lock is only ever called once and never recursively. Thus the lock can be converted to a normal mutex.
It's currently always a meaningless 1. Make it so it returns 0 is cmd is NULL. Remove the unused return value from queue_cmd.
na-na-hi
force-pushed
the
remove-recursive-lock
branch
from
7651abc
to
095764d
Compare
kasper93
approved these changes
Apr 7, 2024
sub->sd can be destroyed and recreated when update_segment is called inside a lock. Fixes: f9918b5
|
Added a fix for another locking issue I found in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This restructures code by establishing a call hierarchy between public and private functions, so that any lock is called no more than necessary and recursive locks are no longer necessary for
input.canddec_sub.c, which means they no longer need recursive mutex. All uses of recursive mutex are now removed exceptvdpau.c, where we call a function outside of our control inside a lock.See also: #9727 (comment)