Page MenuHomeFreeBSD

factor out common wiring code
ClosedPublic

Authored by dougm on Jul 2 2019, 7:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 6, 6:46 PM
Unknown Object (File)
Fri, Mar 1, 5:07 PM
Unknown Object (File)
Feb 25 2024, 2:18 AM
Unknown Object (File)
Feb 15 2024, 1:34 AM
Unknown Object (File)
Feb 14 2024, 9:15 PM
Unknown Object (File)
Feb 10 2024, 1:40 AM
Unknown Object (File)
Feb 2 2024, 12:40 AM
Unknown Object (File)
Jan 19 2024, 1:47 AM
Subscribers

Details

Summary

vm_map_unwire and vm_map_wire_locked have substantial code duplication in the attempt to get the first or next entry not in an IN_TRANSITION state. Define vm_map_entry_in_transition to reduce, but not eliminate entirely, that code duplication. No changes outside of the wiring/unwiring code.

Tested by: pho

Test Plan

Compiles and boots. Passed 14 hours of testing by Peter Holm.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Fatal trap 12: page fault while in kernel mode
cpuid = 0; current process		= 4000 (tmlock)
interrupt enabled, fault virtual address	= 0x20
stack pointer	        = 0x28:0xfffffe00ad822860
resume, IOPL = 0
current process		= 4002 (tmlock)
code segment		= base 0x0, limit 0xfffff, type 0x1b
code segment		= base 0x0, limit 0xfffff, type 0x1b
			= DPL 0, pres 1, long 1, def32 0, gran 1
trap number		= 12
panic: page fault
cpuid = 11
time = 1562057287
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00ac311520
vpanic() at vpanic+0x19d/frame 0xfffffe00ac311570
panic() at panic+0x43/frame 0xfffffe00ac3115d0
trap_fatal() at trap_fatal+0x39c/frame 0xfffffe00ac311630
trap_pfault() at trap_pfault+0x62/frame 0xfffffe00ac311680
trap() at trap+0x2b4/frame 0xfffffe00ac311790
calltrap() at calltrap+0x8/frame 0xfffffe00ac311790
--- trap 0xc, rip = 0xffffffff80f0db15, rsp = 0xfffffe00ac311860, rbp = 0xfffffe00ac311910 ---
vm_map_wire_locked() at vm_map_wire_locked+0x155/frame 0xfffffe00ac311910
vm_map_wire() at vm_map_wire+0x40/frame 0xfffffe00ac311940
kern_mlock() at kern_mlock+0x179/frame 0xfffffe00ac311990
amd64_syscall() at amd64_syscall+0x291/frame 0xfffffe00ac311ab0

https://people.freebsd.org/~pho/stress/log/dougm046.txt

Correct handling of initial failures in map_wire_locked, map_unlock.

Add necesssary updates to 'last_timestamp' in vm_map_wire_locked().

Combine the two last_timestamp updates into one.

I ran tests on D20833.59317.diff for 14 hours. LGTM.

