Before 2019, the entire ConditionVariable system was "giant"-locked:
that is, there was a single global lock that all ConditionVariable
and ConditionVariableEntry operations had to pass through. This of
course was not very performant on multicore systems and when
ConditionVariables see significant use, so I reworked it then to have
more granular locking.
Those patches took a number of attempts to get right, as having two
objects in separate threads that can each access the other not turn
into a deadlock or use-after-free is not easy to say the least,
and the ultimate solution I came up with erased most of the performance
gains I initially saw on the first (partially broken) patchsets.
So I have wanted to revisit this and see if there was a better way
even since then. Recently there have been a few reports of
ConditionVariable-related panics (apparently double unlocks),
notably #16894, and so that was reason enough to actually revisit
this code and see if a better solution could be found.
Well, I think I have come up with one: after this commit, Entries
no longer have their own lock, and instead accesses to Entry members
are almost always atomic; and there is now a case where we spin inside
Variable::_NotifyLocked as well as one in Entry::_RemoveFromVariable.
This leads to somewhat simpler code (no more lock/unlock dance in Notify),
though it is significantly more difficult to understand the nuances of it,
so I have left a sizable number of comments explaining the intricacies
of the new logic.
Note: I initially tried 1000 for "tries", but on a few instances I did see
the panic hit, strangely. I don't think the code that is waited on can
be reasonably reduced any further, so I have just increased the limit to
10000 (which is still well below what spinlocks use.) Hopefully this suffices.
Quick benchmark, x86, compiling HaikuDepot and the mime_db in VMware, 2 cores:
before:
real 0m23.627s
user 0m25.152s
sys 0m7.319s
after:
real 0m23.962s
user 0m25.229s
sys 0m7.330s
Though I occasionally I saw sys times as low as 7.171s, so this seems
to be at least not a regression if not a definitive improvement.
Change-Id: Id042947976885cd5c1433cc4290bdf41b01ed10e
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4727
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Reviewed-by: Alex von Gluck IV <kallisti5@unixzen.com>
GCC still assumes that the dirent has no data past the end for some
scenarios here and still mis-optimizes things. Therefore, drop the
usages of unions altogether, and instead use a casted character array.
Additionally, use B_FILE_NAME_LENGTH for the array, not B_PATH_NAME_LENGTH,
and make sure to add 1 for the NULL terminator.
The lock entry is the first thing in the struct, so this is a no-op
change, but it is safer to do in case of changes, of course.
Spinlocks have been structures for quite a long time, so this was
probably just missed in the conversion.
we don't sample if the last sample is too recent and use the cached result.
Change-Id: I17ed29bda7fe7276f1a4148b3e1985c9d32ae032
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4101
Reviewed-by: Jérôme Duval <jerome.duval@gmail.com>
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
This file should ideally contain only those things needed
across all system headers, even POSIX ones, and all other
declarations (B_* ones especially) should go in SupportDefs.h.
However, as nothing but riscv64 uses this right now, I've just
moved it to there.
GCC 11 treats [1] as a fixed-length array and not a flexible-length
array, and so some things that used direct strcmp("..", ent->d_name),
for instance, would be optimized out as being always unequal,
which was the cause of #17389. Using a real FLA informs GCC that
there is going to be more than one byte of data, and thus this
fixes that bug.
BeOS used [1] and not [0], possibly because it had to deal with
compilers (MetroWerks? Early GCC2?) that did not support FLAs.
GCC 2.95 does, using [0], and GCC 4 does, using [], so we can go
with that here.
(I did try using [0] for both, which seems to be OK with GCC 11,
but GCC 8 throws errors when d_name is dereferenced directly
as being-out-of-bounds. So, we have to use the #if here and give
newer GCC the [] syntax and not [0] to avoid that problem.)
The real question probably is whether or not we should backport
some variant of these changes to R1/beta3, as software at HaikuPorts
very well may run in to the same issue. (The alternative workaround
is to compile with -O1 and not -O2 for any affected software.) But
maybe this is an argument for keeping with the beta4 schedule of
this coming January...
At present, it does, but that is an oddity we have preserved from BeOS
that the next commit is going to remove. (This commit thus wastes 1 byte
without the following one.)
Most changes are pretty straightforward: only a +1 is needed,
and a few removed from sizing calculations. Some filesystems like UDF
originally passed back the length with the \0 included, so they have
been adjusted further. UFS2 had some other sizing problems which are also
corrected in this commit.
Our dirent structure is "slim": it has a flexible-length array at the
end which must be allocated to whatever size the consumer wants. However,
we use [1] there and not [0] or [], which meant GCC thought it was not
a flexible-length array, and so it optimized various string accesses
that it assumed must be always false. Among these was BDirectory's
check for "." and "..", and so that resulted in infinite loops.
When changing our dirent structure to a proper FLA instead of [1],
GCC then throws errors on LongDirEntry as it has data "after" the
FLA; which is what we want, but there is no way to tell GCC that.
So now we use a union instead, which is the proper way to statically
allocate a FLA.
This is part of #17389, but the real fix requires changing our dirent
structure, which is coming in a separate commit.
As suggested by PulkoMandy. This was done before my commits yesterday,
but those just reverted patches that had only been in since May,
so it's not clear how much this is actually needed. Nonetheless
it seems like the more correct thing to do.
While FreeBSD and glibc feature-guard it, they also feature-guard
a lot of other things that we don't, and musl does not guard it,
so it seems more than safe enough to leave it unguarded.
Fixes compilation errors with GCC 11. (The other possible solution
was including features.h in more places, but this seems simpler.)
* A few things need alignment, instead of forcing them all
to align themselves, support alignment of the kernel_args
* Default of 1 is "no alignment"
Change-Id: Iff05dcec8adaa963c8444d701464ea11616062f6
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4698
Reviewed-by: Alex von Gluck IV <kallisti5@unixzen.com>
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
* Catch errors and report them in bus parsing code
* Align the FDT kernel_arg to 8-bytes
* we still choose BSD-2-clause :-)
Change-Id: If2a88b7f131025ff1c1a2d903ed52f039e5bbcb5
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4694
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
Reviewed-by: Alex von Gluck IV <kallisti5@unixzen.com>
In most cases we don't need to use the complete display_mode struct and
we just need the timings. This will avoid future confusion between the
virtual width/height and the actual display timings, if we implement
scrolling someday.
Change-Id: I6c4430b84130b956a47ea0a01afb0843f5a34fd2
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4665
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
This way it becomes much easier to write multiple console implementations
in one bootloader.
Tested for bios_ia32 and efi.
Change-Id: I67134f5c3de109b15d46898864ba7f51c6592afc
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4642
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
* Delete dropped out networks.
* Add in newly discovered networks.
* Add static (aka class) compare method to WirelessNetworkMenuItem
that is used to sort items by signal strength descending.
Add == operator to wireless_network struct to determine if
existing items have a known network attached.
Remove the non-network items from the menu, save them, sort
network menu items, then add non-network items back into the
menu.
Update NetworkStatus preflet to use same compare method as Network
preflet. signal_strength_compare function had a bool return value
instead of int which worked to sort items the first time, but does
not work on successive compares.
By not deleting and recreating the menu items each Pulse(),
the Network preflet no longer crashes on update. The menu flashes
on update still but doesn't crash.
Fixes#12024
Change-Id: Ie5b22cea4e66350b9c5df8e3b8de266ede50ad6d
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4243
Reviewed-by: John Scipione <jscipione@gmail.com>
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Reviewed-by: Axel Dörfler <axeld@pinc-software.de>
... methods which call the respective methods in BList.
These convinience methods allow you to sort a menu of menu items
via a compare function, swap two menu items, or move a menu item
to a new index. Update items layout if menu is open.
Previously there was no easy way to rearrange menu items in a menu.
Change-Id: Ice3d6e5404e895196d8bd32d696dce7c55bd72d4
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4296
Reviewed-by: John Scipione <jscipione@gmail.com>
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
This cuts out almost 40,000 lines of these headers. (I did something similar
in the atheroswifi AR93xx/94xx driver when importing it from FreeBSD,
which had a lot more than 40,000 lines.)
The code in this module was derived from the one in driver/tty. However,
the driver uses a shared lock between the master and slave side of a
TTY, and this was changed to use two separate locks. The approach with
two locks does not work. It seems the change was unfinished and the
second TTY was never locked. But attempting to lock it will result in
lock inversion problems, unless we do complicated things (try to find
which of the two TTY is the master side, and lock that first, for
example). It is simpler to restore the shared lock as used in the
driver.
To set up the shared lock, I modified the tty_create function to take a
pointer to the master TTY when creating the slave. Maybe it makes more
sense to create both sides in the same call, create_tty_pair?
However, this does not work as easily as I wanted, because there is some
recursion going on: at least in one case, the tty_control function is
calling the driver's tty_service function, which in turns attempts to
call back into tty_control for the "other side" TTY. To handle this
case, replace the mutex with a recursive_lock.
Fixes#17091, where the root problem was access to
other_tty->select_pool without locking. This was also made unconvenient
to debug because select_pool objects are self-deleting, when the last
item in the pool is removed. As a result, the code accessing it without
log would suddenly find out that the data it was accessing had been
freed and erased.
This also makes the TTY code in driver/tty and generic/tty a bit more
similar than it was before, and brings us one step closer to merging the
two together. There are still two main differences and I don't know
enough about TTY to decide if they are important, and which version
should be kept:
- The driver has extra code for "background" read and write. I don't
know what this is used for.
- The driver has a single "settings" instance shared by a master and
slave TTY, while the module has two separate instances, but seems to
copy one to the other. I'm not sure which approach is correct.
Change-Id: Ie2daddd027859ce32ba395af76b4f109f8b984b0
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4604
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
These will be needed to implement custom modes in the VESA driver.
Change-Id: I9b52de691baa14e1f1a3ccce500ced9bb040b113
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4622
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Now that it is not used anywhere in the source tree following
previous commits.
Change-Id: Id2fc417a0658d09148e99587c613a928f1fbe4c2
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4611
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
This reverts commit 3e8376c6dd.
Reason for revert: Bootloader currently fails to load kernel
It should be added back once the kernel can start.
Change-Id: Iebefbf8681aff4dff09cef7b7eb832b61f7789c7
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4579
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Reviewed-by: Alex von Gluck IV <kallisti5@unixzen.com>
Someone on the internet found out gcc only understand posix_memalign.
The alloc_align attribute may be applied to a function that returns
a pointer and takes at least one argument of an integer or enumerated
type. It indicates that the returned pointer is aligned on a boundary
given by the function argument at position.
Change-Id: I4b0af6ef3020da1fb460652117286193d5d72f1e
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4514
Reviewed-by: Fredrik Holmqvist <fredrik.holmqvist@gmail.com>
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Change-Id: Ifadd47204be1ec688017a567d43dca38c80bd1df
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4431
Reviewed-by: Alex von Gluck IV <kallisti5@unixzen.com>
Reviewed-by: Fredrik Holmqvist <fredrik.holmqvist@gmail.com>
Set first stack frame return address to
<commpage>commpage_thread_exit, so it will be called
when thread entry point returns.
Change-Id: Ide5cde8d4501eb7241e03ff4052174e984e78870
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4493
Reviewed-by: Alex von Gluck IV <kallisti5@unixzen.com>
