Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6df12647-c90c-97d5-c1bb-5b066affae46@nvidia.com>
Date:   Wed, 16 Feb 2022 19:19:55 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH v4 3/4] mm: Change zap_details.zap_mapping into even_cows
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Matthew Wilcox <willy@infradead.org>,
        Yang Shi <shy828301@gmail.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Alistair Popple <apopple@nvidia.com>,
        David Hildenbrand <david@redhat.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Hugh Dickins <hughd@google.com>
References: <20220216094810.60572-1-peterx@redhat.com>
 <20220216094810.60572-4-peterx@redhat.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20220216094810.60572-4-peterx@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OVlReFp1emtRSHFjYUN6TFh6emRWWjNVMlF4MDkvMjh2Y0xvR0F5bE1RcDBx?=
 =?utf-8?B?OEtGL3ZZRTIxczYwVjJpSjNBWHNicC9EUUNzaC9URGtLSWdQQXo1ZGtWZWtz?=
 =?utf-8?B?dTBYSlNvQzB1RE1MZmRKMjJWMG5UWG9td2tCNnd2TXlwUHQ3VFp2UzBKbXVY?=
 =?utf-8?B?N0lFKzhFQ3hjajNCNTFGN1dWZW4zOGtJcFhvb2poNGNrWkNIaTl2aDA1cmM5?=
 =?utf-8?B?TG5ONlpoV1dUdzAza3I3bzlqRjN4bWo0ckVjZTlyVkpRRElUTXRQMWNQNzNs?=
 =?utf-8?B?NEtWOHNBK2hwRUpJWG9NZHFvVDNiUlArYm5DK0pyNVh4Vlk4VzJLZlQ4TmhI?=
 =?utf-8?B?Rzc4K3lzTEtRZFRRZGhZRGorb2dqTS9WVlNEQ1A5eDdvaWVZa05QYmZDSzdH?=
 =?utf-8?B?TlQ3N1FqaTgxblRzR1FvUUZmelBzRDRuYWdJdEJTaUlGMTNHNmtRcTRPZHhV?=
 =?utf-8?B?TnJGZURpZ0gvdWRXb25SczE4R0NjYzNtd0FjcUVVOE92KzR5eTZXczJZRWhY?=
 =?utf-8?B?blhrRXQrM0t0VFVZRXNCNHhkNkw5eS9mTjhzRTh3c3VObVJaRnp0M3RCZTdW?=
 =?utf-8?B?dmpJQkQ3bWtrdFE3cWtpVkxZMDdmMUlsR2ZRRmxtMnBIV1lQM3MycW1sNHZS?=
 =?utf-8?B?bldRbkEvblhQSTVWMXNpcDAwKzV1enlaS2pPYVBUajNjOStueUlEaGJIOElZ?=
 =?utf-8?B?WkU1L3NBMkMzYXlFS2dhbDdqTmc0UTJJQ1VVbGw0MzByUlBzb0dSRFQ3bG4x?=
 =?utf-8?B?bTVzSXhhVXRMRHhWeXFFMURPY1hyM1lVUm1KN25JdEFrUVZBWk04TzB0Y2JX?=
 =?utf-8?B?TjdBbkdMckdjZnJDZEs1WjRhMXNWOERUZXZlYVpza3pKVis1MmxZM0dXTUI2?=
 =?utf-8?B?T3cyNytEY3h6OGp0S0sxb1FTNmV1cW8xUTE4N3lxbDZuZVZPSlFNYURkN1ZV?=
 =?utf-8?B?V1NpaUFZWHlUK2VXN1N3TTBOam1iRWJ2eEUydjJOaDF6bEVhdER2Qlo4Lyt1?=
 =?utf-8?B?R3crNkZ6VzdqTk1sVENJL2pRay85bGZSUHJaakhUbU5nczhreUZPeE5hVksz?=
 =?utf-8?B?RmJqYXNZY0FzRFBsbEF6RVhUWXVTcHBwOGZ5Nm5wWk1qSG94UGozL2RKdG9D?=
 =?utf-8?B?eUJSSzlTbDlMY04rMExMaFZ4bHlBZmx1RjFTcTQ1S3VITnYzYmJsRC9UMFVo?=
 =?utf-8?B?Z3F0M0orWm95UndQTEp5WnR6NGRpTUVLbEV4U0JnN3VxRFh2dUw5RmI0N084?=
 =?utf-8?B?S2U2YnJ3d1VET3JqQ1FnNzFkb1ViYlU3Z1F1dG9KSHE5YzBJQ2Y4WHZnTDVM?=
 =?utf-8?B?b2JXY0thMzZHY0dsM2thdUJBa3o0NnhUNjBuTGg5WmdhWHltSWNTWCtlK3Qv?=
 =?utf-8?B?VGxYVUNZUlR6MW1RTUNDbFZHNVpwK2E2ak9yUGxGZVhncUFCZ1QyZk11bVlm?=
 =?utf-8?B?MnB6M29VOWZNNlVDRVNkLzJlOHJ3Vk5uNEEyeExrOFplVnRYdVoyNEsyc3hK?=
 =?utf-8?B?dk9QMFZvVUQ0ek5DWDB0VzdjYnZCZmcrcnZtc2g3d2FKNXU1cjd1aGFEcFVZ?=
 =?utf-8?B?b1huZHZ1blE2ZElFWGFZWjBmRHNtYjQxNEcxM09TRlIyZmVzeVBKQ1NLQnJy?=
 =?utf-8?B?TjFtSEVCWHdZbThCTURXQWg3M2hwUWM3VWRSd1VFUTRCVkFJaWNDSTJQZDhu?=
 =?utf-8?B?YWwxTmhWNm9sU0l3NmFYWXF6bS96RklCeXZsM3EzNkxqajA4OXBkLzZtazFy?=
 =?utf-8?B?UnFWRGVsVmNmL3daWVJSRnIxOVpRTU9QVTVNVzg4YlBHN2FxcjJBeW1hZXla?=
 =?utf-8?B?cjY2Z2hvM2hsTmNEbm1ITVg4TVUycUpBM1B2QnpyNEI0SXRSczVDTGJNQ3BK?=
 =?utf-8?B?TDNPbFJlQk1rSHJwZmNabHBad3F3ekxuT0dKSnVHMXFmU1lwZWlKeEVWdFJ1?=
 =?utf-8?B?d1NwYVk5UGtNYnBPOXdFbjFVMXJuUEJSSnJzaUtjNlpVVzFKcUE2TlprTlVs?=
 =?utf-8?B?TG5meHB6UVNRMWpGbGNRWFovVFZxZE0zd1lXYmdKOGMvZ2RXWmZHd05RMEtl?=
 =?utf-8?B?cTJTNW51bkVOUVZuNk4xNkNrWGV2Znkzay8yNEJIaU9EK0FhYVpiYU4zQXl6?=
 =?utf-8?B?cm1JZWpOZSsvNURaVU5rdkNMY3BmTG1FRFFEZUNWdXFEb1h0Qko5Q3owY3BC?=
 =?utf-8?Q?ewuWJmCBwC/z94h301Mjj5A=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8e48d5f4-803a-43d3-55ad-08d9f1c45fc9
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2022 03:19:57.5830
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dzseDkpyIYOlV3lm7oG4c342noOa8B/QhNYtUocmKjnb+22NoCTdGJMz60Bogk3EIUdGniEC84FLrabH1YLDsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR12MB3522
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/16/22 1:48 AM, Peter Xu wrote:
> Currently we have a zap_mapping pointer maintained in zap_details, when it is
> specified we only want to zap the pages that has the same mapping with what the
> caller has specified.
> 
> But what we want to do is actually simpler: we want to skip zapping
> private (COW-ed) pages in some cases.  We can refer to unmap_mapping_pages()
> callers where we could have passed in different even_cows values.  The other
> user is unmap_mapping_folio() where we always want to skip private pages.
> 
> According to Hugh, we used a mapping pointer for historical reason, as
> explained here:
> 
>   https://lore.kernel.org/lkml/391aa58d-ce84-9d4-d68d-d98a9c533255@google.com/
> 
> Quoting partly from Hugh:
> 
>   Which raises the question again of why I did not just use a boolean flag
>   there originally: aah, I think I've found why.  In those days there was a
>   horrible "optimization", for better performance on some benchmark I guess,
>   which when you read from /dev/zero into a private mapping, would map the zero
>   page there (look up read_zero_pagealigned() and zeromap_page_range() if you
>   dare).  So there was another category of page to be skipped along with the
>   anon COWs, and I didn't want multiple tests in the zap loop, so checking
>   check_mapping against page->mapping did both.  I think nowadays you could do
>   it by checking for PageAnon page (or genuine swap entry) instead.
> 
> This patch replaced the zap_details.zap_mapping pointer into the even_cows
> boolean, then we check it against PageAnon.
> 
> Suggested-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  mm/memory.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA

> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 14d8428ff4db..ffa8c7dfe9ad 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1309,8 +1309,8 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
>   * Parameter block passed down to zap_pte_range in exceptional cases.
>   */
>  struct zap_details {
> -	struct address_space *zap_mapping;	/* Check page->mapping if set */
>  	struct folio *single_folio;	/* Locked folio to be unmapped */
> +	bool even_cows;			/* Zap COWed private pages too? */
>  };
>  
>  /* Whether we should zap all COWed (private) pages too */
> @@ -1321,13 +1321,10 @@ static inline bool should_zap_cows(struct zap_details *details)
>  		return true;
>  
>  	/* Or, we zap COWed pages only if the caller wants to */
> -	return !details->zap_mapping;
> +	return details->even_cows;
>  }
>  
> -/*
> - * We set details->zap_mapping when we want to unmap shared but keep private
> - * pages. Return true if we should zap this page, false otherwise.
> - */
> +/* Decides whether we should zap this page with the page pointer specified */
>  static inline bool should_zap_page(struct zap_details *details, struct page *page)
>  {
>  	/* If we can make a decision without *page.. */
> @@ -1338,7 +1335,8 @@ static inline bool should_zap_page(struct zap_details *details, struct page *pag
>  	if (!page)
>  		return true;
>  
> -	return details->zap_mapping == page_rmapping(page);
> +	/* Otherwise we should only zap non-anon pages */
> +	return !PageAnon(page);
>  }
>  
>  static unsigned long zap_pte_range(struct mmu_gather *tlb,
> @@ -3403,7 +3401,7 @@ void unmap_mapping_folio(struct folio *folio)
>  	first_index = folio->index;
>  	last_index = folio->index + folio_nr_pages(folio) - 1;
>  
> -	details.zap_mapping = mapping;
> +	details.even_cows = false;
>  	details.single_folio = folio;
>  
>  	i_mmap_lock_write(mapping);
> @@ -3432,7 +3430,7 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
>  	pgoff_t	first_index = start;
>  	pgoff_t	last_index = start + nr - 1;
>  
> -	details.zap_mapping = even_cows ? NULL : mapping;
> +	details.even_cows = even_cows;
>  	if (last_index < first_index)
>  		last_index = ULONG_MAX;
>