Re: [patch 3/8] mm: merge nopfn into fault

Nick Piggin
2007-05-26T07:34:26Z

On Fri, May 25, 2007 at 09:36:26AM -0700, Linus Torvalds wrote:
>
>
> On Fri, 25 May 2007, Nick Piggin wrote:
> >
> > What do you think? Any better?
>
> Yes, I think this is getting there. It made the error returns generally
> much simpler.
>
> That said, I think it has room for more improvement. Why not make the
> return value just be a bitmask, rather than having two separate "bytes" of
> data.

Yeah, that would be really nice, but I guess that now goes out and
touches all arch code too, doesn't it? Or... actually we could just
retain compatibility by masking off the high bits, and defining some
sane definition for VM_FAULT_MINOR (seems like 0x0000 would work).

Then in a subsequent patch I can update all architectures to use bit
masks (I think Ben wanted to make some changes here too, and we could
probably consolidate a bit of code).

Actually if we have a VM_FAULT_ERROR bitmask, we can probably be a
tiny bit more efficient in the arch fault handlers as well by doing an

if (unlikely(ret & VM_FAULT_ERROR))
out_of_line_error_handler();

> For example, you now do:
>
> > +
> > +/*
> > + * VM_FAULT_ERROR is set for the error cases, to make some tests simpler.
> > + */
> > +#define VM_FAULT_ERROR 0x20
> > +
> > +#define VM_FAULT_OOM (0x00 | VM_FAULT_ERROR)
> > +#define VM_FAULT_SIGBUS (0x01 | VM_FAULT_ERROR)
> > #define VM_FAULT_MINOR 0x02
> > #define VM_FAULT_MAJOR 0x03
>
> And it would be so much cleaner (I think) to just realize:
>
> - successful VM faults are always either major or minor, so having two
> different values for them is silly (it comes from the fact that we did
> _not_ have a bitmask). JUst make a "MAJOR" bit, and if it's clear, then
> it's not major, of course!
>
> - rather than have one bit to say "we had an error", just make each error
> be a bit of its own. We don't have that many (two, to be exact), so you
> actually don't even use any more bits, but it means that you can do:
>
> #define VM_FAULT_OOM 0x0001
> #define VM_FAULT_SIGBUS 0x0002
> #define VM_FAULT_MAJOR 0x0004
> #define VM_FAULT_WRITE 0x0008
>
> #define VM_FAULT_NONLINEAR 0x0010
> #define VM_FAULT_NOPAGE 0x0020 /* We did our own pfn map */
> #define VM_FAULT_LOCKED 0x0040 /* Returned a locked page */
>
> /* Helper defines: */
> #define VM_FAULT_ERROR (VM_FAULT_OOM | VM_FAULT_SIGBUS)
>
> and you're done. No magic semantics: you just always return a set of
> result flags.
>
> So now the _user_ would simply do something like
>
> unsigned int flags;
>
> flags = vma->vm_ops->fault(...);
> if (flags & VM_FAULT_ERROR)
> return flags;
>
> if (flags & VM_FAULT_MAJOR)
> increment_major_faults();
> else
> increment_minor_faults();
>
> /* Did the fault handler do it all already? All done! */
> if (flags & VM_FAULT_NOPAGE)
> return 0;
>
> page = fault->page;
> .. install page ..
>
> /*
> * If the fault handler returned a locked page, we should now
> * unlock it
> */
> if (flags & VM_FAULT_LOCKED)
> unlock_page(page);
>
> /* All done! */
> return 0;
>
> or something like that. Yeah, the above is simplified, but it's not
> *overly* so. And it never worries about "high bytes" and "low bytes", and
> it never worries about a certain set of bits meaning one thing, and
> another set meaning somethign else. Isn't that just much simpler for
> everybody?

Yes I think that seems nice. I'll do another inc patch.