This reverts a8877df135.
Previously, the "unmergeable" flag was necessary for the RAMFS,
because if the last vnode reference was released while there
was still a consumer (as the old ordering of _RemoveConsumer
had), then the release of the cache reference when the vnode
was removed would result in the cache trying to merge with
its now-only consumer and sole referrer.
Now, instead, we remove the consumer before releasing the store
reference, so that there's no chance the cache will be merged
inside this method.
mmap_cut_tests still pass, web browsers using ramfs shared_memory
still seem to work.
This way, if the Resize() is supposed to take care of the commitment,
it will (and will fail early if it can't), while if we are the ones
responsible for adjusting the commitment, map_backing_store won't
commit at all (avoiding committing far more than will be necessary),
and we can just steal the commitment from the first cache for the second.
This reverts 3a81e9446d (2022).
That commit fixed#17556 by just checking if the area had an
underlying cache that wasn't a RAM cache. But there are cases
where there will be RAM source caches that we have to take
into account, too, not just vnode caches or the like. The
most common example of that would be all areas of a team
after a fork(); the original pages will be in a read-only
source cache.
This commit fixes the real underlying problem: if the first area
has a source cache, then the new second cache needs to have that
as its source, too; and furthermore must have the correct offsets
in order to access its pages correctly.
The test for #17556 that was added in 9ed77019b6
still works as before, as do all the applications I tested that
use cut_area. Some assertion failures that the cut tests triggered
(related to commitment sizes) are fixed by this, as well.
This also seems to fix the remaining instability on fork() in the
boehm-gc's "gctest".
The page_protections aren't changed at all, so all pages that exist
should already have the same protections as are specified in the array.
The only thing different is what cache and area they now belong to, but
the VMTranslationMap does not care about that.
So we don't need to loop over the pages and re-protect them in this case.
We already didn't for all cases where no page_protections were involved.
(It seems this logic was introduced in bdcc293fa8
along with general page_protections support in cut_area.)
When the area has no page_protections but isn't writable,
we also want to use a smaller-than-default commitment.
So, adjust compute_area_page_commitment to handle that case,
and then use it in cut_area where appropriate.
cache->virtual_base is the cache's start address, which no pages will
be found before. area->cache_offset on the other hand is the area's
offset into the cache (i.e. offset 0 in the area will be offset
0 + area->cache_offset in the cache.) These addresses may well be
the same (even if they're not 0), and in many situations they are,
but in situations with shared or cut areas, they may not be.
The only thing that uses this method is madvise(MADV_FREE), which
probably not many things besides the guarded_heap use at present.
We shouldn't return B_OK here, because then the page writer will
assume it's acquired a store ref and can write pages from this
cache, when of course it's done nothing of the sort.
Previously, we'd wind up adding pages from the source to the consumer
that were potentially or actually outside the consumer's bounds.
Now we check the consumer's size and ignore any pages that we don't
want or need; they'll just be freed along with the source cache.
While at it, drop VMAnonymousCache::_MergePagesSmallerSource; it
was the same as the base class's implementation of Merge preceding
this commit; and add a comment to _MergePagesSmallerConsumer noting
that some of the pages may be busy (indeed, I manage to trigger an
assert related to copy-on-write in here at least once.)
I discovered this problem because the page commitment size ASSERT()s
triggered inside Resize() and Rebase(); but the out-of-range pages
already existed in the cache before those functions were called. So,
I've also added an ASSERT to MovePage() that would have caught this
problem more directly.
The original meaning of vfork is "fork, sharing virtual memory" (until
exec). We don't implement that, and may never do so. However, since
calling any functions besides exec() in a vfork'ed child is "undefined
behavior", we can take advantage of that fact at least by not calling
any of the pre- and post-fork hooks, saving a lot of page faults from
copy-on-write.
On one run of the "compile HaikuDepot and the mime_db" benchmark with -j4,
the total waits count on the top two VMCaches by contention dropped
from 62125 and 58927, to 52034 and 41225.
musl apparently does more or less this same thing (vfork() is fork()
but without calling any of the hooks.)
The first was added in 2004 when there wasn't even a branch for
initializing the child; I think this can be considered done now.
The second was added in 2010, but it seems in the meantime we've
decided that reinitializing locks is the best way to make them
consistent after calling fork(), so it's also obsolete.
Otherwise, platform loaders couldn't make heap allocations inside
platform_start_kernel(), which some loaders (e.g. EFI) do.
Implement calling heap_release() for the BIOS loaders at least.
This gets us back the ~1.5MB of bootloader heap memory there.
Rather than setting it from the total count of pages, and then reducing
it by the size of the B_ALREADY_WIRED areas incrementally. This means
that other things allocated in the early boot period (like page tables)
will also be accounted for. The downside is that, if they don't have
a corresponding area, then any pages freed later on won't also unreserve
memory at present; but the early boot page tables likely won't be freed
at all (since they'll be in use; or should have already been freed in the
case of the 32-bit to PAE transition.)
(In the future, we should reserve memory as well as pages for the page
tables, and that will take care of that problem anyway.)
Booting x86_64 in QEMU with 1GB of RAM, the old accounting method produced
an initial (after ALREADY_WIRED accounting) sAvailableMemory of 251,368
total pages, while this new accounting method gives 250,812 instead,
a difference of 556 pages. (Some of that is probably the never-freed
bootloader memory, which I think is around ~360 pages.)
Overall this should reduce the amount of "theoretically available but
actually inaccessible" memory, which should hopefully help with the VM
getting itself into trouble thinking memory is available when it
really isn't.
See inline comment: we can potentially wind up with conflicting mappings,
depending on what the system ACPI firmware tells ACPICA to do, so it's
best if we avoid using non-default types on architectures where they
aren't strictly necessary.
Fixes#19119 and related issues.
Otherwise we may fault later but have no memory to satisfy the fault.
For a compile of HaikuDepot and the mime_db in VMware with -j4, this
seems to increase the wait time on the "available memory" lock from
~0.1s to ~0.5s, and the wait count from ~500 to ~1500 (overall real time
~30s.) Probably we can mitigate that later by doing atomic updates on
sAvailableMemory, at least for releasing memory.
Change-Id: I61abc28d1fc30f7b3d5fd9a2e68e4f4ec960f88d
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8677
Reviewed-by: waddlesplash <[email protected]>
* Fix a silly bug in compute_area_page_commitment that was leading to
the cache's pages not being taken into account at all.
* Don't let VMCache::Resize() and Rebase() alter the commitments,
but rather let us do that. Add assertions that they did not fail.
* Move area->cache_offset increment up so that compute_area_page_commitment
can use it as it needs to.
Fixes assertion failures from boehm-gc tests following the previous commits.
This means that we won't try and change the commitment at all, and it
will be up to the caller to do that instead.
Also move the commitment change from the beginning to the end of Rebase,
matching Resize. This way, we won't trip the new asserts added to Commit()
in the previous commits.
Add a relevant assert to vm_try_reserve_memory to make sure the
negative priority doesn't end up down that far.
Overcommitted caches should only have commitments equal to the
number of pages they actually contain, so we should decommit
whenever pages are discarded.
This changes the API of VMCache::Discard to return an ssize_t
of the size of pages that were discarded (or a negative error on
failure.) Nothing checked the return value besides things in VMCache
itself, it appears; but it apparently never fails, so that's fine.
Also add asserts to Commit() that the new commitment at least
encompasses all pages the cache actually contains.
In copy_on_write_area, the copied cache should have the same overcommit
status as the original area, and in set_memory_protection, we shouldn't
change the committed size at all if the cache is overcommitting (otherwise,
we'd wind up shrinking cache's commit sizes below the actual number of
pages they contained in some cases.)
It seems this method was never renamed when MergeStore was renamed
to Merge all the way back in hrev27179. However, that wound up
working out, because this method also didn't call the base class
implementation that actually merges the page trees properly, so
it wouldn't have worked anyway.
Follow input_device_type above: we don't have _TYPE or _SUBTYPE on
the end, but _POINTING in the middle, because these aren't in a global
"subtype" enumeration, but a B_POINTING_DEVICE-specific enumeration.
Also don't bother adding the UNKNOWN type to messages that have no
type; if it's not included, UNKNOWN is implied. Saves a few CPU cycles.
Change-Id: I9088b9fcee63bf001b43febbe1e3ac17eb1792b4
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8635
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
"Move" now sounds like it has 'move' semantics (i.e. replaces this
structure's data with the other structure's data), while MoveFrom()
really had 'move+append' semantics (appends the other list's elements
to this list, and clears the other list.) To make this clearer, it's
here renamed to "TakeFrom".
This should reduce confusion with the other move-related APIs that
are starting to show up in the Haiku tree (e.g. "MoveFrom" in BRegion.)
Change-Id: Ib0a61a9c12fe8812020efd55a2a0818883883e2a
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8634
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
Reviewed-by: X512 X512 <[email protected]>
This is now necessary after enabling delayed commitments for anonymous
mappings with PROT_NONE.
Change-Id: I33b76f9d9f6a1d560793e523b74e9ac9fd7a4f62
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8676
Reviewed-by: waddlesplash <[email protected]>
These were declared in this header on BeOS, so we need to keep
them around for ABI compatibility, but they are nonstandard
and no other C library besides glibc appears to provide them
at all (not even musl, and none of the BSDs.)
Reduces the size of WiFi drivers by a bit (and reduces the number
of symbols the kernel has to resolve within the binaries.)
Tested with realtekwifi, still works.
The POSIX specification says that the behavior of specifying O_TRUNC
with O_RDONLY is "undefined", but the Linux manpages ominously state
"On many systems the file is actually truncated." I tested this,
and indeed on Linux the file is actually truncated.
This doesn't seem like a very sensible behavior, so in this commit
it's changed to return B_NOT_ALLOWED (EPERM) if those flags are
specified together. The FAT driver already did this, but most other
filesystem drivers just checked write access permissions and
truncated the file anyway; so this is indeed a behavioral change.
Change-Id: If2e76782743ee91d934dc7e0c2f306f37b159a0f
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8625
Reviewed-by: waddlesplash <[email protected]>
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: Axel Dörfler <[email protected]>
And also return an error if the offset of an mmap() request isn't
page-aligned, rather than silently aligning it. libroot already
did this for mmap() itself, so this only affects things that invoke
the operation or syscall directly.
Fixes#19155.
Change-Id: I081dd1492d06f56536c1dbb5d4028345f95c4460
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8622
Reviewed-by: waddlesplash <[email protected]>
Move one test from map_cut_tests, otherwise the other tests are new.
Includes a test for the cause of #19155.
Change-Id: I15abbf11f2c6db7385754825abbcc159414f6fd8
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8631
Reviewed-by: waddlesplash <[email protected]>
* Clarify the fResizeThreshold logic and remove the comment.
* Rename "count" constructor argument to "blockSize", as this is
what it actually does.
No functional change intended.
Change-Id: I993bf0e695f47da181e9fb50b9a964edfd4a0adc
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8629
Reviewed-by: waddlesplash <[email protected]>
There's still a number of tests in the root that should be moved
to other subdirectories, but this at least gets the VM-related ones
into a subdirectory (and removes a stale entry in the VM Jamfile.)
This reverts commit 1db0961121.
It turns out the comment is not obsolete; what it refers to isn't
PAE systems but true 32-bit ones. I'm not sure we should use
64-bit cache offsets even there, but that's a decision for another
time.
We need at least 19 characters on 64-bit architectures:
2 for "0x", 16 for the pointer, and 1 for the \0. So just
use a round 20.
Fixes cut-off pointer values in the display.
That is, if we have a path like "/nonexistent-1/nonexistent-2/file",
we shouldn't report "nonexistent-1" as the "leaf" node, but rather
nothing at all. Otherwise, create_vnode() that expects the result to be a
directory vnode plus a nonexistent file will get confused and try
to create a file "/nonexistent-1" rather than just bailing out.
This fixes#19062. The reproducer in that ticket caused a scenario
much like the above; and since rootfs doesn't support regular files,
it returned "EINVAL" when attempting to create the "/nonexistent" file.
After this change, we get ENOENT as expected.
Change-Id: Ifcaaa858403fb747858800afbf644051bb9913ad
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8621
Reviewed-by: Jérôme Duval <[email protected]>
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
This is an optimization in two ways: first, it allows us to avoid
unlocking the cache or needing the "unreferenced" store-ref acquisition
if we can write the next page in the same cache; and second, the
I/O will be much more tightly combined, as PageWriteTransfer will
be able to merge the iovecs more often and do less I/O (and on
spinning disks, we'll write adjacent regions more often, too.)
Based on some basic logging, this happens very often. I saw adjacent
writes numbers of e.g. 203, 255 (kNumPages), 139, 15, 99, etc. There
were a fair number of 0s, but that case shouldn't add too much overhead
since we bail out very rapidly.
In the case of things like "dd if=/dev/zero of=file ...", this is a
major optimization, since it massively reduces lock contention between
the dd thread and the page_writer thread.
A compile benchmark seems relatively similar, maybe slightly faster.
Otherwise, the FD won't be closed, and then the underlying vnode
won't ever be released, leading to files whose space can't be reclaimed
without rebooting and running checkfs.
* The Be Book specifies that events will be flushed on destruction.
The previous implementation of this class didn't do that, but
we ought to. Unless the queued events have attached buffers or
a cleanup hook, this is a no-op anyway.
* Use a mutex rather than a BLocker for the allocation lock. This
saves a semaphore per queue.
* Put the queue_entry in an anonymous namespace. It shouldn't have
any global symbols anyway, but it doesn't hurt.
The previous implementation allocated and freed event objects
on every insertion and removal using malloc()/free(). It was also
licensed under a "distributions in binary form must reproduce ...
in the binary" license, which is more restrictive than the MIT license
that we prefer.
So, this is a rewrite from scratch. It uses the standard
DoublyLinkedList<> rather than rolling its own, and manages
a free list of event queue objects rather than hitting malloc()
all the time. It only frees chunks on destruction, though,
but that hopefully won't be an issue anyway.
All tests from the TimedEventQueueTest still pass, and media playback
still works as before.
Change-Id: Ia940b6176f8051ae4823b75acd305ded8783d1e0
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8594
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
Previously, lower was better, and higher was worse. But really we want
the scores to be based primarily around the index sizes, which can
grow to be very large, so a maximum score is hard to determine.
Instead, start with the index size, and then divide to make it smaller
based on how "useful" the equation terms will be in searching it.
Improves the performance of queries like those in #19080; according
to humdinger's testing, the query with the most expensive term first
went from ~2.0s execution time down to ~0.7s, same as the query with
the least expensive term first.
Change-Id: Id71fa21c95cfe3d8d0019ff356bdf4935446411f
Reviewed-on: https://review.haiku-os.org/c/haiku/+/8593
Reviewed-by: waddlesplash <[email protected]>
This amends 0f42cf8d7d.
That commit accidentally contained an unrelated patch to the one
described in the commit message, turning a number of prints that
happened unconditionally to ones that happened only for faults
occurring in kernel context. This change re-enables those prints
when TRACE_FAULTS is set (which it isn't by default.)
The original change to reduce prints was done to cut down on syslog spam
from applications like Boehm-GC, the JVM, or emulators that trigger
many (thousands or more) faults on purpose and then catch them in
signal handlers. In the case where these faults are crashes, then
the debug_server will still log those crashes into the syslog anyway.
B_PATH_NAME_LENGTH == PATH_MAX, and PATH_MAX is inclusive of the final
NULL terminator, so we don't need a + 1 here.
The original KPath default was to not use + 1, but that was changed in
42e3c6f978 due to all the consumers that did.
But all those consumers are wrong, it appears; they should just be
using the default length instead. So now we do that.
* If we coerce types inside the switch(), then the "type already converted"
check at the beginning will fail every time, causing us to reconvert,
which is surely bad for performance.
* B_TIME_TYPE should be INT32 or INT64 depending on what its size is.
May help with #19080.
Its values must be shifted before comparing against them.
Also handle the last-modified times correctly in NodeGetLastModifiedTime.
Fixes an issue noticed in #19080 which was a regression from
the query refactor earlier this year.
Also while at it, remove a needless lock in EntryGetName;
Inode::GetName acquires this lock for us.
We pass a NULL destination address, so we need to keep the added
NULL check, but after the previous commit we can now use the
real hook function here.
WiFi still works (tested with realtekwifi.)
This is what FreeBSD does. The default implementation (in this
same file) does what ether_output did before, but the FreeBSD
net80211 stack overrides the method.
IORequest notifications begin by notifying the "finished" condition,
then invoking the request callback, then invoking the parent callback.
vfs_{read|write}_pages wait on the "finished" condition and then
return at once, potentially releasing their references to the vnode
in question, before the request callback is even invoked. The request
callback is what (eventually) invokes iterative_io_finished_hook,
which meant that we were accessing the Inode object after our reference
to it had already been released.
We can't really change IORequest notification order, as any one of the
notifications could delete the request, and indeed the first one does here.
So the solution is just to acquire another reference to the vnode and
release it in the finished hook.
Fixes#19122 and #8405.
* Use do_fd_io and drop the unnecessary iteration callback.
* Make the callback call a non-static function to avoid needing
to fetch values.
* Unlock the cache before calling the I/O function.
Should fix#19259.
If get_vecs returns 0 and no vecs, then we need to quit iterating.
Otherwise we'll just loop forever and leak memory.
Also add an assert in IOBuffer::GetNextVirtualVec that would have caught
this problem, and the memory leak it results in.
We need to hold a reference to the file descriptor throughout
the I/O operation, which do_iterative_fd_io already takes care of.
So just modify it to handle not having the additional callbacks.
Nothing actually uses do_fd_io at present, though it will be
in the next commits.
Augustin Cavalier