Page MenuHomeFreeBSD
Differential D20833

factor out common wiring code
ClosedPublic
Actions

Authored by dougm on Jul 2 2019, 7:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 11, 7:18 AM
Unknown Object (File)
Fri, Mar 21, 2:10 PM
Unknown Object (File)
Mon, Mar 17, 2:24 PM
Unknown Object (File)
Mar 9 2025, 12:32 AM
Unknown Object (File)
Mar 8 2025, 1:11 PM
Unknown Object (File)
Mar 5 2025, 8:25 PM
Unknown Object (File)
Mar 2 2025, 4:59 AM
Unknown Object (File)
Feb 9 2025, 11:50 AM
View All Files
Subscribers
imp
pho

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
pho added a comment.Jul 2 2019, 8:57 AM
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

dougm updated this revision to Diff 59314.Jul 2 2019, 3:21 PM

Correct handling of initial failures in map_wire_locked, map_unlock.

dougm updated this revision to Diff 59316.Jul 2 2019, 4:06 PM

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

dougm updated this revision to Diff 59317.Jul 2 2019, 4:10 PM

Combine the two last_timestamp updates into one.

pho added a comment.Jul 3 2019, 7:01 AM

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
2844

Typo: "unacceptable"

2946

Extra parentheses.

2962–2963

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

dougm updated this revision to Diff 59415.Jul 4 2019, 7:40 PM

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
2855

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

this assumes that vm_offset_t is same as unsigned long ?

2881

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

2883

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

dougm updated this revision to Diff 59422.Jul 4 2019, 8:03 PM

Address reviewer comments.

dougm marked 4 inline comments as done.Jul 4 2019, 8:03 PM
alc added a comment.Jul 4 2019, 10:07 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.                                                                                                                       
                         */
...
                }
dougm updated this revision to Diff 59430.Jul 5 2019, 3:10 AM

Apply reviewer suggestion, unhappily.

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

I would call it vm_map_entry_in_transition ...

2853

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

2947

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

3243

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

pho added a comment.Jul 5 2019, 11:11 AM

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

dougm updated this revision to Diff 59446.Jul 5 2019, 3:25 PM
dougm marked 4 inline comments as done.

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 ?

dougm updated this revision to Diff 59452.Jul 5 2019, 8:38 PM
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.

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
2862

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
2862

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

dougm updated this revision to Diff 59460.Jul 6 2019, 3:25 AM

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

This revision now requires review to proceed.Jul 6 2019, 3:25 AM
pho added a comment.Jul 6 2019, 12:24 PM

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
2862

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

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
2930

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
2930

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
2930

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

dougm added a comment.Jul 6 2019, 6:23 PM

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.

dougm updated this revision to Diff 59496.Jul 7 2019, 7:20 AM

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
pho added a comment.Jul 7 2019, 9:13 AM

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

pho added a comment.Jul 7 2019, 12:48 PM

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

pho added a comment.Jul 8 2019, 5:50 AM

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

dougm added a comment.Jul 8 2019, 8:23 AM

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

pho added a comment.Jul 8 2019, 8:29 AM
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.

pho added a comment.Jul 8 2019, 9:34 AM

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
dougm updated this revision to Diff 59747.Jul 14 2019, 11:21 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.

dougm updated this revision to Diff 59786.Jul 15 2019, 9:37 PM
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'.

pho added a comment.Jul 18 2019, 5:20 AM

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

alc added a comment.Jul 19 2019, 4:09 PM

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

vm_map.c
2842

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.

dougm updated this revision to Diff 59932.Jul 19 2019, 4:23 PM

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

dougm updated this revision to Diff 59938.Jul 19 2019, 7:00 PM

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

Revision Contents
Changeset List

PathSize
152 lines

Diff 59452

View Options

vm_map.c

