Page MenuHomeFreeBSD

vm_fault: Introduce a fault_status enum for internal return types
ClosedPublic

Authored by markj on Nov 16 2021, 9:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 18 2024, 1:36 PM
Unknown Object (File)
Dec 20 2023, 7:20 AM
Unknown Object (File)
Nov 23 2023, 5:51 AM
Unknown Object (File)
Nov 6 2023, 11:46 PM
Unknown Object (File)
Oct 30 2023, 7:34 PM
Unknown Object (File)
Oct 20 2023, 8:44 AM
Unknown Object (File)
Oct 5 2023, 10:45 PM
Unknown Object (File)
Sep 1 2023, 1:07 AM
Subscribers

Details

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)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 42833
Build 39721: arc lint + arc unit

Event Timeline

markj requested review of this revision.Nov 16 2021, 9:14 PM
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 marked 3 inline comments as done.

Handle feedback.

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.

This revision is now accepted and ready to land.Nov 17 2021, 10:00 AM
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().

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.