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

Nick Piggin
2007-05-24T02:16:16Z

On Wed, May 23, 2007 at 07:04:28PM -0700, Linus Torvalds wrote:
>
>
> On Thu, 24 May 2007, Nick Piggin wrote:
> >
> > At most, if Linus really doesn't want ->fault to do the nopfn thing, then
> > I would be happy to leave in ->nopfn... but I don't see much reason not
> > to just merge them anyway... one fewer branch and less code in the
> > page fault handler.
>
> I just think that the "->fault" calling convention is _broken_.
>
> If you want to install the PFN in the low-level driver, just
>
> - pass the whole "struct vm_fault" to the PFN-installing thing (so that
> the driver at least doesn't have to muck with the address)

I was going to suggest that too, not a bad idea, but I think it is
not a big problem if we allow some special drivers to get access to
the address if they need to (otherwise they'll just go off and try
to invent their own thing).

> - return a nice return value saying that you aren't returning a page. And
> since you *also* need to return a value saying whether the page you
> want to install is locked or not, that just means that the "struct page
> *" approach (with a few extra error cases) won't cut it.

Yeah we can do that. We could have the VM_FAULT_ code in the first byte,
and return flags in the second, with the upper 2 bytes reserved... or
something like that.

> - don't add yet another interface. Replace it cleanly. If you don't want
> to re-use the name, fine, but at least replace it with something
> better.

I will definitely do that, but it is far easier to first merge it and
*then* clean up the drivers, rather than carry around all the patches
to do it. The nopage compat code is really small, so I don't see a
problem with carrying it around for a few -rcs until people have had time
to test and ack their driver conversions.

Actually I had a tear in my eye when removing the nopage name... it is
more quirky and has more character than the bland "fault"! :)

> The old "nopage()" return values weren't exactly pretty before either, but
> dang, do we really have to make it even MORE broken by having to have
> _both_ the old and the new ones, with different interfaces and magic
> locking rules depending on flags? I say "no".

Well thanks a lot for the feedback. I'll try to make some improvements
and hopefully we can try again when 2.6.23 opens.