Re: [PATCH] mm: Skip opportunistic reclaim for dma pinned pages

[Jason Gunthorpe]

On Wed, Jun 24, 2020 at 01:47:23PM -0700, John Hubbard wrote:
> On 2020-06-24 12:21, Jason Gunthorpe wrote:
> > On Wed, Jun 24, 2020 at 08:14:17PM +0100, Chris Wilson wrote:
> > > A general rule of thumb is that shrinkers should be fast and effective.
> > > They are called from direct reclaim at the most incovenient of times when
> > > the caller is waiting for a page. If we attempt to reclaim a page being
> > > pinned for active dma [pin_user_pages()], we will incur far greater
> > > latency than a normal anonymous page mapped multiple times. Worse the
> > > page may be in use indefinitely by the HW and unable to be reclaimed
> > > in a timely manner.
> > 
> > A pinned page can't be migrated, discarded or swapped by definition -
> > it would cause data corruption.
> > 
> > So, how do things even get here and/or work today at all? I think the
> > explanation is missing something important.
> > 
> 
> Well, those activities generally try to unmap the page, and
> have to be prepared to deal with failure to unmap. From my reading,
> it seemed very clear.

I think Yang explained it - the page is removed from the mappings but
freeing it does not happen because page_ref_freeze() does not succeed
due to the pin.

Presumably the mappings can reconnect to the same physical page if
it is re-faulted to avoid any data corruption.

So, the issue here is the mappings are trashed while the page remains
- and trashing the mapping triggers a mmu notifier which upsets i915.

> What's less clear is why the comment and the commit description
> only talk about reclaim, when there are additional things that call
> try_to_unmap(), including:
> 
>     migrate_vma_unmap()
>     split_huge_page_to_list() --> unmap_page()

It looks like the same unmap first then abort if the refcount is still
elevated design as shrink_page_list() ?

> I do like this code change, though. And I *think* it's actually safe to
> do this, as it stays away from writeback or other filesystem activity.
> But let me double check that, in case I'm forgetting something.

It would be nice to have an explanation why it is OK now to change
it..

I don't know, but could it be that try_to_unmap() has to be done
before checking the refcount as each mapping is included in the
refcount? ie we couldn't know a DMA pin was active in advance?

Now that we have your pin stuff we can detect a DMA pin without doing
all the unmaps?

Jason