This change adds a static lock hierarchy / ranking to the Kernel with
the goal of reducing / finding deadlocks when running with SMP enabled.
We have seen quite a few lock ordering deadlocks (locks taken in a
different order, on two different code paths). As we properly annotate
locks in the system, then these facilities will find these locking
protocol violations automatically
The `LockRank` enum documents the various locks in the system and their
rank. The implementation guarantees that a thread holding one or more
locks of a lower rank cannot acquire an additional lock with rank that
is greater or equal to any of the currently held locks.
There are certain checks that we should skip if the system is crashing.
The system can avoid stack overflow during crash, or even triple
faulting while while handling issues that can causes recursive panics
or aborts.
We previously allowed Thread to exist in a state where its m_name was
null, and had to work around that in various places.
This patch removes that possibility and forces those who would create a
thread (or change the name of one) to provide a NonnullOwnPtr<KString>
with the name.
The default template argument is only used in one place, and it
looks like it was probably just an oversight. The rest of the Kernel
code all uses u8 as the type. So lets make that the default and remove
the unused template argument, as there doesn't seem to be a reason to
allow the size to be customizable.
This commit moves the KResult and KResultOr objects to Kernel/API to
signify that they may now be freely used by userspace code at points
where a syscall-related error result is to be expected. It also exposes
KResult and KResultOr to the global namespace to make it nicer to use
for userspace code.
This is the idiomatic way to declare type aliases in modern C++.
Flagged by Sonar Cloud as a "Code Smell", but I happen to agree
with this particular one. :^)
Previously, we would try to acquire a reference to the all processes
lock or other contended resources while holding both the scheduler lock
and the thread's blocker lock. This could lead to a deadlock if we
actually have to block on those other resources.
Let's use an RAII helper to avoid having to update this on every path
out of block().
Note that this extends the time under `m_in_block == true` by a little
but that should be harmless.
The `m_should_block` member variable that many of the Thread::Blocker
subclasses had was really only used to carry state from the constructor
to the immediate-unblock-without-blocking escape hatch.
This patch refactors the blockers so that we don't need to hold on
to this flag after setup_blocker(), and instead the return value from
setup_blocker() is the authority on whether the unblock conditions
are already met.
This was previously used after construction to check for early unblock
conditions that couldn't be communicated from the constructor.
Now that we've moved early unblock checks from the constructor into
setup_blocker(), we don't need should_block() anymore.
Instead of registering with blocker sets and whatnot in the various
Blocker subclass constructors, this patch moves such initialization
to a separate setup_blocker() virtual.
setup_blocker() returns false if there's no need to actually block
the thread. This allows us to bail earlier in Thread::block().
When adding a WaitQueueBlocker to a WaitQueue, it stored the blocked
thread in the registration's custom "void* data" slot.
This was only used to print the Thread* in some debug logging.
Now that Blocker always knows its origin Thread, we can simply add
a Blocker::thread() accessor and then get the blocked Thread& from
there. No need to register custom data.
There's no harm in the blocker always knowing which thread it originated
from. It also simplifies some logic since we don't need to think about
it ever being null.
The BlockerSet stores its blockers along with a "void* data" that may
contain some blocker-specific context relevant to the specific blocker
registration (for example, SelectBlocker stores a pointer to the
relevant entry in an array of SelectBlocker::FDInfo structs.)
When unregistering a blocker from a set, we don't need to key the
blocker by both the Blocker* and the data. Just the Blocker* is enough,
since all registrations for that blocker need to be removed anyway as
the blocker is about to be destroyed.
So we stop passing the "void* data" to BlockerSet::remove_blocker(),
which also allows us to remove the now-unneeded Blocker::m_block_data.
Namely, will_unblock_immediately_without_blocking(Reason).
This virtual function is called on a blocker *before any block occurs*,
if it turns out that we don't need to block the thread after all.
This can happens for one of two reasons:
- UnblockImmediatelyReason::UnblockConditionAlreadyMet
We don't need to block the thread because the condition for
unblocking it is already met.
- UnblockImmediatelyReason::TimeoutInThePast
We don't need to block the thread because a timeout was specified
and that timeout is already in the past.
This patch does not introduce any behavior changes, it's only meant to
clarify this part of the blocking logic.
Namely, unblock_all_blockers_whose_conditions_are_met().
The old name made it sound like things were getting unblocked no matter
what, but that's not actually the case.
What this actually does is iterate through the set of blockers,
unblocking those whose conditions are met. So give it a (very) verbose
name that errs on the side of descriptiveness.
By the time we end up destroying a BlockerSet, we don't need to take
the internal spinlock. And nobody else should be holding it either.
So replace the SpinlockLocker with a VERIFY().
This patch does three things:
- Convert the global thread list from a HashMap to an IntrusiveList
- Combine the thread list and its lock into a SpinLockProtectedValue
- Customize Thread::unref() so it locks the list while unreffing
This closes the same race window for Thread as @sin-ack's recent changes
did for Process.
Note that the HashMap->IntrusiveList conversion means that we lose O(1)
lookups, but the majority of clients of this list are doing traversal,
not lookup. Once we have an intrusive hashing solution, we should port
this to use that, but for now, this gets rid of heap allocations during
a sensitive time.
The LOCK_DEBUG conditional code is pretty ugly for a feature that we
only use rarely. We can remove a significant amount of this code by
utilizing a zero sized fake type when not building in LOCK_DEBUG mode.
This lets us keep the same API, but just let the compiler optimize it
away when don't actually care about the location the caller came from.
By making these functions static we close a window where we could get
preempted after calling Processor::current() and move to another
processor.
Co-authored-by: Tom <tomut@yahoo.com>
I had to move the thread list out of the protected base area of Process
so that it could live with its lock (which needs to be mutable).
Ideally it would live in the protected area, so maybe we can figure out
a way to do that later.
...and also RangeAllocator => VirtualRangeAllocator.
This clarifies that the ranges we're dealing with are *virtual* memory
ranges and not anything else.
