Page MenuHomeFreeBSD

Eliminate code duplication around clip_start
ClosedPublic

Authored by dougm on May 23 2019, 7:13 AM.

Details

Summary

There is code for creating objects to back map entries that is duplicated, and this change seeks to eliminate that duplication.

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.May 23 2019, 7:13 AM
dougm updated this revision to Diff 57775.May 23 2019, 4:01 PM

Add a 'modify' argument to vm_map_lookup_clip_start and use it everywhere vm_map_clip_start is used now.

kib added inline comments.May 23 2019, 4:04 PM
vm_map.c
2071 ↗(On Diff #57753)

Assert that entry->object.vm_object == NULL.
Assert locks.

2094 ↗(On Diff #57753)

Assert locks. Assert that the entry is not a submap.

2100 ↗(On Diff #57753)

Use the opportunity to fix indentation of these two lines.

2118 ↗(On Diff #57753)

May be convert this to static function (optionally inline) ?

2613 ↗(On Diff #57753)

I prefer to either have {} in both branches for if(), or to not have {} in both.

dougm updated this revision to Diff 57782.May 23 2019, 4:27 PM

Add assertions. Make macros into inline functions.

kib added inline comments.May 23 2019, 5:05 PM
vm_map.c
2137 ↗(On Diff #57782)

Do we need this function standalone ? If vm_map_clip_start() do at the very beginning

if(start <= entry->start)
     return;

and then put the body of the _vm_map_clip_start(), it seems to make the code simpler. I believe the old structure was for some quite non-optimizing compiler.

dougm updated this revision to Diff 57789.May 23 2019, 6:48 PM

Eliminate the tiny wrapper macros/functions around clip_start and clip_end.

kib accepted this revision.May 23 2019, 7:14 PM
kib added inline comments.
vm_map.c
2134 ↗(On Diff #57789)

It is no longer _vm_map_clip_start, remove prefix '_'.

2199 ↗(On Diff #57789)

Same there.

This revision is now accepted and ready to land.May 23 2019, 7:14 PM
dougm updated this revision to Diff 57791.May 23 2019, 7:17 PM

Fix functions names in KASSERTs.

This revision now requires review to proceed.May 23 2019, 7:17 PM
markj added inline comments.May 24 2019, 3:57 PM
vm_map.c
2090 ↗(On Diff #57791)

The comment doesn't really make sense anymore; what does "now" mean?

2113 ↗(On Diff #57791)

I don't really understand the purpose of lifting this portion out into a separate function: it doesn't eliminate any duplication. I also find it hard to reason about what this function is supposed to do given its name.

2133 ↗(On Diff #57791)

Can't the assertion come before the check?

3854 ↗(On Diff #57791)

IMO it would be more natural to return the new object from vm_map_entry_attach_backing_object().

dougm updated this revision to Diff 57844.May 24 2019, 4:13 PM

Address some reviewer comments: Move a KASSERT, shorten a comment.

dougm marked 10 inline comments as done.May 24 2019, 4:23 PM
dougm added inline comments.
vm_map.c
2090 ↗(On Diff #57791)

I have tried to make it more sensible.

2113 ↗(On Diff #57791)

The code appears in both clip_start and clip_end functions, so that duplication is removed.

I could change the name vm_map_entry_find_backing_object, but if I drop "vm_map_entry", Alan will object, and if I drop "backing_object", I'm dropping what the comment that precedes this code block says it's about. It either creates a backing object, or uses one already there. Maybe I can do better than "find"?

2133 ↗(On Diff #57791)

Okay.

3854 ↗(On Diff #57791)

In one place, I would cast away the returned result. In the other, I would get to drop one assignment line, but I would also need to shorten the function name to fit in 80 columns after doing proper line wrapping.

kib added inline comments.May 24 2019, 4:35 PM
vm_map.c
2113 ↗(On Diff #57791)

What about vm_map_entry_clip_object() ?

dougm marked 2 inline comments as done.May 24 2019, 4:54 PM
dougm added inline comments.
vm_map.c
2113 ↗(On Diff #57791)

I'm willing to do it, if that will satisfy everyone, but I don't think that name makes much sense. It seems like the actual clipping depends on a 'start' or 'end' value that isn't passed to this function.

markj added inline comments.May 24 2019, 8:22 PM
vm_map.c
2113 ↗(On Diff #57791)

vm_map_entry_charge_object()?

3854 ↗(On Diff #57791)

Maybe call it vm_map_entry_back()? That is admittedly terse.

Otherwise I would remove "_backing". I think it's implied.

markj added inline comments.May 24 2019, 8:24 PM
vm_map.c
3854 ↗(On Diff #57791)

... implied by virtue of the fact that the function doesn't take an object parameter.

dougm updated this revision to Diff 57859.May 24 2019, 8:36 PM

Accept most recent suggestions on function renaming.

alc added inline comments.May 24 2019, 8:46 PM
vm_map.c
2168–2169 ↗(On Diff #57859)

This can't compile.

dougm updated this revision to Diff 57860.May 24 2019, 8:54 PM

Fixed syntax error. Snuck a minute on the test box to verify that there were no other syntax errors arising from this patch to this file in one particular configuration.

dougm updated this revision to Diff 58348.Jun 7 2019, 3:55 AM

Drop an inessential assertion. It would be useful to keep it, to see if any callers led it to fail so that those callers could be taught not to send out-of-bounds addresses. But it's not essential to correctness here.

dougm updated this revision to Diff 58563.Jun 12 2019, 3:10 PM

Restore a patch that got accidentally copied over last week.

dougm added a comment.Jun 12 2019, 3:22 PM

Ignore this patch while I study an unexpected negative performance impact.

dougm edited the summary of this revision. (Show Details)Jun 12 2019, 6:23 PM
dougm updated this revision to Diff 58568.

Restrict the scope of this patch just to reducing code duplication. On this amd64, it reduces the size of vm_map.o from 456608 to 455816, with no obvious impact on performance.

kib added inline comments.Jun 12 2019, 7:07 PM
vm_map.c
2071 ↗(On Diff #57753)

You did not asserted that the map is locked there.

dougm added inline comments.Jun 12 2019, 8:39 PM
vm_map.c
2071 ↗(On Diff #57753)

There is no map here. I could assert something in the caller, or pass a map here so that I could assert that it is locked. In the case of the call from *_fork, I'm not sure which map I would assert as locked.

kib accepted this revision.Jun 12 2019, 8:57 PM
kib added inline comments.
vm_map.c
2071 ↗(On Diff #57753)

That would be the map which owns the entry. But ok, I think it is fine to not add an argument only used for assert.

This revision is now accepted and ready to land.Jun 12 2019, 8:57 PM
This revision was automatically updated to reflect the committed changes.