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=-6.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 AB1D7C433F5 for ; Wed, 15 Sep 2021 02:52:57 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2D96B60F6F for ; Wed, 15 Sep 2021 02:52:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2D96B60F6F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id B36226B0071; Tue, 14 Sep 2021 22:52:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AE5CE6B0072; Tue, 14 Sep 2021 22:52:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 95F366B0073; Tue, 14 Sep 2021 22:52:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0210.hostedemail.com [216.40.44.210]) by kanga.kvack.org (Postfix) with ESMTP id 879DC6B0071 for ; Tue, 14 Sep 2021 22:52:56 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 1319D182F0AAA for ; Wed, 15 Sep 2021 02:52:56 +0000 (UTC) X-FDA: 78588285552.22.C2E9AC2 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf15.hostedemail.com (Postfix) with ESMTP id A0DBED000096 for ; Wed, 15 Sep 2021 02:52:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1631674375; 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=mNknc3PAGLPEbfdAPlC78Lmk68krcKaGTS1ga1ZhVzQ=; b=GC/0clbsHuwfH4Zyc1+XnIhC6SvQEi1IvFtM0mo/REHxf5vXy2jyibNai4t3jDA/gtCJ0C /bRrG1fzzmhtXoizd3750tEq8UmuF+RBPy9RsKjCM5Th0dLb1qcgm0VAMbqKEGp2/JImHO /4IYgOmIIX4Op3KR88S7jXO4UPzEPH0= Received: from mail-il1-f198.google.com (mail-il1-f198.google.com [209.85.166.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-268-caiELaCkMvWjhU-_lQp1Dw-1; Tue, 14 Sep 2021 22:52:52 -0400 X-MC-Unique: caiELaCkMvWjhU-_lQp1Dw-1 Received: by mail-il1-f198.google.com with SMTP id s15-20020a056e02216f00b002276040aa1dso942934ilv.3 for ; Tue, 14 Sep 2021 19:52:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=mNknc3PAGLPEbfdAPlC78Lmk68krcKaGTS1ga1ZhVzQ=; b=ttiCCzsiR+xeOBdsmqpivin70MkYAAUtxSwECiouN2OBgSTL3kk4UsR0Py5zMa6Fx0 PyDqWwguNdy1+jwrlXIuut2VGwlXuJbkO4R/ruvnsI8ViAVOdY115iJA104BcpfoqHSh FSSehhiHT0a3jD8tD1guZULUrlWarmG6mJ/iMJ84NHblJN8nGRgPEgPUqw/bTITEEu6b WEEDq+yi8TZk7wAI4/RoSTd2SVBo07gNZ55Hvq1iZyLyc7ova2u5apxi7TDeNUH8ANn7 21gjQfNuivybaPIeDvzONWvJnD/+S+ZCuz/SNK/4kYQIcCTCfjWH0h2b2/sJkT6JE3OH 4Cvg== X-Gm-Message-State: AOAM533uxBihLfqc1LKi+RCtbbInIo3eRLMhJLdd1sJ7OQ1KNDCEmxRf Dhj/qZnhvXNS/v9LPEpcVcvO0NLP9sMwb/o6PkByeonmtZPdX4CmxYJeFcQ3bM8q5WR6h+VCPwA V+evzhFvrov8= X-Received: by 2002:a05:6e02:144c:: with SMTP id p12mr9551339ilo.292.1631674371218; Tue, 14 Sep 2021 19:52:51 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzHNRphTXwxe/+Sfyf1gmfRf0CW0WLj7r6TJhh24pys6jEp3ccyd4fnS3xT0F3TuQSiiABz9A== X-Received: by 2002:a05:6e02:144c:: with SMTP id p12mr9551326ilo.292.1631674370967; Tue, 14 Sep 2021 19:52:50 -0700 (PDT) Received: from xz-m1.local ([2607:fea8:56a2:9100:f950:40ca:dc9b:42e7]) by smtp.gmail.com with ESMTPSA id t15sm7375529ioi.7.2021.09.14.19.52.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Sep 2021 19:52:50 -0700 (PDT) Date: Tue, 14 Sep 2021 22:52:48 -0400 From: Peter Xu To: Alistair Popple Cc: linux-kernel@vger.kernel.org, Andrew Morton , Hugh Dickins , linux-mm@kvack.org, Miaohe Lin , David Hildenbrand , Andrea Arcangeli , Yang Shi , Matthew Wilcox , "Kirill A . Shutemov" , Jerome Glisse , Liam Howlett , Mike Rapoport Subject: Re: [PATCH v3 5/5] mm: Add ZAP_FLAG_SKIP_SWAP and zap_flags Message-ID: References: <20210908163516.214441-1-peterx@redhat.com> <20210908163628.215052-1-peterx@redhat.com> <2576475.WBpAVSM2eX@nvdebian> MIME-Version: 1.0 In-Reply-To: <2576475.WBpAVSM2eX@nvdebian> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: A0DBED000096 Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b="GC/0clbs"; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf15.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=peterx@redhat.com X-Stat-Signature: txs56gddhd8mzysehkg8au5a35w7ne1d X-HE-Tag: 1631674375-146542 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: Hi, Alistair, On Wed, Sep 15, 2021 at 12:25:07PM +1000, Alistair Popple wrote: > On Thursday, 9 September 2021 2:36:28 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, there's no explicit reason why passing in the details pointer should > > 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 > > erroneously 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 should still > > be the major case here). > > > > We may want to look into when exactly we need ZAP_FLAG_SKIP_SWAP and we should > > have it in a synchronous manner, e.g., currently even if ZAP_FLAG_SKIP_SWAP is > > set we'll still look into swap pmds no matter what. But that should be a > > separate effort of this patch. > > I didn't really follow what you mean by "synchronous" here, although the > explanation about pmds makes sense so it's probably just terminology. Yes, maybe I should use "aligned manner", or please suggest anything that sounds better; sorry for my awkward English. > > > The flag introduced in this patch will be a preparation for more bits defined > > in the future, e.g., for a new bit in flag to show whether to persist the > > upcoming uffd-wp bit in pgtable entries. > > That's kind of the problem. The patch itself looks correct to me however as > mentioned it is mostly reverting a previous cleanup and it's hard to tell why > that's justified without the subsequent patches. Perhaps it makes the usage of > zap_details a bit clearer, but a comment also would with less code. > > I know you want to try and shrink the uffd-wp series but I think this patch > might be easier to review if it was included as part of that series. I posted it because I think it's suitable to have it even without uffd-wp. I tried to explain it above on two things this patch wanted to fix: Firstly the comment is wrong; we've moved back and forth on changing the zap_details flags but the comment is not changing along the way and it's not matching the code right now. Secondly I do think we should have a flag showing explicit willingness to skip swap entries. Yes, uffd-wp is the planned new one, but my point is anyone who will introduce a new user of zap_details pointer could overlook this fact. The new flag helps us to make sure someone will at least read the flags and know what'll happen with it. For the 2nd reasoning, I also explicitly CCed Kirill too, so Kirill can provide any comment if he disagrees. For now, I still think we should keep having such a flag otherwise it could be error-prone. Could you buy-in above reasoning? Basically above is what I wanted to express in my commit message. I hope that can justify that this patch (even if extremly simple) can still be considered as acceptable upstream even without uffd-wp series. If you still insist on this patch not suitable for standalone merging and especially if some other reviewer would think the same, I can move it back to uffd-wp series for sure. Then I'll repost this series with 4 patches only. In all cases, thanks for looking at the series. -- Peter Xu