On Tue, Apr 12, 2005 at 06:04:47PM -0700, Libor Michalek wrote:
> On Mon, Apr 11, 2005 at 05:13:47PM -0700, Andrew Morton wrote:
> > Roland Dreier <roland@topspin.com> wrote:
> > >
> > >     Troy> Do we even need the mlock in userspace then?
> > > 
> > > Yes, because the kernel may go through and unmap pages from userspace
> > > while trying to swap.  Since we have the page locked in the kernel,
> > > the physical page won't go anywhere, but userspace might end up with a
> > > different page mapped at the same virtual address.
> 
> With the last few kernels I haven't had a chance to retest the problem
> that pushed us in the direction of using mlock. I will go back and do
> so with the latest kernel. Below I've given a quick description of the
> issue.
> 
> > That shouldn't happen.  If get_user_pages() has elevated the refcount on a
> > page then the following can happen:
> > 
> > - The VM may decide to add the page to swapcache (if it's not mmapped
> >   from a file).
> > 
> > - Once the page is backed by either swapcache of a (mmapped) file, the VM
> >   may decide the unmap the application's pte's.  A later minor fault by the
> >   app will cause the same physical page to be remapped.
> 
> The driver did use get_user_pages() to elevated the refcount on all the
> pages it was going to use for IO, as well as call set_page_dirty() since
> the pages were going to have data written to them from the device.
> 
> The problem we were seeing is that the minor fault by the app resulted
> in a new physical page getting mapped for the application. The page that
> had the elevated refcount was still waiting for the data to be written
> to by the driver at the time that the app accessed the page causing the
> minor fault. Obviously since the app had a new mapping the data written
> by the driver was lost.
> 
> It looks like code was added to try_to_unmap_one() to address this, so
> hopefully it's no longer an issue...

  I wrote a quick test module and program to confirm that the problem
we saw in older kernels with get_user_pages() no longer exists. The
module creates a character device with three different ioctl commands:

  - Pin the pages of a buffer using get_user_pages()
  - Check the pages by calling get_user_pages() a second time and
    comparing the new and original page list.
  - Relase the pages using put_page()

  The program opens the charcter device file descriptor, pins the pages
and waits for a signal, before checking the pages, which is sent to the
process after running some other program which exercises the VM. On older
kernels the check fails, on my 2.6.11 kernel the check succeeds. So
mlock is not needed on top of get_user_pages() as it was before.

  Thanks for the heads up.

  Module and program attached.

-Libor