simplify entries after vm_map_protect -> KERN_RESOURCE_SHORTAGE

[dougm]

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.

[markj]

vm_map.c

2573 ↗

(On Diff #58989)

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

2575 ↗

(On Diff #58989)

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

[dougm]

Accept reviewer advice.

[markj]

vm_map.c

2576 ↗

(On Diff #59008)

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

[dougm]

vm_map.c

2576 ↗

(On Diff #59008)

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]

Don't over-simplify.

[markj]

vm_map.c

2576 ↗

(On Diff #59008)

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.

[dougm]

Back-up one version.

[kib]

vm_map.c

2516 ↗

(On Diff #59015)

some may BE mergeable.

[dougm]

Be more articulate.

[alc]

vm_map.c

2515 ↗

(On Diff #59020)

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

[dougm]

Discard the parts of a comment that may mislead.

[kib]

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]

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]

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]

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

[alc]

vm_map.c

2520–2524 ↗

(On Diff #59035)

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 ↗

(On Diff #59035)

Add

end = current->end;

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

2567 ↗

(On Diff #59035)

Ditto.

2580 ↗

(On Diff #59035)

I would move the parenthetical comment to the end.

2584 ↗

(On Diff #59035)

This line is indented by one space too many.

[dougm]

Accept reviewer suggestions.

[alc]

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