At least a read lock of the sVnodeLock must be held when calling
lookup_vnode, but we held none at all. This rectifies that problem.
This bug appears to have been around for many years, but no-one
noticed since ASSERT_READ_LOCKED_RW_LOCK only works with more
debug options turned on than the kernel is built with. I discovered
this while working on a new version of those additional options.
The functions declared in locks.h were and are exactly identical
to these inline blocks of code. So, rather than duplicate them,
just invoke them directly. The compiler will probably inline them
anyway.
This requires the use of fault handlers in the atomics.
GLTeapot now runs in the range of 590-610 FPS on my VM. However, it still
isn't using anywhere near 100% CPU usage. Some of that may be waiting for
app_server to respond to draw requests, but a lot of it still isn't.
Change-Id: I7be87d10cb1b00f07b055d9094b77837b49c5055
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6603
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
This requires the introduction of the flag B_USER_MUTEX_SHARED, and then
actually using the SHARED flags in pthread structures to determine when
it should be passed through.
This commit still uses wired memory even for per-team contexts.
That will change in the next commit.
GLTeapot FPS seems about the same.
Change-Id: I749a00dcea1531e113a65299b6d6610f57511fcc
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6602
Reviewed-by: waddlesplash <[email protected]>
Tested-by: Commit checker robot <[email protected]>
Now the table is locked with a rw_lock, and individual entries have
each their own rw_locks also.
This improves the "contention" benchmark I have here (2 locks, 6 threads,
acquire & release one lock 50,000 times per thread) mentioned in the last
commit down to under 4s consistently, around 3.45s with the system idle
and around 3.9s when moving windows around. Before this commit, it was
around 6.7s in the best case and 7.0s in the worst, and before that,
it couldn't break 3.8s in the best case.
This does make GLTeapot just slightly worse: it is now down to 310-330
(with occasional dips to 300-310) from 320-340. But the following commits
will improve that substantially.
Change-Id: Ie029a2510746f876f4d4c74d7e878fdadf3cf590
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6601
Reviewed-by: waddlesplash <[email protected]>
Tested-by: Commit checker robot <[email protected]>
* Get rid of the multiple entries/condition-variables system.
Instead, allocate one structure per variable and do not add/remove
it from the hash until all waiters are gone.
* Get rid of "locked". All wait wakeups of B_OK mean "locked". All
nonzero values mean "not locked." This mirrors what the kernel mutex
implementation does (however, that also tracks the owning thread,
for assertion's sake.)
* Remove "lastWaiter" logic (for now.) As we no longer hold a lock
after wakeup, we cannot reliably check and act on it outside the
"wait" function. This means that interrupted or timed-out waits
will cause a potentially unnecessary syscall on next unblock,
but that will be resolved in the next commit.
Due to the single global lock, user mutex acquisition is an extremely
"noisy" process that can take shorter or longer depending on what is
going on elsewhere on the system, so performance is hard to measure.
With one benchmark that acquires mutexes as fast as possible with
lots of contention, most runs came in as being around the same amount
of time both before and after this change (around 4.25s real). Moving
Terminal's window around while running the test caused runtime to go
up to around 6.7s before this change, and about 7.0s after.
GLTeapot seems to go from 350-380 FPS before this change and 320-340
after. It still spends the vast majority of its time waiting for address
space and cache locks, however.
It is expected that the next commits will build on this change to
improve performance beyond even the "before" numbers above.
Change-Id: I6581a6f7cb0ca0513ea639f8499a1c0c8596c026
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6490
Reviewed-by: waddlesplash <[email protected]>
Reviewed-by: Adrien Destugues <[email protected]>
Tested-by: Commit checker robot <[email protected]>
Condition variables are now a pretty common way the kernel blocks threads.
That means the "threads" command was getting difficult to navigate, since
at any given time, a lot of threads could be blocked on "cvar".
Now we try (carefully, because it could fault!) to fetch the first 4
characters of the "type" name and display then. This suffices to
distinguish the most common object block types in the list at a glance
(e.g. "cvar:port" for port reads, the most common.)
Change-Id: I94f4b59fd78b7ebdce913944551a5e98f0ca2e33
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6605
Reviewed-by: waddlesplash <[email protected]>
No longer is it required that the mutex be unlocked.
This was the case before the recent refactor, though it
wasn't noted anywhere. Now it is the case once more.
Should fix#18445.
We need to set the "to" mutex as locked+waiting before performing
the unlock of the first mutex, otherwise something in userland could
unset the "locked" flag but never call the kernel because "waiting"
had not yet been set.
In practice, the one consumer of this API (pthread_cond) could not,
at present, wind up in that situation, as far as I can tell, so this
race was entirely theoretical.
Suppose the following scenario:
1. Thread A holds a mutex.
2. Thread B goes to acquire the mutex, winds up in kernel waiting.
3. Thread A unlocks; first unsets the LOCKED flag.
As WAITING is set, it calls the kernel; but instead of processing
this immediately, the thread is suspended for any reason (locks,
reschedule, etc.)
4. Thread B hits a timeout, or a signal. It then unblocks in the kernel,
which causes the WAITING flag to be unset.
5. Thread C goes to acquire the lock. It sets the LOCKED flag.
It sees the WAITING flag is not set, so it returns at once,
having successfully acquired the lock.
6. Thread A, suspended back in step 3, resumes.
Now we encounter the problem. Under the previous code, the following
would occur.
7. Thread A sees that no threads are waiting. It thus unsets the LOCKED
flag, and returns from the kernel. Now we have a mutex theoretically
held by thread C but which (illegally) has no LOCKED flag set!
8. Some other thread tries to acquire the lock, and succeeds, for LOCKED
is not set. We now have one lock owned by two separate threads.
That's very bad!
The solution, in this commit, is to (1) switch from using "atomic_or"
to lock mutexes, to using "atomic_test_and_set", and (2) mandate that
_kern_unblock_mutex must be invoked with the mutex already unlocked.
Trying to solve the problem with (2) but without (1) produces other
complications and would overall be more complicated. For instance,
all existing userland code expected that it would set LOCKED, but then
check LOCKED|WAITING. If _kern_mutex_unlock does not unset LOCKED,
then whichever thread sets LOCKED when it was previously unset is
now the mutex's undisputed owner, and if it fails to notice this,
would deadlock.
That could have been solved with extra checks at all lock points, but
then that would mean locks would not be acquired "fairly": it would
be possible for any thread to race with an unlocking thread, and
acquire the lock before the kernel had a chance to wake anyone up.
Given how fast atomics can be, and how slow invoking the kernel is
comparatively, that would probably make our mutexes extremely "unfair."
This would not violate the POSIX specification, but it does seem like
a dangerous choice to make in implementing these APIs.
Linux's "futex" API, which our API bears some similarities to, requires
at least one atomic test-and-set for an uncontended acquisition,
and multiple atomics more for even the simplest case of contended
acquisition. If it works for them, it should work for us, too.
Fixes#18436.
Change-Id: Ib8c28acf04ce03234fe738e41aa0969ca1917540
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6537
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: Adrien Destugues <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
The first of these assertions in the pthread code is actually possible
to trigger under some specific circumstances, which is ticket #18436.
This makes that problem more obvious when it does happen.
Change-Id: I026ea6e4c569a7c20d82b70722f752d87e57c5a1
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6536
Reviewed-by: waddlesplash <[email protected]>
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: Adrien Destugues <[email protected]>
pthread_barrier_wait can return errors, which on Haiku are negative
and so -1 is an error condition, it should not be reused for a magic
constant.
This breaks ABI. However, until the recent fixes, barriers were so broken
that I doubt any application was using them seriously (Mesa, for instance,
has them disabled.)
Change-Id: Ica23921de012a33e9e7aded816bb1347bd157b31
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6517
Reviewed-by: Jérôme Duval <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: X512 <[email protected]>
The previous implementation was prone to deadlocks when the next round
of threads tried to enter the barrier before the prior round exited it.
This new version takes care of that problem, and also removes some
other contention.
Basic design:
* waiter_count is now atomic, which means only the "serial" thread, or
in case of contention threads that raced, need acquire the mutex.
* mutex remains locked during threads wakeup, at which point waiter_count
is negative. It is only unlocked when count reaches 0 in the last-woken
thread. This protects against the races that lead to deadlocks.
* Remove usage of _kern_mutex_switch_lock. This was done incorrectly;
if it returned EINTR, the first lock would be unlocked but the second
would not be acquired, creating further races. Instead, we leave
the barrier lock in "LOCKED" state at all times except when we
actually want to wake threads up, when it is left "Unlocked"
(and "unlocked" by each successive exiting thread, just in case.)
Fixes#15736.
We must do this to prevent lock order inversion: when busses are
initialized, they are started by the (locked) device manager, and
then acquire the explore lock. We must do the same in Explore itself,
for when called by the explore thread, we would otherwise first acquire
the explore lock, then (when publishing new nodes) acquire the device lock.
Should fix#18421 and #18393.
If the dimensions are already larger than needed, don't add to them.
Fixes Terminal and other non-layout applications' display
following the prior change.
In the case of devices where the mass-storage interface is not the
first one, we will miss it unless we were to check all attributes
one at a time in order. Instead of doing that, just fetch the configuration
and enumerate it directly.
I saw in a KDL stack trace that this function was using over 4KB of stack.
As it is called as part of device_manager startup, where there can be
pretty deep recursion happening, that seems like a bad idea.
Additionally, it did not actually do bounds checking.
Now we dynamically allocate an array of sufficient length.
There is now an "io_lock" which must be held when performing any USB
operations. All ioctls that read basic status, size, icon, etc. information
do not need to acquire it, of course, which should improve userland
lockup occurrences on congested USB disks.
A "bypass" mechanism is left in for when DMAResource would just add
overhead for no reason. All other I/O goes through it and is submitted
to the USB stack as physical addresses.
Tested in QEMU, can still boot from USB on all busses.
Fixes#15569.
Change-Id: I26bfd2208de4ebe1a17170a7034316076927663f
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6480
Reviewed-by: waddlesplash <[email protected]>
Introduce a new utility method, "generic_memcpy", which takes
generic_addr_t plus indications of whether these specify virtual or
physical addresses (and potentially user addresess) and calls the
appropriate memcpy variant depending.
All bus drivers adjusted to support this at once. We don't actually
take advantage of the physical addresses in any way (yet), as USB
controllers have some pretty specific requirements that would have
to be carefully validated to use these directly.
All bus drivers tested and confirmed to still be working.
Change-Id: I66326667e148091147bb2b3d0843a26fb7e5bda6
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6479
Reviewed-by: waddlesplash <[email protected]>
Prepares the way for actual physical-address request support,
which is not yet wired up.
Change-Id: I1b2d04a31e334a79b8361280fa0f3a5fbdb43d2b
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6478
Reviewed-by: waddlesplash <[email protected]>
This way it is more easily accessed from drivers outside the kernel,
which it soon will be, without having to add an explicit UseHeaders.
(The drivers that use it already all use the IOScheduler.)
No functional change.
Change-Id: Ibc2d2678e37d9d7ab73391cb17b72cca86f92132
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6477
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
Before the PCI refactor, PCI initialization/enumeration occurred
immediately after the PCI module was loaded, and so by the time
we got to IOAPIC initialization, it was already complete.
After the refactor, PCI enumeration is deferred until slightly later,
and so we would try to initialize IO-APICs without knowing PCI
information. This would fail, as read_irq_routing_table needs to
have that available.
Hopefully fixes#18425, #18393, #18398.
Change-Id: I1e4b06367da26eeb10085a1c6322ed39885b632b
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6476
Reviewed-by: X512 <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
This is mostly important for B_GET_MEDIA_STATUS, which actually will
retry actions if the err_act is set appropriately.
Following this change, stalls are now fully recoverable without causing
the file descriptors of the mounted partition to be cleared, and the
system can resume normal operation without a problem.
Fixes part of #15569.
May help with #16745, #18185, #17543, #18421.
XHCI, at least, has "sticky" HALT states which must be cleared at
the controller level. Invoking cancel_queued_transfers takes care
of this.
This fixes USB disks spontaneously unmounting whenever stalls occur.
However, trying to read anything from the mounted partition results
in "Bad file descriptor", probably due to media-status error propagation.
This allows us to drop the PCI-specific ACPI management entirely.
Confirmed working on x86, and the fallbacks when there is no ACPI
also still work.
Change-Id: I6dac9f5539f99b934b17b341634ce22628bc66fd
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6470
Reviewed-by: X512 <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
We did not ever implement these, it seems, and so they are just
cluttering up the global namespace.
Change-Id: Ib37c3a31663525a18268c9bfe326bfba9afbc794
The old TTY driver had one set of indexes for both master
and slave TTYs, and it used those indexes interchangeably.
We thus need to check both lists here also.
Fixes SSH passphrase prompts not working.
This new driver uses the "generic" TTY layer, unlike the old driver
which had its own implementation (which the generic module was derived
from, originally.)
The remaining bits of support for controlling TTYs is added to the kernel &
generic layer at the same time, which should allow for serial interfaces
to be controlling terminals now, as well.
Tested with bash, nano, vim; all seems to still be working as expected.
This was used by the BeOS port of ncurses, as well as our own ncurses
port until not too long before the alpha1 days, so we should keep it
around for compatibility.
Implementation copied directly from the "legacy" TTY driver.
It doesn't make much sense to have separate window sizes, termios, etc.
settings for the two halves of a TTY. Moreover, having separate settings
which can get out of sync breaks applications, e.g. double-printing
in shells.
The original TTY driver has unified settings. It seems likely that the
settings were separated as part of the locking simplification, however
the lock separation was reverted a while back, anyway.
I am not sure how this path could be hit besides having
O_APPEND set on a socket, which appears to be possible,
though I don't know what purpose that would serve.
Tested by adding these two lines between the sleep() and close()
in the in-tree tcp_connection_test:
fcntl(fd, F_SETFL, O_APPEND);
write(fd, "Hello", 5);
Before this commit, the above lines cause a KDL.
May fix#18133, but I don't presently have access to the
reproduction setup described in that ticket.
Until the introduction of the nvme_disk driver, these classes were
mostly only used directly by the IO scheduler, and then a few direct
usages of IOOperation itself in the individual disk drivers; so
API confusions were easily missed.
When writing the nvme_disk driver's IORequest support, however, it
became readily apparent that there were some pretty bad confusions
around transferred-bytes accounting in IOOperation. This commit
attempts to resolve all of those.
There are two basic changes here:
1. Move transferred-bytes accounting into IOOperation::SetStatus.
The "TransferredBytes" field of IOOperation is against the *original*
range, not the actual operation's range (which will be wider, due to
bouncing, etc.), and furthermore only applies to the actual content
of the request (and not e.g. to a read half of a bounced write.)
These two facts meant that determining what value to pass to
SetTransferredBytes was not trivial, and was easy to get wrong.
I recall messing that up when working on nvme_disk multiple times
before reading the API carefully.
2. Do not pass redundant values to IORequest::OperationFinished.
All of the values here can be derived (albeit indirectly) from the
IOOperation, and all consumers of this API basically did just that.
Rather than make them do it, make the IORequest take care of
computing all of those values itself.
Change-Id: Ic9ae29e1100319e5b7647647c4db7e5aad4d125e
This substitutes for the already-existing behavior of scsi_cd, scsi_disk,
and virtio_block, so we can delete their hooks and let them use
these new generic fallbacks.
Some other drivers perform clamping, and so using these fallbacks would
constitute a behavioral change.
Change-Id: I9a2e503f2e03abc276bdfc02d1cff1565a9742e9
* Move geometry fetching to open(); it was not done
in io() and was duplicated in multiple places.
* Clean up clamping and bounds checking.
* Use IORequest::TransferredBytes().
Change-Id: I4157e516098dc0362c1478abd21a545c1f235cd7
The user address space has not included NULL for a while, so this
has actually been broken for years, and nobody noticed. I guess
XSI message queues are not very well used?
Fixes the in-tree "xsi_msg_queue_test1".
This removes a lot of custom logic for managing waiting threads,
which was not even correct in all cases (and the code actually
acknowledged this with a big TODO about it, which weinhold
added all the way back in 2008!)
THREAD_BLOCK_TYPE_OTHER implies the "object" pointer in the
wait information is a string. But sometimes we want to pass
through objects which are not strings, for inspection in KDL.
We need to store the isGlobal value, so that the destructor
can take care of unsetting the glibc locale properly.
It seems this has been broken since d338200e2b.
Fixes#18344 and probably #18336.
We cannot actually initiate scans, but we can at least detect when
one is in progress.
This fixes "ifconfig ... scan" on OpenBSD devices returning errors
instead of scan results ("list" always worked.)
The condition variable that the load_image'ing thread is waiting on
is also owned by that same thread, so as soon as it wakes up, it
will soon return, thus destroying it. Under high load or other unlucky
scheduling conditions, it seems this could occur before the other thread
had even returned from the condition variable's NotifyAll.
Since team->loading_info is protected by the team lock, simply
acquire the team lock once more after being awoken and returning,
to synchronize and prevent this race.
Should fix#18352.
Augustin Cavalier