Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 23 Jan 2022 22:51:57 -0800 (PST)
From:   Hugh Dickins <hughd@google.com>
To:     Peter Xu <peterx@redhat.com>
cc:     Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, David Hildenbrand <david@redhat.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Yang Shi <shy828301@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Andrew Morton <akpm@linux-foundation.org>,
        Alistair Popple <apopple@nvidia.com>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details
 specified
In-Reply-To: <YepAgBz4EUXc/jrM@xz-m1.local>
Message-ID: <56e33192-965d-691a-afc-f77f5856afd@google.com>
References: <20211115134951.85286-1-peterx@redhat.com> <20211115134951.85286-2-peterx@redhat.com> <9937aaa-d9ab-2839-b0b7-691d85c9141@google.com> <Yd7VEXI+/Lu8MVOo@xz-m1.local> <Yd7W6ndSPkXQjurY@xz-m1.local> <391aa58d-ce84-9d4-d68d-d98a9c533255@google.com>
 <Yek6PaW8fAXY4Bft@xz-m1.local> <YeokbrpCdAGAYHEl@xz-m1.local> <YepAgBz4EUXc/jrM@xz-m1.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 21 Jan 2022, Peter Xu wrote:
> 
> Oh, one more thing..
> 
> When reading the history and also your explanations above, I figured one thing
> that may not be right for a long time, on zero page handling of zapping.
> 
> If to quote your comment above, we should keep the zero page entries too if
> zap_details.zap_mapping is specified.  However it's not true for a long time, I
> guess starting from when vm_normal_page() returns NULL for zero pfns.  I also
> have a very strong feeling that in the old world the "page*" is non-NULL for
> zero pages here.
> 
> So... I'm boldly thinking whether we should also need another fix upon the zero
> page handling here too, probably before this whole patchset (so it'll be the
> 1st patch, it should directly apply to master) because if I'm right on above it
> can be seen as another separate bug fix:
> 
> ---8<---
> diff --git a/mm/memory.c b/mm/memory.c
> index f306e698a1e3..9b8348746e0b 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1320,11 +1320,18 @@ struct zap_details {
>  static inline bool
>  zap_skip_check_mapping(struct zap_details *details, struct page *page)
>  {
> -       if (!details || !page)
> +       /* No detail pointer or no zap_mapping pointer, zap all */
> +       if (!details || !details->zap_mapping)
>                 return false;
>  
> -       return details->zap_mapping &&
> -               (details->zap_mapping != page_rmapping(page));
> +       /*
> +        * For example, the zero page.  If the user wants to keep the private
> +        * pages, zero page should also be in count.
> +        */
> +       if (!page)
> +               return true;
> +
> +       return details->zap_mapping != page_rmapping(page);
>  }
>  
>  static unsigned long zap_pte_range(struct mmu_gather *tlb,
> ---8<---
> 
> page can be NULL for e.g. PFNMAP and when error occured too above.  I assume we
> don't need to bother with them (e.g., I don't think PFNMAP or MIXED should
> specify even_cows=false at all, because there's no page cache layer), though.
> Mostly it's about how we should handle zero page right.

I have not understood the above.

I don't know of any problem that needs fixing with the zero page:
how do you suppose the zero page gets into a truncatable or hole-punchable
mapping?  We use it for read faults in anonymous mappings.  And I told the
story of how once-upon-a-time it could get inserted into any mapping by
reading from /dev/zero, but that odd case was dropped years ago.  And I
am open to (even encouraging) a change to make use of zero page for read
faults of holes in shmem: but that's potential future work, which would
require some changes elsewhere (though perhaps none here: the zero page
could never be used for the result of a COW).

Please explain the zero page problem you hope to fix here.

Hugh