This "race prevention" code does not seem to be really hit at all
in practice, at least from testing, so no need to do a full
unlock/lock universally for it.
I'm still not sure why the previous fixes here removed 80% of
the performance benefits of the original change; I need to
investigate that more.
Consider this scenario:
* A userland thread puts its ID into some structure so that it
can be woken up later, sets its wait_status to initiate the
begin of the wait, and then calls _user_block_thread.
* A second thread finishes whatever task the first thread
intended to wait for, reads the ID almost immediately
after it was written, and calls _user_unblock_thread.
* _user_unblock_thread was called so soon that the first
thread is not yet blocked on the _user_block_thread block,
but is instead blocked on e.g. the thread's main mutex.
* The first thread's thread_block() call returns B_OK.
As in this example it was inside mutex_lock, it thinks
that it now owns the mutex.
* But it doesn't own the mutex, and so (until yesterday)
all sorts of mayhem and then a random crash occurs, or
(after yesterday) an assert-failure is tripped that
the thread does not own the mutex it expected to.
The above scenario is not a hypothetical, but is in fact the
exact scenario behind the strange panics in #15211.
The solution is to only have _user_unblock_thread actually
unblock threads that were blocked by _user_block_thread,
so I've introduced a new BLOCK_TYPE to differentiate these.
While I'm at it, remove the BLOCK_TYPE_USER_BASE, which was
never used (and now never will be.) If we want to differentiate
different consumers of _user_block_thread for debugging
purposes, we should use the currently-unused "object"
argument to thread_block, instead of cluttering the
relatively-clean block type debugging code with special
types.
One final note: The race condition which was the case of
this bug does not, in fact, imply a deadlock on the part
of the rw_lock here. The wait_status is protected by the
thread's mutex, which is acquired by both _user_block_thread
and _user_unblock_thread, and so if _user_unblock_thread
succeeds faster than _user_block_thread can initiate
the block, it will just see that wait_status is already
<= 0 and return immediately.
Fixes#15211.
* It's safe to assume that if the file is shorter than
the provided header, things will go poorly.
* Avoids a random vauge ReadBuffer error.
* This doesn't fix#15230, but makes the issue clearer.
Change-Id: I3471e6de384a0c9be94049ad891c01be980f7846
Reviewed-on: https://review.haiku-os.org/c/1679
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
This change renders a striped background in the
'barber pole' in HaikuDepot application when it is
idle. This makes the 'barber pole' easier to use
in situations where the space that the UI control
takes up should not be blank. The logic for the
striped background is from the 'drivesetup'
application.
Change-Id: I87791c70b4d1a21d91e661433d6c940ca69ece87
Reviewed-on: https://review.haiku-os.org/c/1674
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
This avoids a few potential race conditions mmlr pointed out on
the mailing list. See inline comments.
Change-Id: I605523c1d2683c749751599c417a68a20c70edea
Previously, both the "old" owner and the "new" lock owner set
the lock holder to the "new" lock holder. Now the old owner does
it before calling unblock(), and we check in the lock() functions
that we are indeed the new owner of the lock.
This may affect what the panic is for #15211 and its duplicates:
I have a suspicion that these threads are getting unblocked
when they have no business being unblocked, and this should
catch that condition.
Should (actually) fix#15223 and #15226. After things settle
down a bit, I'll refactor our ConditionVariable constructors
to detect this at compile time instead of creating obscure
KDLs...
Before this commit, *all* ConditionVariable operations (yes, all;
even Wait, Notify, etc.) went through a single spinlock, that also
protected the sConditionVariableHash. This obviously does not scale
so well with core count, to say the least!
With this commit, we add spinlocks to each Variable and Entry.
This makes locking somewhat more complicated (and nuanced; see
inline comment), but the trade-off seems completely worth it:
(compile HaikuDepot in VMware, 2 cores)
before
real 1m20.219s
user 1m5.619s
sys 0m40.724s
after
real 1m12.667s
user 0m57.684s
sys 0m37.251s
The more cores there are, the more of an optimization this will
likely prove to be. But 10%-across-the-board is not bad to say
the least.
Change-Id: I1e40a997fff58a79e987d7cdcafa8f7358e1115a
This way the static functions (used only in KDL) are below
the actual notification methods, which are now all grouped
together in one place.
No functional change.
Since this thread has a very high priority, this causes the whole system
to lock up, making recovery from such a critical condition even more
impossible. Instead use the Warning-level timeout instead (0.3 seconds),
and of course we will be notified via the semaphore if something occurs
we should know about.
I find this handy and in addition it makes the slide show cyclic again,
fixing #10386.
Going the other way might also be useful, but that requires some additions to
the Navigator code first.
This is part of a new diagnostic I've managed to wire up
that finds all instances of "status_t" function return
values being discarded using attribute trickery and some
creative Clang pragmas. As you might guess, the resultant
errors list is absolutely gigantic, and most of them are
ones where the failure condition will never (that we care
about) be hit.
The ones in this commit likely are no-op changes, but
they were low-hanging fruit spotted while reviewing
the larger list. The next commit will bring more
substantial changes.
Also do some cleanup in private headers, I can't imagine why the build
libraries would need this function.
Change-Id: Ib08810b6efe4738dad596a735d741582a3781b28
Reviewed-on: https://review.haiku-os.org/c/1670
Reviewed-by: Ryan Leavengood <leavengood@gmail.com>
memcpy with a negative parameter doesn't look like a great idea.
Change-Id: I3d1a8c224bd288784f068086652a84b2b75fcea8
Reviewed-on: https://review.haiku-os.org/c/1671
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Checking errors is important. This properly fixes#10365 and resolves a few
TODOs.
This DirectoryFilter is also used in Expander, though with a lower level
implementation that did not trigger this bug. This feels like it could be
in the the Tracker or shared kit.
Change-Id: Icd2ddc241c1879a7c4235726bf089570ba00dc0a
Reviewed-on: https://review.haiku-os.org/c/1672
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Bitshifts and masks are a lot more readable here.
Change-Id: I94c8603b75d42456843a0b53bf2a0547aaffdb74
Reviewed-on: https://review.haiku-os.org/c/1669
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
FDs are now object_cached, so this triggered an assert-failure panic
trying to put them through the regular allocator. Just use put_fd
to free these instead, so that there is only one "destruction" path
for FDs.
Fixes#15213.
In CodyCam, we attempt to cast the result of this macro to const char*.
However, the ternary operator has lower priority than the cast so it
doesn't work as expected.
Add some protective parentheses here.
Change-Id: I5e9875187cec67b9534b1bbe58d82217c6cd5524
Reviewed-on: https://review.haiku-os.org/c/1667
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Just rewrite the loop in a more readable way.
Change-Id: I174016eac74eb54a01e5226b5f8a92fb1b335830
Reviewed-on: https://review.haiku-os.org/c/1665
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Use #if 0 to disable code that should be disabled.
Change-Id: I797383eb8c68681459d8cd56d1b3addda960fa44
Reviewed-on: https://review.haiku-os.org/c/1664
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Unless __STRICT_ANSI__ is defined (as it is when running the compiler in
--std=c89 or --std=c99, but not when running it without any specific
args), we can enable these by default and behave like most other
systems. I don't know why no one has done this yet despite suggesting it
multiple times and people prefer to #define _BSD_SOURCE manually
everywhere.
Remove all places in our Jamfiles and sources where it had been defined.
_DEFAULT_SOURCE is now enabled by default for all sources of Haiku, since we
let the compiler use GNU extensions (no strict C standard specified on
command line)
Use _DEFAULT_SOURCE as the define name to match current versions of
glibc. Enable it if _BSD_SOURCE is #defined in compiler flags, for
backward compatibility.
Change-Id: I6db04da5f6db437723cdfba3478f5094a69d7727
Reviewed-on: https://review.haiku-os.org/c/1633
Reviewed-by: Jérôme Duval <jerome.duval@gmail.com>
Previously, a new log-flush thread was created every time there was
an idle transaction. Now we just release a semaphore for an
already-existing thread.
Change-Id: If788dbe17ef8e069ce12aa7b778626d051cce2d0
We already have a dummy spawn_kernel_thread, so this makes sense to add
(and will be required by the next commit.)
Change-Id: Ic46607d46dabc6fd46fcfc0b6f8da0ed9897cfc9
Reviewed-on: https://review.haiku-os.org/c/1650
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
This behavior was always used by the semaphore-based MultiLocker,
and it seems a good chunk of code depends on this, so we need
to preserve the behavior.
