Page MenuHomeFreeBSD

When traversing vm_map entries, don't look up the same neighbor twice
ClosedPublic

Authored by dougm on Nov 13 2019, 5:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 8:55 PM
Unknown Object (File)
Fri, Nov 22, 8:44 PM
Unknown Object (File)
Fri, Nov 22, 3:25 PM
Unknown Object (File)
Wed, Nov 20, 11:57 AM
Unknown Object (File)
Tue, Nov 19, 11:16 AM
Unknown Object (File)
Mon, Nov 18, 10:09 AM
Unknown Object (File)
Sat, Nov 2, 6:48 AM
Unknown Object (File)
Sat, Nov 2, 6:06 AM
Subscribers

Details

Summary

Instead of looking up a predecessor or successor to the current map entry, when that entry has been seen already, keep the already-looked-up value in a variable and use that instead of looking it up again.

Define a standard naming convention for map entry iterators in vm_map.c and apply it.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Add parens around returned value in vm_map_entry_first.

This change reduces the size of mac_process.o and vm_map.o by 56 and 656 bytes, respectively, on the amd64 machine I'm testing on.

It would be nice if the loop variable names were more consistent: in some places we use prev_entry/entry, in others it's temp_entry/current or entry/current, etc.. I think I prefer prev_entry/entry, FWIW. Anyway, that does not block this patch.

sys/vm/vm_map.c
2576

Why do we need an opening paren here?

3003

first_entry is a bit of a misnomer now, perhaps it should become prev_entry.

3291

Same comment about changing to prev_entry.

This revision is now accepted and ready to land.Nov 15 2019, 8:37 PM

Rename variables in vm_map loops, so that the fixed starting point is called 'first_entry', the iterator is called 'entry', and the iterator trailing it, if any, is called 'prev_entry'.

This revision now requires review to proceed.Nov 16 2019, 4:23 AM

Fix a transcription error.

Rewrite a couple of lines to that prev_entry isn't, briefly, after entry.

This revision is now accepted and ready to land.Nov 16 2019, 10:25 AM
sys/vm/vm_map.c
1430

Unsorted.

dougm marked 4 inline comments as done.
dougm edited the summary of this revision. (Show Details)

Update to account for r354785. Address reviewer comments.

This revision now requires review to proceed.Nov 17 2019, 7:06 AM

Use the FOREACH macros for iterating over the old map in vm_fork. Reformat a comment to fit in 80 columns.

markj added inline comments.
sys/vm/vm_map.c
2679

I suspect that some versions of GCC will complain that prev_entry may be left initialized while handling the modify_map case below, even though it is initialized correctly in that case.

2854

These assignments could be placed in a for-loop header.

sys/vm/vm_map.h
426

You have an extra newline in the function below. The newline is now optional per style(9), but we should be consistent within the same file.

This revision is now accepted and ready to land.Nov 18 2019, 6:53 PM
sys/vm/vm_map.c
3400–3404

I would find this clearer if it were written with entry and next_entry, not prev_entry.

sys/vm/vm_map.c
2970–2971

Again, I think that this would be clearer if it were written with entry and next_entry.

3287–3288

Initializing prev_entry and entry in the same order in both the then and else clause will make this easier to read.

dougm marked 6 inline comments as done.

Accept reviewer suggestions. Where the introduction of "next_entry" suggests that a for loop could replace a while loop, do so. Where that looks tricky because of a mid-loop continue (wire/unwire), make sure the continue case sets next_entry too. Observe that this all makes the 'first_iteration' state variable redundant, and eliminate it.

This revision now requires review to proceed.Nov 19 2019, 4:17 AM

Address small style violations from an in-person review.

sys/vm/vm_map.c
3127

unsorted

3243

indentation

dougm marked 2 inline comments as done.

Address reviewer feedback.

sys/vm/vm_map.c
2976–2977

unnecessary parens

sys/vm/vm_map.c
3755

need spaces around the bitwise or.

dougm marked 2 inline comments as done.

Address online and offline comments.

Undo some overzealous style-fixing.

In all the loops over map entries where the iterator wasn't called 'entry' and the last several versions of this patch renamed it to 'entry', revert the name back to what is was before, mostly limiting this patch to its original purpose and leaving a renaming patch for another day.

sys/vm/vm_map.c
2692

This assignment would fit at the end of the previous line.

While I like the suggestion that we use a consistent variable naming schema, I felt like those style changes were overwhelming the functional changes. So, I asked Doug to separate the functional and stylistic changes.

This revision is now accepted and ready to land.Nov 20 2019, 3:58 PM