On 09/12/24(Mon) 12:21, Mark Kettenis wrote:
> > Date: Mon, 9 Dec 2024 11:38:53 +0100
> > From: Martin Pieuchot <mpi@grenadille.net>
> 
> Not looked at the complete diff in detail yet, but I spotted something
> that raised a question about the approach taken:
> 
> You're mapping both VM_PAGER_BAD and VM_PAGER_ERROR to EACCESS here.
> But those really are different things and the difference should really
> be visible in userland.  When I wrote this code, my expectation was
> that return VM_PAGER_ERROR would result in uvm_fault() returning EIO,
> which would then be translated into a SIGBUS.  Now, looking at
> uvm_fault.c it seems we currently do return EACCESS in that case.

I'm not doing it.  It is already like that.  I agree with all you said
and I also want more EIO to be returned.

> Maybe that was always wrong or maybe this got lost in some kind of
> refactoring.

That was always wrong and that's why I added some XXX below where we
could change EACCESS into EIO.  More could be converted afterward, I'm
not interested in doing this work for the moment.

> Now you probably don't want to introduce a change of behaviour with
> this diff.  But maybe it is a good idea to mark those VM_PAGER_ERROR
> cases that now become EACCESS with /* XXX */ such that we can revisit
> them later.  That's what you did with the -ENOMEM case in
> i915_error_to_vmf_fault(), which we may indeed want to propagate up as
> ENOMEM instead of EACCESS as well.  But that needs a bit of thought.

That's exactly what I did.

> Also, returning errno values instead of the VM_PAGER_XXX constants
> means that it becomes less obvious what error values can actually be
> returned.  We probably should document the accepted error values
> somewhere.  Maybe we should add those to the uvm_fault(9) page?

Sure, I'll add that to my VM_PAGER_* removal todo list.

> Cheers,

Thanks for the review.

Martin