Sponsored by: Dell EMC Isilon
Details
Details
Significant reduction in vm_object lock contention shown.
Seems to fit with: https://reviews.freebsd.org/D23847
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 33891
Event Timeline
Comment Actions
Could you please upload with extra context? Add -U99999 if you are uploading a raw diff, per https://wiki.freebsd.org/Phabricator .
| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3613 | There is a vm_page_grab_pages_unlocked() that handles runs of multiple pages, did you consider using it? | |
| 3617 | I think the gotos should be replaced with a plain return statement. | |
| 3621 | Note that the shared busy state on its own does not block a concurrent vm_page_invalid() call on the page. I'm not sure if this matters much since in the previous implementation results were stale as soon as the object lock is dropped. | |
| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3613 | I have decided against that, because it will block other users for longer - iff somebody wants an exclusive busy on the first page, while we are looking through the results. This way also preserved more of the original code flow. | |
| 3617 | I was trying to preserve as much of the original code as possible, but I can rework it. | |
| 3621 | Yeah - this interface is completely advisory. My goal was to make it as unimpactful as possible - hence the single page busy above as well... There is an argument for changing this to to a VM_ALLOC_WIRED above and return the pages back to PQ_ACTIVE (because why would you check inmem if you're not planning to use it) ... but I didn't want to add side-effects. | |
Comment Actions
This is fine.
But since completely advisory (it is used to avoid unneeded read-ahead), you can avoid sbusy as well, I believe. False-negatives should be safe.
You would remove RLOCK as in patch, and use vm_radix_lookup_unlocked. Then after the check for validity you can re-check with vm_radix_lookup_unlocked() that the page is still within object.
In fact I think that it would not give false negatives because the vnode lock is owned.
| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3617 | The diff is already quite small, I don't think it hurts anything to simplify the function a bit. | |
| 3621 | You don't need to wire the page if it is busied. It would be reasonable to call vm_page_reference() before unbusying the page. If the page is already in PQ_ACTIVE, then the extra reference will be counted next time the page is visited. If the reference bit is observed during a PQ_INACTIVE scan, the page will either be requeued or moved to PQ_ACTIVE depending on whether it might be mapped. | |
| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3620 | I think this can move to the end of the loop body. | |
Comment Actions
I had committed this prematurely and now reverted.
Building /usr/obj/usr/src/i386.i386/sys/SARAH/vfs_bio.o
--- vfs_bio.o ---
/usr/src/sys/kern/vfs_bio.c:3614:7: error: implicit declaration of function 'vm_page_lookup_unlocked' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
m = vm_page_lookup_unlocked(obj, OFF_TO_IDX(off + toff));
^
/usr/src/sys/kern/vfs_bio.c:3614:7: note: did you mean 'vm_map_lookup_locked'?
/usr/src/sys/vm/vm_map.h:480:5: note: 'vm_map_lookup_locked' declared here
int vm_map_lookup_locked(vm_map_t *, vm_offset_t, vm_prot_t, vm_map_entry_t *, vm_object_t *,
^
/usr/src/sys/kern/vfs_bio.c:3614:5: error: incompatible integer to pointer conversion assigning to 'vm_page_t' (aka 'struct vm_page *') from 'int' [-Werror,-Wint-conversion]
m = vm_page_lookup_unlocked(obj, OFF_TO_IDX(off + toff));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/src/sys/kern/vfs_bio.c:3624:5: error: incompatible integer to pointer conversion assigning to 'vm_page_t' (aka 'struct vm_page *') from 'int' [-Werror,-Wint-conversion]
n = vm_page_lookup_unlocked(obj, OFF_TO_IDX(off + toff));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
*** [vfs_bio.o] Error code 1| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3601 | Ravi Pokala pointed out that this changed to bool but the return values did change to true/false. | |
| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3601 | did NOT | |
| sys/kern/vfs_bio.c | ||
|---|---|---|
| 3614 | (To be clear, the problem is that there is no vm_page_lookup_unlocked() yet. It should be straightforward to add. I would prefer that to using a naked vm_radix_lookup_unlocked() call here.) | |