Message-ID: <849f1e44-d35e-b8c6-c7c3-a73941028ba7@redhat.com>
Date:   Mon, 10 Jan 2022 09:37:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Cc:     Andrea Arcangeli <aarcange@redhat.com>,
        Yang Shi <shy828301@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Alistair Popple <apopple@nvidia.com>,
        "Kirill A . Shutemov" <kirill@shutemov.name>
References: <20211115134951.85286-1-peterx@redhat.com>
 <20211115134951.85286-2-peterx@redhat.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details
 specified
In-Reply-To: <20211115134951.85286-2-peterx@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 15.11.21 14:49, Peter Xu wrote:
> This check existed since the 1st git commit of Linux repository, but at that
> time there's no page migration yet so I think it's okay.
> 
> With page migration enabled, it should logically be possible that we zap some
> shmem pages during migration.  When that happens, IIUC the old code could have
> the RSS counter accounted wrong on MM_SHMEMPAGES because we will zap the ptes
> without decreasing the counters for the migrating entries.  I have no unit test
> to prove it as I don't know an easy way to trigger this condition, though.
> 
> Besides, the optimization itself is already confusing IMHO to me in a few points:
> 
>   - The wording "skip swap entries" is confusing, because we're not skipping all
>     swap entries - we handle device private/exclusive pages before that.

I think one part of the confusion is "swap vs non-swap" entries.
For !pte_none() && !pte_present() we can have

* swap entry
* non-swap entry
** device exclusive entry
** device private entry
** HWpoison entry
** migration entry

So the comment claims to skip "swap entries" but also skips HWpoison and
migration entries, and I think that's the confusing part.
Both only apply to PageAnon().


IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages().

I do wonder if any of the callers really cares about PageAnon() pages where this would be relevant.

Am I wrong or is unmap_mapping_pages() never called with "even_cows == true" and we can remove
that paremeter:


git grep -C2 unmap_mapping_pages
fs/afs/callback.c-      struct afs_vnode *vnode = container_of(work, struct afs_vnode, cb_work);
fs/afs/callback.c-
fs/afs/callback.c:      unmap_mapping_pages(vnode->vfs_inode.i_mapping, 0, 0, false);
fs/afs/callback.c-}
fs/afs/callback.c-
--
fs/dax.c-               if (dax_is_zero_entry(entry)) {
fs/dax.c-                       xas_unlock_irq(xas);
fs/dax.c:                       unmap_mapping_pages(mapping,
fs/dax.c-                                       xas->xa_index & ~PG_PMD_COLOUR,
fs/dax.c-                                       PG_PMD_NR, false);
--
fs/dax.c-        * get_user_pages() slow path.  The slow path is protected by
fs/dax.c-        * pte_lock() and pmd_lock(). New references are not taken without
fs/dax.c:        * holding those locks, and unmap_mapping_pages() will not zero the
fs/dax.c-        * pte or pmd without holding the respective lock, so we are
fs/dax.c-        * guaranteed to either see new references or prevent new
fs/dax.c-        * references from being established.
fs/dax.c-        */
fs/dax.c:       unmap_mapping_pages(mapping, start_idx, end_idx - start_idx + 1, 0);
fs/dax.c-
fs/dax.c-       xas_lock_irq(&xas);
--
fs/dax.c-               /* we are replacing a zero page with block mapping */
fs/dax.c-               if (dax_is_pmd_entry(entry))
fs/dax.c:                       unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,
fs/dax.c-                                       PG_PMD_NR, false);
fs/dax.c-               else /* pte entry */
fs/dax.c:                       unmap_mapping_pages(mapping, index, 1, false);
fs/dax.c-       }
fs/dax.c-
--
include/linux/mm.h-                         bool *unlocked);
include/linux/mm.h-void unmap_mapping_page(struct page *page);
include/linux/mm.h:void unmap_mapping_pages(struct address_space *mapping,
include/linux/mm.h-             pgoff_t start, pgoff_t nr, bool even_cows);
include/linux/mm.h-void unmap_mapping_range(struct address_space *mapping,
--
include/linux/mm.h-}
include/linux/mm.h-static inline void unmap_mapping_page(struct page *page) { }
include/linux/mm.h:static inline void unmap_mapping_pages(struct address_space *mapping,
include/linux/mm.h-             pgoff_t start, pgoff_t nr, bool even_cows) { }
include/linux/mm.h-static inline void unmap_mapping_range(struct address_space *mapping,
--
mm/khugepaged.c-
mm/khugepaged.c-                if (page_mapped(page))
mm/khugepaged.c:                        unmap_mapping_pages(mapping, index, 1, false);
mm/khugepaged.c-
mm/khugepaged.c-                xas_lock_irq(&xas);
--
mm/memory.c- * Unmap this page from any userspace process which still has it mmaped.
mm/memory.c- * Typically, for efficiency, the range of nearby pages has already been
mm/memory.c: * unmapped by unmap_mapping_pages() or unmap_mapping_range().  But once
mm/memory.c- * truncation or invalidation holds the lock on a page, it may find that
mm/memory.c- * the page has been remapped again: and then uses unmap_mapping_page()
--
mm/memory.c-
mm/memory.c-/**
mm/memory.c: * unmap_mapping_pages() - Unmap pages from processes.
mm/memory.c- * @mapping: The address space containing pages to be unmapped.
mm/memory.c- * @start: Index of first page to be unmapped.
--
mm/memory.c- * cache.
mm/memory.c- */
mm/memory.c:void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
mm/memory.c-            pgoff_t nr, bool even_cows)
mm/memory.c-{
--
mm/memory.c-    i_mmap_unlock_write(mapping);
mm/memory.c-}
mm/memory.c:EXPORT_SYMBOL_GPL(unmap_mapping_pages);
mm/memory.c-
mm/memory.c-/**
--
mm/memory.c-    }
mm/memory.c-
mm/memory.c:    unmap_mapping_pages(mapping, hba, hlen, even_cows);
mm/memory.c-}
mm/memory.c-EXPORT_SYMBOL(unmap_mapping_range);
--
mm/truncate.c-                           * zap the rest of the file in one hit.
mm/truncate.c-                           */
mm/truncate.c:                          unmap_mapping_pages(mapping, index,
mm/truncate.c-                                          (1 + end - index), false);
mm/truncate.c-                          did_range_unmap = 1;
--
mm/truncate.c-   */
mm/truncate.c-  if (dax_mapping(mapping)) {
mm/truncate.c:          unmap_mapping_pages(mapping, start, end - start + 1, false);
mm/truncate.c-  }
mm/truncate.c-out:


-- 
Thanks,

David / dhildenb