# $FreeBSD$ # $FreeBSD$
SSP_CFLAGS= SSP_CFLAGS=
NO_WMISSING_VARIABLE_DECLARATIONS= NO_WMISSING_VARIABLE_DECLARATIONS=
.include <src.opts.mk> .include <src.opts.mk>
.if !defined(BUILDING_TESTS) .if !defined(BUILDING_TESTS)
OBJS+= Scrt1.o crt1.o gcrt1.o OBJS+= Scrt1.o crt1.o gcrt1.o
OBJS+= crtbegin.o crtbeginS.o crtbeginT.o OBJS+= crtbegin.o crtbeginS.o crtbeginT.o
OBJS+= crtend.o crtendS.o OBJS+= crtend.o crtendS.o
OBJS+= crti.o crtn.o OBJS+= crti.o crtn.o
CRT1OBJS+= crtbrand.o ignore_init_note.o CRT1OBJS+= crtbrand.o feature_note.o ignore_init_note.o
ACFLAGS+= -DLOCORE ACFLAGS+= -DLOCORE
CFLAGS+= -fno-asynchronous-unwind-tables CFLAGS+= -fno-asynchronous-unwind-tables
CFLAGS+= -fno-omit-frame-pointer CFLAGS+= -fno-omit-frame-pointer
CFLAGS+= -I${.CURDIR:H}/common \ CFLAGS+= -I${.CURDIR:H}/common \
-I${SRCTOP}/lib/libc/include -I${SRCTOP}/lib/libc/include
CFLAGS_CRTS= -DSHARED ${PICFLAG} CFLAGS_CRTS= -DSHARED ${PICFLAG}
FILES= ${OBJS} FILES= ${OBJS}
FILESMODE= ${LIBMODE} FILESMODE= ${LIBMODE}
FILESOWN= ${LIBOWN} FILESOWN= ${LIBOWN}
FILESGRP= ${LIBGRP} FILESGRP= ${LIBGRP}
FILESDIR= ${LIBDIR} FILESDIR= ${LIBDIR}
# These FILES qualify as libraries for the purpose of LIBRARIES_ONLY. # These FILES qualify as libraries for the purpose of LIBRARIES_ONLY.
.undef LIBRARIES_ONLY .undef LIBRARIES_ONLY
CLEANFILES+= ${OBJS} ${CRT1OBJS} crt1_c.o gcrt1_c.o Scrt1_c.o CLEANFILES+= ${OBJS} ${CRT1OBJS} crt1_c.o gcrt1_c.o Scrt1_c.o
CLEANFILES+= crti_s.o
crt1.o: crt1_c.o ${CRT1OBJS} crt1.o: crt1_c.o ${CRT1OBJS}
${LD} ${_LDFLAGS} -o ${.TARGET} -r ${.ALLSRC:M*.o} ${LD} ${_LDFLAGS} -o ${.TARGET} -r ${.ALLSRC:M*.o}
.if ${MACHINE_ARCH} == "i386" .if ${MACHINE_ARCH} == "i386"
${OBJCOPY} --localize-symbol _start1 ${.TARGET} ${OBJCOPY} --localize-symbol _start1 ${.TARGET}
.endif .endif
gcrt1_c.o: crt1_c.c gcrt1_c.o: crt1_c.c
Show All 18 Lines
crtendS.o: crtend.c crtendS.o: crtend.c
crtbegin.o crtend.o crtbeginT.o: crtbegin.o crtend.o crtbeginT.o:
${CC} ${CFLAGS} -I${.CURDIR} -c -o ${.TARGET} ${.ALLSRC:N*.h:[1]} ${CC} ${CFLAGS} -I${.CURDIR} -c -o ${.TARGET} ${.ALLSRC:N*.h:[1]}
crtbeginS.o crtendS.o: crtbeginS.o crtendS.o:
${CC} ${CFLAGS} -I${.CURDIR} ${CFLAGS_CRTS} -c -o ${.TARGET} \ ${CC} ${CFLAGS} -I${.CURDIR} ${CFLAGS_CRTS} -c -o ${.TARGET} \
${.ALLSRC:N*.h:[1]} ${.ALLSRC:N*.h:[1]}
crti_s.o: crti.S
${CC} ${CFLAGS} ${ACFLAGS} -c ${.ALLSRC:M*.S} -o ${.TARGET}
crti.o: crti_s.o crtbrand.o
jhbAuthorUnsubmitted
Done
Inline Actions

Here we link crti_s.o (generated from per-arch crti.S) and crtbrand.o to build crti.o. This is then included in both shared libraries and executables.

jhb: Here we link crti_s.o (generated from per-arch crti.S) and crtbrand.o to build crti.o. This is…
${LD} ${_LDFLAGS} -o ${.TARGET} -r ${.ALLSRC:M*.o}
.endif .endif
.include "../Makefile.inc" .include "../Makefile.inc"