Details

Reviewers

alc
kib
markj

Commits

rS350155: Define vm_map_entry_in_transition to handle an in-transition map

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

Lint

Lint Skipped

Unit

Tests Skipped

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 summary of this revision. (Show Details)Jul 3 2019, 7:12 AM

dougm edited the test plan for this revision. (Show Details)

dougm added reviewers: alc, kib, markj.

markj added inline comments.Jul 4 2019, 6:46 PM

vm_map.c
2846	Typo: "unacceptable"
2942	Extra parentheses.
2967–2968	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 updated this revision to Diff 59418.Jul 4 2019, 7:43 PM

dougm marked 3 inline comments as done.

kib added inline comments.Jul 4 2019, 7:53 PM

vm_map.c
2857	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 ?
2876	this assumes that vm_offset_t is same as unsigned long ?
2883	I think if () proven to not be useful, I suggest removing it and converting the comment to one-liner.
2885	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.

kib added inline comments.Jul 5 2019, 8:44 AM

vm_map.c
2848	I would call it vm_map_entry_in_transition ...
2855	... and asserted the map lock and MAP_ENTRY_IN_TRANSITION flag there.
2943	Commit this separately. You can change line split, moving the `entry->end < end` expression to the previous line.
3231–3232	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.

Apply reviewer suggestions.

kib added inline comments.Jul 5 2019, 8:15 PM

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 ?

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.

kib accepted this revision.Jul 5 2019, 9:00 PM

This revision is now accepted and ready to land.Jul 5 2019, 9:00 PM

markj added inline comments.Jul 5 2019, 9:06 PM

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

kib added inline comments.Jul 5 2019, 9:17 PM

vm_map.c
2864	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.

alc added inline comments.Jul 6 2019, 4:53 PM

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

dougm added inline comments.Jul 6 2019, 5:02 PM

vm_map.c
2864	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.

alc added inline comments.Jul 6 2019, 5:05 PM

vm_map.c
2927	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.

dougm added inline comments.Jul 6 2019, 5:34 PM

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

alc added inline comments.Jul 6 2019, 5:42 PM

vm_map.c
2927	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.

kib accepted this revision.Jul 7 2019, 8:56 AM

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?

In D20833#452429, @dougm wrote:

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.

dougm edited the summary of this revision. (Show Details)Jul 11 2019, 1:35 PM

Rename a variable. Restore a comment.

This revision now requires review to proceed.Jul 14 2019, 11:21 PM

dougm added 1 blocking reviewer(s): alc.Jul 15 2019, 6:10 AM

alc added inline comments.Jul 15 2019, 8:35 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.

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
2844	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.

Improve a comment.

alc added inline comments.Jul 19 2019, 5:51 PM

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 ..."

Edit comments.

alc accepted this revision.Jul 19 2019, 7:56 PM

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

Closed by commit rS350155: Define vm_map_entry_in_transition to handle an in-transition map (authored by dougm). · Explain WhyJul 19 2019, 8:47 PM

This revision was automatically updated to reflect the committed changes.

dougm added a commit: rS350155: Define vm_map_entry_in_transition to handle an in-transition map.

Herald added a subscriber: imp. · View Herald TranscriptJul 19 2019, 8:47 PM

factor out common wiring code
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 59316

vm_map.c

factor out common wiring codeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 59316

vm_map.c

factor out common wiring code
ClosedPublic
Actions

Revision Contents
Changeset List