dougm edited the test plan for this revision. (Show Details)
dougm added reviewers: alc, kib, markj.
vm_map.c
2846 ↗(On Diff #59317)

Typo: "unacceptable"

2942 ↗(On Diff #59317)

Extra parentheses.

2967 ↗(On Diff #59317)

I think you can just write if (!vm_map_lookup_entry(...)) like you do elsewhere in this diff, and get rid of result.

Resolve conflicts with parts already checked in. Drop some goto-elimination in vm_map_wire_locked.

dougm marked 3 inline comments as done.
vm_map.c
2855 ↗(On Diff #59418)

I almost always dislike the reuse of the function arguments as locals. Would use of different name for the argument allow to eliminate first_call bool as well ?

2874 ↗(On Diff #59418)

this assumes that vm_offset_t is same as unsigned long ?

2881 ↗(On Diff #59418)

I think if () proven to not be useful, I suggest removing it and converting the comment to one-liner.

2883 ↗(On Diff #59418)

Fix style, '+' should have spaces around it.

Address reviewer comments.

dougm marked 4 inline comments as done.Jul 4 2019, 8:03 PM

I would refactor this differently. I would create a helper function to handle the "in transition" case, in other words, the body of the following "if" statement.

                if (entry->eflags & MAP_ENTRY_IN_TRANSITION) {
                        /*                                                                                                                                                          
                         * We have not yet clipped the entry.                                                                                                                       
                         */
...
                }

Apply reviewer suggestion, unhappily.

vm_map.c
2846 ↗(On Diff #59430)

I would call it vm_map_entry_in_transition ...

2853 ↗(On Diff #59430)

... and asserted the map lock and MAP_ENTRY_IN_TRANSITION flag there.

2944 ↗(On Diff #59430)

Commit this separately. You can change line split, moving the entry->end < end expression to the previous line.

3240 ↗(On Diff #59430)

When you break out of the loop due to tmp_entry == NULL and setting rv == KERN_INVALID_ADDRESS, wouldn't this assignment obliterate rv ? Before, that break was 'goto done'.

Ran a brief test (5 hours) on D20833.59430.diff. No problems seen.

dougm marked 4 inline comments as done.

Apply reviewer suggestions.

sys/vm/vm_map.c
2854 ↗(On Diff #59446)

I still suggest to add assert about the map lock.

3173 ↗(On Diff #59446)

Why did you moved the last_timestamp memorization ?

dougm marked 2 inline comments as done.

Add a locked-assertion.

sys/vm/vm_map.c
3173 ↗(On Diff #59446)

If I simply move the assignment back to where it was, the compiler will complain that last_timestamp is uninitialized in line 3188. I could fix that by inserting another assignment above - like after 'first_pass = FALSE'.

last_timestamp is only used in this wired_count==0 block, so there's no point in setting it elsewhere, or updating the value for anywhere else in the code.

This revision is now accepted and ready to land.Jul 5 2019, 9:00 PM
vm_map.c
2862 ↗(On Diff #59452)

This comment is wrong - the wait is not interruptible. I think the previous comment meant to suggest a possible improvement.

vm_map.c
2862 ↗(On Diff #59452)

The comment lacks a question mark at the end, instead of dot.

Change '.' to '?' in a comment.

This revision now requires review to proceed.Jul 6 2019, 3:25 AM

I tested D20833.59460.diff for 5 hours without seeing any problems.

vm_map.c
2862 ↗(On Diff #59452)

... and also why the comment appeared inside the body of an otherwise empty "if".

vm_map.c
2862 ↗(On Diff #59452)

The formatting and presentation of this comment has changed only as a result of reviewer feedback. I'm happy to leave it in whatever state pleases everyone else. I don't know what that state is.

vm_map.c
2930 ↗(On Diff #59460)

I'm puzzled by this. There may be multiple entries within the range [start, end) and it appears that the flag first_pass is being set to false just because we are past the first entry.

vm_map.c
2930 ↗(On Diff #59460)

That's right. That determines whether we trust the value of 'first_pass' or NULL it and force recalculation later.

vm_map.c
2930 ↗(On Diff #59460)

Then, perhaps, "pass" is the wrong word. I interpret a "pass" as completing iteration over all entries in the range [start, end).

The motivation for this change is that I was working on another change that would do something to every function that simplified entries, and when I got to the wiring/unwiring functions, I found that it was hard to do and that I'd have to do it twice. So I concocted this patch so that I'd only have to do it once. I'm sorry that it seems I just mess with your code for no real purpose other than to annoy you.

Change a misleading variable name. Drop a contentious comment.

This revision is now accepted and ready to land.Jul 7 2019, 8:56 AM

I have started a brief test run on i386 with D20833.59496.diff.

I ran tests for 5 hours on i386 with D20833.59496.diff. No problems seen.

Came across this panic:

20190708 06:25:35 all (313/652): sort.sh
swp_pager_getswapspace(8): failed
swp_pager_getswapspace(4): failed
swp_pager_getswapspace(32): failed
swp_pager_getswapspace(32): failed
swp_pager_getswapspace(32): failed
Jul  8 06:32:26 mercat1 kernel: pid 72536 (sort), jid 0, uid 0, was killed: out of swap space
swp_pager_getswapspace(18): failed
panic: freeing free block: ffff40, size 8, mask 1
cpuid = 9
time = 1562560350
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe10a12b8270
vpanic() at vpanic+0x19d/frame 0xfffffe10a12b82c0
panic() at panic+0x43/frame 0xfffffe10a12b8320
blst_meta_free() at blst_meta_free+0x12b/frame 0xfffffe10a12b8360
blst_meta_free() at blst_meta_free+0x102/frame 0xfffffe10a12b83a0
blst_meta_free() at blst_meta_free+0x102/frame 0xfffffe10a12b83e0
blst_meta_free() at blst_meta_free+0x102/frame 0xfffffe10a12b8420
blst_meta_free() at blst_meta_free+0x102/frame 0xfffffe10a12b8460
blist_free() at blist_free+0x2e/frame 0xfffffe10a12b8480
swp_pager_freeswapspace() at swp_pager_freeswapspace+0x8a/frame 0xfffffe10a12b84a0
swp_pager_meta_free_all() at swp_pager_meta_free_all+0xbb/frame 0xfffffe10a12b84f0
swap_pager_dealloc() at swap_pager_dealloc+0x115/frame 0xfffffe10a12b8510
vm_object_terminate() at vm_object_terminate+0x27b/frame 0xfffffe10a12b8560
vm_object_deallocate() at vm_object_deallocate+0x412/frame 0xfffffe10a12b85c0
vm_map_process_deferred() at vm_map_process_deferred+0x7f/frame 0xfffffe10a12b85e0
vm_map_remove() at vm_map_remove+0xc6/frame 0xfffffe10a12b8610
vmspace_exit() at vmspace_exit+0xd3/frame 0xfffffe10a12b8650
exit1() at exit1+0x5ad/frame 0xfffffe10a12b86c0
sigexit() at sigexit+0xdaf/frame 0xfffffe10a12b89a0
postsig() at postsig+0x336/frame 0xfffffe10a12b8a70
ast() at ast+0x4c7/frame 0xfffffe10a12b8ab0
doreti_ast() at doreti_ast+0x1f/frame 0x7fffffffded0

https://people.freebsd.org/~pho/stress/log/sort.txt

The obvious questions are:
Does this happen without the patch in place?
Does this happen before r349777?

The obvious questions are:
Does this happen without the patch in place?
Does this happen before r349777?

I currently working in getting some gdb info from the current kernel. I'll then try with a pristine r349777 and then go back in time. Note that this is a "options DEBUG_MEMGUARD" build.

So, this is unrelated to D20833.

panic: freeing free block: ffff40, size 16, mask 1
cpuid = 7
time = 1562577077
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe102d97e080
vpanic() at vpanic+0x19d/frame 0xfffffe102d97e0d0
panic() at panic+0x43/frame 0xfffffe102d97e130
blst_leaf_free() at blst_leaf_free+0x7c/frame 0xfffffe102d97e160
blst_meta_free() at blst_meta_free+0x44/frame 0xfffffe102d97e1c0
blst_meta_free() at blst_meta_free+0x158/frame 0xfffffe102d97e220
blst_meta_free() at blst_meta_free+0x158/frame 0xfffffe102d97e280
blst_meta_free() at blst_meta_free+0x158/frame 0xfffffe102d97e2e0
blst_meta_free() at blst_meta_free+0x158/frame 0xfffffe102d97e340
blist_free() at blist_free+0xa1/frame 0xfffffe102d97e370
swp_pager_freeswapspace() at swp_pager_freeswapspace+0xb0/frame 0xfffffe102d97e3a0
swp_pager_update_freerange() at swp_pager_update_freerange+0x52/frame 0xfffffe102d97e3d0
swp_pager_meta_free_all() at swp_pager_meta_free_all+0xdc/frame 0xfffffe102d97e410
swap_pager_dealloc() at swap_pager_dealloc+0x20d/frame 0xfffffe102d97e440
vm_object_terminate() at vm_object_terminate+0x2a5/frame 0xfffffe102d97e490
vm_object_deallocate() at vm_object_deallocate+0x83b/frame 0xfffffe102d97e500
vm_map_entry_deallocate() at vm_map_entry_deallocate+0x2f/frame 0xfffffe102d97e530
vm_map_process_deferred() at vm_map_process_deferred+0x172/frame 0xfffffe102d97e570
_vm_map_unlock() at _vm_map_unlock+0x7a/frame 0xfffffe102d97e5a0
vm_map_remove() at vm_map_remove+0xbe/frame 0xfffffe102d97e5e0
vmspace_dofree() at vmspace_dofree+0x3f/frame 0xfffffe102d97e610
vmspace_exit() at vmspace_exit+0x154/frame 0xfffffe102d97e650
exit1() at exit1+0x5ad/frame 0xfffffe102d97e6c0
sigexit() at sigexit+0xdaf/frame 0xfffffe102d97e9a0
postsig() at postsig+0x336/frame 0xfffffe102d97ea70
ast() at ast+0x4c7/frame 0xfffffe102d97eab0
doreti_ast() at doreti_ast+0x1f/frame 0x7fffffffded0
KDB: enter: panic
[ thread pid 3567 tid 100246 ]
Stopped at      kdb_enter+0x3b: movq    $0,kdb_why
db> x/s version
version:        FreeBSD 13.0-CURRENT #3 r349777: Mon Jul  8 10:58:08 CEST 2019\012    pho@mercat1.netperf.freebsd.org:/usr/src/sys/amd64/compile/MEMGUARD\012
db>

I have updated sort.txt with some debug info. I'll attempt a binary commit search for this problem.

Rename a variable. Restore a comment.

This revision now requires review to proceed.Jul 14 2019, 11:21 PM
sys/vm/vm_map.c
2842–2843 ↗(On Diff #59747)

This comment misses the big picture. The "getting" is really a side-effect of having to release the map lock. Specifically, while we slept, the entry could have been mangled or even destroyed (as described in a comment below). That is why we perform the lookup if the time stamp shows that the map has changed in some way. However, the expected case is that the in-transition entry still exists.

2851 ↗(On Diff #59747)

vm_map.h's definition of the timestamp in struct vm_map uses the shorter spelling "u_int". I suggest that you change this to match.

2925 ↗(On Diff #59747)

This looks wrong. Specifically, the pointer "entry" may no longer point to a valid map entry after sleeping in vm_map_entry_in_transition().

3131 ↗(On Diff #59747)

Ditto.

dougm marked 4 inline comments as done.

Act on reviewer feedback.

sys/vm/vm_map.c
2851 ↗(On Diff #59747)

Will do, even most other definitions of last_timestamp in *this* file use 'unsigned int'.

A have completed a full stress2 test of D20833.59786.diff without seeing any problems.

I don't see any correctness issues with this version.

vm_map.c
2842 ↗(On Diff #59786)

The following is true: "Release and reacquire the map lock." But, it misses the more important aspect of what's happening here. We are sleeping until the given map entry is no longer in transition.

sys/vm/vm_map.c
2842 ↗(On Diff #59932)

The "unlock" and "sleep" happen atomically, so I would suggest. "... map lock, and sleep until ..."

2843 ↗(On Diff #59932)

You have a mix of two spaces and one space after periods. For consistency, I would suggest adding a second space here: "... map lock. If the ..."

This revision is now accepted and ready to land.Jul 19 2019, 7:56 PM