Page MenuHomeFreeBSD

Add a tunable which changes mincore(2) algorithm to only report data from the local mapping.
ClosedPublic

Authored by kib on Jan 7 2019, 12:15 AM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Jan 7 2019, 12:15 AM
kib updated this revision to Diff 52610.Jan 7 2019, 12:21 AM

Further simplify, m cannot be non-NULL when mincore_mapped is true.

markj accepted this revision.Jan 7 2019, 12:55 AM

The mincore(2) man page should be updated as well.

Should we bother keeping the old behaviour at all if Linux does not?

sys/vm/vm_mmap.c
814 ↗(On Diff #52610)

Maybe something like:

/*
 * We only care about this pmap's mapping of the page, if any.
 */

to match the other comments?

This revision is now accepted and ready to land.Jan 7 2019, 12:55 AM
kib updated this revision to Diff 52611.Jan 7 2019, 1:13 AM

Add comment, update man page.

This revision now requires review to proceed.Jan 7 2019, 1:13 AM
kib added a comment.Jan 7 2019, 1:18 AM

Should we bother keeping the old behaviour at all if Linux does not?

I believe that yes, old behavior is useful for introspection tools. This is demonstrated by the article which causes that Linux commit. So even if somebody does not want that, others might do.

markj accepted this revision.Jan 7 2019, 1:30 AM
markj added inline comments.
lib/libc/sys/mincore.2
50 ↗(On Diff #52611)

This isn't really accurate: now it only determines whether the page is mapped.

92 ↗(On Diff #52611)

"mappings of the pages in the specified virtual address range"

93 ↗(On Diff #52611)

"the system"

97 ↗(On Diff #52611)

I'd be explicit here and add, "Otherwise, all mappings of resident pages in the specified address range are examined."

This revision is now accepted and ready to land.Jan 7 2019, 1:30 AM
kib updated this revision to Diff 52614.Jan 7 2019, 1:57 AM

More man page editing.

This revision now requires review to proceed.Jan 7 2019, 1:57 AM
markj accepted this revision.Jan 7 2019, 3:58 AM
markj added inline comments.
lib/libc/sys/mincore.2
50 ↗(On Diff #52611)

I think "a sysctl value" or "depending on the value of .Va vm.mincore_mapped" would sound more natural.

This revision is now accepted and ready to land.Jan 7 2019, 3:58 AM
kib marked an inline comment as done.Jan 7 2019, 4:16 AM
kib updated this revision to Diff 52619.

I also updated the last sentence in the DESCRIPTION section that tries to explain the '0' sysctl operations. It was very confusing by referencing mappings where the real algorithm examines pages regardless of mappings presence.

This revision now requires review to proceed.Jan 7 2019, 4:16 AM
markj accepted this revision.Jan 7 2019, 4:29 AM
In D18764#400393, @kib wrote:

I also updated the last sentence in the DESCRIPTION section that tries to explain the '0' sysctl operations. It was very confusing by referencing mappings where the real algorithm examines pages regardless of mappings presence.

Indeed, sorry.

This revision is now accepted and ready to land.Jan 7 2019, 4:29 AM
kib edited the summary of this revision. (Show Details)Jan 7 2019, 5:18 PM
lattera-gmail.com added inline comments.
sys/vm/vm_mmap.c
102 ↗(On Diff #52619)

Given that there is a security context for this change, would it make sense to make use of CTLFLAG_SECURE for this sysctl node?

This revision was automatically updated to reflect the committed changes.