From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C7D26C49EAB for ; Mon, 21 Jun 2021 16:26:29 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5FBBD613D1 for ; Mon, 21 Jun 2021 16:26:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5FBBD613D1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id ECCFA6B0080; Mon, 21 Jun 2021 12:26:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EA2256B0081; Mon, 21 Jun 2021 12:26:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D43DC6B0082; Mon, 21 Jun 2021 12:26:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0247.hostedemail.com [216.40.44.247]) by kanga.kvack.org (Postfix) with ESMTP id A10646B0080 for ; Mon, 21 Jun 2021 12:26:28 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 3FCA91DAE5 for ; Mon, 21 Jun 2021 16:26:28 +0000 (UTC) X-FDA: 78278258856.04.D271836 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf18.hostedemail.com (Postfix) with ESMTP id BAA7220015F6 for ; Mon, 21 Jun 2021 16:26:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624292787; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qKkXwwbllZu80vSnXLRPT1a/f0c84KqiWlFVpCbhJxY=; b=UmmK836w5boPLuuQMUF7G7813hx61oqq1nH1E0khFE8d38W+tLIzAq7RROHmGWtTwR9lOR MwwR2JK8GfUdVj8i8jt/+cEJM3XXMxqsSLstjGCXlecBYhFI78fKFBojUJDfr1EiJ5/UQb 7nl9yTCxXRfZS6lPA1ADjq2grRn7uok= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-339-U-bky6anM5e_zvJqWBUiTg-1; Mon, 21 Jun 2021 12:26:25 -0400 X-MC-Unique: U-bky6anM5e_zvJqWBUiTg-1 Received: by mail-qk1-f198.google.com with SMTP id x4-20020a3763040000b02903ab95237c25so14617123qkb.0 for ; Mon, 21 Jun 2021 09:26:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=qKkXwwbllZu80vSnXLRPT1a/f0c84KqiWlFVpCbhJxY=; b=sFfEj2gVOjrXBDXjBzQxfXU7h+35mbzQGJeWu/cIT2j43edYhJKI+gkCkQPhwueyw2 BpJ6sqh09vn+84ja5eio/u9VxGoQbw860/wcGciZ6gW9UQdJ7wqQ1JeAEkS+SHoR+Oqb 24kgu7/osG15SrWQWm07BGL7ffh/wz4RyNExERzTypQCj+wBj9Nfsz07WF7c8ldM/bGw 1X3EguuscdadfEjSHurGxwF+A/CzRaL/9+eVLyqZLQ1PvKEliey+Y/uvCqmeehR0BeI+ IobvzvyYN1VRbAxooxBZYY/2zezNcVfAk1eoV4F4OAOfSiWFkldcetvYvZPRE30Jbz+X CVOA== X-Gm-Message-State: AOAM531kxXjRt9zXvChzgoW3TeuaEnZeZzHS/CSAgBF3Zv6Mog1zMr7W xuxBhFgMGu5GmbDnv9HDcu4zH4LSiSxdPo0Ny6PYcJwdmbEoMfcyBn1LxEss8d9ySMYov2WIdwM mPM59S2szG6o= X-Received: by 2002:a05:620a:1110:: with SMTP id o16mr7164910qkk.399.1624292785453; Mon, 21 Jun 2021 09:26:25 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz1QuLceinHpKjxVIKV/JvwK80nF6xWCGlZeAZS11VKaUOqogtDOaDzT6YcNUxlHXRdr83iIg== X-Received: by 2002:a05:620a:1110:: with SMTP id o16mr7164888qkk.399.1624292785239; Mon, 21 Jun 2021 09:26:25 -0700 (PDT) Received: from t490s (bras-base-toroon474qw-grc-65-184-144-111-238.dsl.bell.ca. [184.144.111.238]) by smtp.gmail.com with ESMTPSA id i19sm4271115qkk.45.2021.06.21.09.26.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 21 Jun 2021 09:26:24 -0700 (PDT) Date: Mon, 21 Jun 2021 12:26:23 -0400 From: Peter Xu To: Alistair Popple Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrea Arcangeli , "Kirill A . Shutemov" , Axel Rasmussen , Nadav Amit , Hugh Dickins , Jerome Glisse , Jason Gunthorpe , Andrew Morton , Miaohe Lin , Mike Rapoport , Matthew Wilcox , Mike Kravetz Subject: Re: [PATCH v3 09/27] mm: Introduce ZAP_FLAG_SKIP_SWAP Message-ID: References: <20210527201927.29586-1-peterx@redhat.com> <20210527202135.30890-1-peterx@redhat.com> <5565576.ugXqPVlkE4@nvdebian> MIME-Version: 1.0 In-Reply-To: <5565576.ugXqPVlkE4@nvdebian> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=UmmK836w; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf18.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 216.205.24.124) smtp.mailfrom=peterx@redhat.com X-Stat-Signature: wkgogo4rou53z6g78p69tu9xe45uuxyn X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: BAA7220015F6 X-HE-Tag: 1624292787-974999 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Jun 21, 2021 at 10:36:46PM +1000, Alistair Popple wrote: > On Friday, 28 May 2021 6:21:35 AM AEST Peter Xu wrote: > > Firstly, the comment in zap_pte_range() is misleading because it checks against > > details rather than check_mappings, so it's against what the code did. > > > > Meanwhile, it's confusing too on not explaining why passing in the details > > pointer would mean to skip all swap entries. New user of zap_details could > > very possibly miss this fact if they don't read deep until zap_pte_range() > > because there's no comment at zap_details talking about it at all, so swap > > entries could be errornously skipped without being noticed. > > > > This partly reverts 3e8715fdc03e ("mm: drop zap_details::check_swap_entries"), > > but introduce ZAP_FLAG_SKIP_SWAP flag, which means the opposite of previous > > "details" parameter: the caller should explicitly set this to skip swap > > entries, otherwise swap entries will always be considered (which is still the > > major case here). > > > > Cc: Kirill A. Shutemov > > Signed-off-by: Peter Xu > > --- > > include/linux/mm.h | 12 ++++++++++++ > > mm/memory.c | 8 +++++--- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 52d3ef2ed753..1adf313a01fe 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1723,6 +1723,8 @@ extern void user_shm_unlock(size_t, struct user_struct *); > > > > /* Whether to check page->mapping when zapping */ > > #define ZAP_FLAG_CHECK_MAPPING BIT(0) > > +/* Whether to skip zapping swap entries */ > > +#define ZAP_FLAG_SKIP_SWAP BIT(1) > > > > /* > > * Parameter block passed down to zap_pte_range in exceptional cases. > > @@ -1745,6 +1747,16 @@ zap_check_mapping_skip(struct zap_details *details, struct page *page) > > return details->zap_mapping != page_rmapping(page); > > } > > > > +/* Return true if skip swap entries, false otherwise */ > > +static inline bool > > +zap_skip_swap(struct zap_details *details) > > Minor nit-pick but imho it would be nice if the naming was consistent between > this and check mapping. Ie. zap_skip_swap()/zap_skip_check_mapping() or > zap_swap_skip()/zap_check_mapping_skip(). Makes sense; I'll use zap_skip_swap()/zap_skip_check_mapping() I think, then I keep this patch untouched. > > > +{ > > + if (!details) > > + return false; > > + > > + return details->zap_flags & ZAP_FLAG_SKIP_SWAP; > > +} > > + > > struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, > > pte_t pte); > > struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, > > diff --git a/mm/memory.c b/mm/memory.c > > index c9dc4e9e05b5..8a3751be87ba 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -1376,8 +1376,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > > continue; > > } > > > > - /* If details->check_mapping, we leave swap entries. */ > > - if (unlikely(details)) > > + if (unlikely(zap_skip_swap(details))) > > continue; > > > > if (!non_swap_entry(entry)) > > @@ -3328,7 +3327,10 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start, > > pgoff_t nr, bool even_cows) > > { > > pgoff_t first_index = start, last_index = start + nr - 1; > > - struct zap_details details = { .zap_mapping = mapping }; > > + struct zap_details details = { > > + .zap_mapping = mapping, > > I meant to comment on this in the previous patch, but it might be nice to set > .zap_mapping in the !even_cows case below to make it very obvious it only > applies to ZAP_FLAG_CHECK_MAPPING. I wanted to make it easy to understand by having zap_mapping always points to the mapping it's zapping, so it does not contain any other information like "whether we want to check the mapping is the same when zap", which now stays fully in the flags. Then it's always legal to reference zap_mapping without any prior knowledge. But indeed it's only used by ZAP_FLAG_CHECK_MAPPING. I do have a slight preference to keep it as the patch does, but I don't have a strong opinion. Let me know if you insist; I can change. > > Otherwise I think this is a good clean up which makes things clearer. I double > checked that unmap_mapping_pages() was the only place in the existing code that > needs ZAP_FLAG_SKIP_SWAP and that appears to be the case so there shouldn't be > any behaviour changes from this. > > Reviewed-by: Alistair Popple Since I won't change anything within this patch, I'll take this away, thanks! -- Peter Xu