Page MenuHomeFreeBSD

Eliminate code duplication around clip_start
ClosedPublic

Authored by dougm on May 23 2019, 7:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 25 2024, 3:34 AM
Unknown Object (File)
Feb 23 2024, 5:55 AM
Unknown Object (File)
Feb 20 2024, 11:48 PM
Unknown Object (File)
Feb 16 2024, 4:17 PM
Unknown Object (File)
Feb 9 2024, 8:27 AM
Unknown Object (File)
Jan 31 2024, 1:46 AM
Unknown Object (File)
Jan 17 2024, 2:46 AM
Unknown Object (File)
Jan 9 2024, 11:20 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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.

Add assertions. Make macros into inline functions.

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.

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

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

Fix functions names in KASSERTs.

This revision now requires review to proceed.May 23 2019, 7:17 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().

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.

vm_map.c
2113 ↗(On Diff #57791)

What about vm_map_entry_clip_object() ?

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.

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.

vm_map.c
3854 ↗(On Diff #57791)

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

Accept most recent suggestions on function renaming.

vm_map.c
2168–2169 ↗(On Diff #57859)

This can't compile.

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.

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.

Restore a patch that got accidentally copied over last week.

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

dougm edited the summary of this revision. (Show Details)

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.

vm_map.c
2071 ↗(On Diff #57753)

You did not asserted that the map is locked there.

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