Page MenuHomeFreeBSD
Differential D20753

simplify entries after vm_map_protect -> KERN_RESOURCE_SHORTAGE
ClosedPublic
Actions

Authored by dougm on Jun 25 2019, 8:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 1:54 AM
Unknown Object (File)
Thu, Dec 12, 11:05 PM
Unknown Object (File)
Nov 18 2024, 2:54 PM
Unknown Object (File)
Nov 17 2024, 11:45 PM
Unknown Object (File)
Nov 16 2024, 11:19 PM
Unknown Object (File)
Sep 22 2024, 7:36 PM
Unknown Object (File)
Sep 17 2024, 2:34 PM
Unknown Object (File)
Sep 7 2024, 6:27 PM
View All Files
Subscribers
imp

Details

Reviewers
kib
markj
alc
Commits
rS349498: If vm_map_protect fails with KERN_RESOURCE_SHORTAGE, be sure to
Summary

If swap_reserve fails in vm_map_protect, don't return KERN_RESOURCE_SHORTAGE immediately. Instead, let the next loop simplify entries, but nothing else.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

dougm created this revision.Jun 25 2019, 8:38 AM
markj added inline comments.Jun 25 2019, 3:25 PM
vm_map.c
2584

I'd suggest adding a comment, perhaps at the beginning of the prior loop, explaining the error handling.

2586

Logically I think it makes more sense to test rv before MAP_ENTRY_GUARD.

dougm updated this revision to Diff 59008.Jun 25 2019, 3:51 PM

Accept reviewer advice.

markj added inline comments.Jun 25 2019, 4:36 PM
vm_map.c
2584

Can't we avoid the calls to vm_map_simplify_entry() when rv == KERN_SUCCESS?

dougm added inline comments.Jun 25 2019, 4:42 PM
vm_map.c
2584

I think you mean that you don't want to start simplifying current when
(current->eflags & MAP_ENTRY_GUARD) != 0) and rv==KERN_SUCCESS.

If that's what you mean, then, yes, I can change that.

dougm updated this revision to Diff 59015.Jun 25 2019, 4:45 PM

Don't over-simplify.

markj accepted this revision.Jun 25 2019, 4:54 PM
markj added inline comments.
vm_map.c
2584

I'm sorry, I missed that we were previously calling vm_map_simplify_entry() at the end of the final loop, so I was confused.

Now I think it makes more sense to leave the code as-is: the loop above clips map entries before checking for MAP_ENTRY_GUARD, so we should simplify before checking for MAP_ENTRY_GUARD.

This revision is now accepted and ready to land.Jun 25 2019, 4:54 PM
dougm updated this revision to Diff 59018.Jun 25 2019, 5:02 PM

Back-up one version.

This revision now requires review to proceed.Jun 25 2019, 5:02 PM
kib accepted this revision.Jun 25 2019, 5:03 PM
kib added inline comments.
vm_map.c
2524

some may BE mergeable.

This revision is now accepted and ready to land.Jun 25 2019, 5:03 PM
dougm updated this revision to Diff 59020.Jun 25 2019, 5:09 PM

Be more articulate.

This revision now requires review to proceed.Jun 25 2019, 5:09 PM
markj accepted this revision.Jun 25 2019, 7:41 PM
This revision is now accepted and ready to land.Jun 25 2019, 7:41 PM
alc added inline comments.Jun 25 2019, 8:29 PM
vm_map.c
2523

This line reads as though pages are being copied here, which they are not.

dougm updated this revision to Diff 59030.Jun 25 2019, 8:34 PM

Discard the parts of a comment that may mislead.

This revision now requires review to proceed.Jun 25 2019, 8:34 PM
kib added a comment.Jun 25 2019, 9:03 PM

IMO removing the sentence at all is overreaction, you can say 'will be copied' or use any other syntax construct that indicates that copying could occur in future.

alc added a comment.Jun 25 2019, 10:46 PM
In D20753#449053, @kib wrote:

IMO removing the sentence at all is overreaction, you can say 'will be copied' or use any other syntax construct that indicates that copying could occur in future.

Agreed. You can say, "If too little swap space is available to back the copy-on-write pages that may now be written to, ...

alc added a comment.Jun 25 2019, 10:50 PM
In D20753#449073, @alc wrote:
In D20753#449053, @kib wrote:

IMO removing the sentence at all is overreaction, you can say 'will be copied' or use any other syntax construct that indicates that copying could occur in future.

Agreed. You can say, "If too little swap space is available to back the copy-on-write pages that may now be written to, ...

Or, "If swap space can't be reserved to back the copy-on-write pages that may now be written, ..."

dougm updated this revision to Diff 59035.Jun 25 2019, 10:55 PM

Fine-tune a comment that still includes "now will do cow".

kib accepted this revision.Jun 26 2019, 6:42 AM
This revision is now accepted and ready to land.Jun 26 2019, 6:42 AM
dougm added 1 blocking reviewer(s): alc.Jun 27 2019, 7:26 PM
This revision now requires review to proceed.Jun 27 2019, 7:26 PM
alc added inline comments.Jun 27 2019, 10:35 PM
vm_map.c
2520–2526

I've tried to eliminate the repeated content between the two sentences:

"Before changing the protections, try to reserve swap space for any private (i.e., copy-on-write) mappings that are transitioning from read-only to read/write access. If a reservation fails, break out of this loop early and let the next loop simplify the entries, since some may now be mergeable.

2543–2544

Add

end = current->end;

so that the next loop doesn't bother iterating over entries that this loop never touched.

2567–2568

Ditto.

2580

I would move the parenthetical comment to the end.

2584

This line is indented by one space too many.

dougm updated this revision to Diff 59131.Jun 27 2019, 10:52 PM

Accept reviewer suggestions.

alc accepted this revision.Jun 27 2019, 11:09 PM

In case of swap reservation failure, I suspect that only the first and last entries in the range will actually be mergeable.

This revision is now accepted and ready to land.Jun 27 2019, 11:09 PM
Closed by commit rS349498: If vm_map_protect fails with KERN_RESOURCE_SHORTAGE, be sure to (authored by dougm). · Explain WhyJun 28 2019, 2:15 AM
This revision was automatically updated to reflect the committed changes.
dougm added a commit: rS349498: If vm_map_protect fails with KERN_RESOURCE_SHORTAGE, be sure to.
Herald added a subscriber: imp. · View Herald TranscriptJun 28 2019, 2:15 AM

Revision Contents
Changeset List

PathSize
32 lines

Diff 59030

View Options

vm_map.c

Loading...