vm_fault: Introduce a fault_status enum for internal return types

[markj]

Summary

Rather than overloading the meanings of the Mach statuses, introduce a
new set for use internally in the fault code. This makes the control
flow easier to follow and provides some extra error checking when a
fault status variable is used in a switch statement.

vm_fault_lookup() and vm_fault_relookup() continue to use Mach statuses
for now, as there isn't much benefit to converting them and they
effectively pass through a status from vm_map_lookup().

Obtained from: jeff (object_concurrency patches)

[kib]

sys/vm/vm_fault.c

390

I would convert this function more thoroughly. If you introduce enum fault_status res variable inited with FAULT_SUCCESS, then you can directly assign FAULT_FAILURE to it in places where rv is tested for != KERN_SUCCESS, and just return res.

IMO it would feel less hackish.

623

I would also introduce a res for this function.

1142

I would prefer to keep rv name for Mach errors, and use some other name (res?) for fault_status.

[markj]

Handle feedback.

[kib]

This looks fine.
I wonder if it makes sense to do some follow-up. I think we do not want to change vm_fault() to return these dedicated statuses, but on the other hand, vm_fault_trap() might benefit from view of the vm_fault() conditions before translations into Mach errors. This would allow to remove the 'unexpected' assert in vm_fault_trap(), and make its coupling with vm_fault() more natural, IMO.

[markj]

In D33017#745937, @kib wrote:

I wonder if it makes sense to do some follow-up. I think we do not want to change vm_fault() to return these dedicated statuses, but on the other hand, vm_fault_trap() might benefit from view of the vm_fault() conditions before translations into Mach errors. This would allow to remove the 'unexpected' assert in vm_fault_trap(), and make its coupling with vm_fault() more natural, IMO.

I like the idea. One complication is that vm_fault() returns untouched errors from vm_fault_lookup(), so a naive implementation would translate such errors to enum fault_status, and then translate back to Mach for external callers of vm_fault().

[kib]

In D33017#746016, @markj wrote:

In D33017#745937, @kib wrote:

I wonder if it makes sense to do some follow-up. I think we do not want to change vm_fault() to return these dedicated statuses, but on the other hand, vm_fault_trap() might benefit from view of the vm_fault() conditions before translations into Mach errors. This would allow to remove the 'unexpected' assert in vm_fault_trap(), and make its coupling with vm_fault() more natural, IMO.

I like the idea. One complication is that vm_fault() returns untouched errors from vm_fault_lookup(), so a naive implementation would translate such errors to enum fault_status, and then translate back to Mach for external callers of vm_fault().

Hm, did you considered, instead of dedicated enum, add a subspace in Mach error codes just for vm_fault() internals? You then might have e.g. a unified assert that error code belongs to subspace, and have a single covertor from fault subspace to normal Mach errors. This allows vm_map_lookup->vm_fault_lookup to directly return expected errors, avoiding double translations.

Or do the double translation, I do not think it is big deal really.