Page MenuHomeFreeBSD

Use a loop macro for enumerating vm map entries
ClosedPublic

Authored by dougm on Thu, Oct 3, 8:00 AM.

Details

Summary

There are many source files that iterate over the entries of a vm_map. A change to the implementation of that iteration would require modifying all of them. Define a VME_FOREACH macro that encapsulates the current implementation, so that other files can be less sensitive to the changes in that implementation. Use the macro everywhere.

Test Plan

I can install the modified kernel and use it to compile a kernel.

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

dougm created this revision.Thu, Oct 3, 8:00 AM
dougm updated this revision to Diff 62874.Thu, Oct 3, 3:47 PM

Correct an off-by-one error.

pho added a comment.Thu, Oct 3, 4:01 PM

I ran tests on D21882.62858.diff for 5 1/2 hours without seeing any problems. I'll move on to 62874.

dougm updated this revision to Diff 62898.Fri, Oct 4, 4:09 AM

Combine two loops in ptrace_vm_entry into one, so that "entry->next" doesn't have to appear.

pho added a comment.Sat, Oct 5, 6:26 PM

I ran tests on D21882.62898 for more than a day. No problems seen.

dougm added a reviewer: markj.Sun, Oct 6, 6:54 PM
markj accepted this revision.Mon, Oct 7, 2:44 PM

I would consider calling it VM_MAP_ENTRY_FOREACH(): it is a bit long, but there doesn't seem to be much risk of hitting the 80 column limit, and VME_FOREACH() seems a bit terse given that the macro is used outside of the VM subsystem. The change looks fine to me either way, so please keep the name if you prefer.

sys/vm/vm_object.c
2387 ↗(On Diff #62898)

What's the purpose of the counter?

This revision is now accepted and ready to land.Mon, Oct 7, 2:44 PM
dougm added inline comments.Mon, Oct 7, 3:05 PM
sys/vm/vm_object.c
2387 ↗(On Diff #62898)

r6129 | dg | 1995-02-02 03:09:15 -0600 (Thu, 02 Feb 1995) | 48 lines

swap_pager.c:
Fixed long standing bug in freeing swap space during object collapses.
Fixed 'out of space' messages from printing out too often.
Modified to use new kmem_malloc() calling convention.
Implemented an additional stat in the swap pager struct to count the
amount of space allocated to that pager. This may be removed at some
point in the future.
Minimized unnecessary wakeups.

I'll remove the counter, if that's okay.

markj added inline comments.Mon, Oct 7, 3:08 PM
sys/vm/vm_object.c
2387 ↗(On Diff #62898)

Looks like it should be fine to remove.

alc added a comment.Mon, Oct 7, 5:55 PM

I would consider calling it VM_MAP_ENTRY_FOREACH(): it is a bit long, but there doesn't seem to be much risk of hitting the 80 column limit, and VME_FOREACH() seems a bit terse given that the macro is used outside of the VM subsystem. The change looks fine to me either way, so please keep the name if you prefer.

I agree.

alc added inline comments.Mon, Oct 7, 6:08 PM
sys/kern/sys_process.c
388 ↗(On Diff #62898)

I'm not sure that I see the point of checking the header repeatedly for an erroneous submap flag. But more generally, I can't make any sense out of the submap handling. Has this changed in the last year or so?

dougm added inline comments.Mon, Oct 7, 6:49 PM
sys/kern/sys_process.c
388 ↗(On Diff #62898)

The origin of the KASSERT is https://reviews.freebsd.org/D14005

dougm updated this revision to Diff 63020.Tue, Oct 8, 2:51 AM
dougm marked 3 inline comments as done.

VME_ -> VM_MAP_ENTRY_
Remove purposeless counter.
Move KASSERT out of loop.

This revision now requires review to proceed.Tue, Oct 8, 2:51 AM
dougm added inline comments.Tue, Oct 8, 6:54 AM
sys/kern/sys_process.c
388 ↗(On Diff #62898)

I'll change the "repeated" checking to make it one-time.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Oct 8, 7:14 AM
This revision was automatically updated to reflect the committed changes.