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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=unavailable 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 28135C4320A for ; Wed, 1 Sep 2021 21:49:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 09BC36109E for ; Wed, 1 Sep 2021 21:49:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231948AbhIAVub (ORCPT ); Wed, 1 Sep 2021 17:50:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47978 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231808AbhIAVu3 (ORCPT ); Wed, 1 Sep 2021 17:50:29 -0400 Received: from mail-il1-x132.google.com (mail-il1-x132.google.com [IPv6:2607:f8b0:4864:20::132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9549FC061575 for ; Wed, 1 Sep 2021 14:49:32 -0700 (PDT) Received: by mail-il1-x132.google.com with SMTP id s16so771521ilo.9 for ; Wed, 01 Sep 2021 14:49:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0vcgS+hcrgfO3SRo6YAvjtrGWuBUKWLMcX0rbxzFDas=; b=T9JBlfYbYH+xAwEpJSgGrx1v6dAJCKRR0isXSU0LUpdPiBs4qyNZVyP9L5B6rpiwqC w2hv+agTuTNFY5yL5iejk3kumsOTzQ/yuOZ5wga0DK9Os+HZAUsPwDmaN3kenfnRr1yO eaNF8N1rlz8wqFgk2Q48EXeyjXv+ycUN7AgBQ2nXza2g+YnFFpnmPZEfoHR2ev1OVbYj UxqP+yujrbJfI4xXS9vjazUgYmWO4E2LS+I+Rvqciz6MGkfOugR1WVD6ZMSODx2ENmXq mCXl+B8MIyjdJyDMuojaQqxO5mmAY94VaMixD99IZwgbAeUFyBEv1neAJRqdxzT5CrOR 5K1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0vcgS+hcrgfO3SRo6YAvjtrGWuBUKWLMcX0rbxzFDas=; b=XVTrah6hv7pBirOvX+Wb8jr+CCcAjueWfsaSuI96tfoBXZ0SsG94ofgTRGl4ereiio mn32FubAbZvW8TP3uUMS4qLB9hroY11MygGiifLUKUaz97yCBjSSVuJCs7UoDCX9pEoV D3fodOk4LHxdNXbRRFLAxAFKEdgbQ5J5jXV/y+0V9ZEJQGx3s5kJlMoib21V7yYo5w+V RjvbjF1QNee31/NQl4mdOHSEj75KcpbIYe206dIsRVoGQq03m82pget6+uC/Ci0EFhEl GIMOb5mPPqh2zy1GLg5d2OtI7qEDsHd3EAPapojDa+/RKK+0xQ1LfI27scpYNBjEr1kU WY4g== X-Gm-Message-State: AOAM531yDMscrTXstLSOi5/t5f3RhaF5em28Id7/B6g87px01Bu5G4vv 3IttJaq8MYtPj8YEFGYCvdFn7L6EcCfRVhJYVFZRAQ== X-Google-Smtp-Source: ABdhPJwbczxunW48/7yrWaaT2VzPWO7NdmVc8dCnDzsQhqJbBoa9m05tpPfxGLBgpfN3kSgKHSD+lNKWYP53UBhSs+s= X-Received: by 2002:a05:6e02:5aa:: with SMTP id k10mr1100592ils.301.1630532971871; Wed, 01 Sep 2021 14:49:31 -0700 (PDT) MIME-Version: 1.0 References: <20210901205622.6935-1-peterx@redhat.com> <20210901205622.6935-2-peterx@redhat.com> In-Reply-To: <20210901205622.6935-2-peterx@redhat.com> From: Axel Rasmussen Date: Wed, 1 Sep 2021 14:48:53 -0700 Message-ID: Subject: Re: [PATCH 1/5] mm/shmem: Unconditionally set pte dirty in mfill_atomic_install_pte To: Peter Xu Cc: LKML , Linux MM , Andrea Arcangeli , Mike Rapoport , Jerome Glisse , Alistair Popple , Yang Shi , Andrew Morton , David Hildenbrand , Miaohe Lin , "Kirill A . Shutemov" , Matthew Wilcox , Hugh Dickins Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 1, 2021 at 1:56 PM Peter Xu wrote: > > It was conditionally done previously, as there's one shmem special case that we > use SetPageDirty() instead. However that's not necessary and it should be > easier and cleaner to do it unconditionally in mfill_atomic_install_pte(). > > The most recent discussion about this is here, where Hugh explained the history > of SetPageDirty() and why it's possible that it's not required at all: > > https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ Thanks for the cleanup Peter! I think the discussion of whether or not the data can be marked dirty below is correct, and the code change looks good as well. But, I think we're missing an explanation why Hugh's concern is indeed not a problem? Specifically, this question: "Haha: I think Andrea is referring to exactly the dirty_accountable code in change_pte_protection() which worried me above. Now, I think that will turn out okay (shmem does not have a page_mkwrite(), and does not participate in dirty accounting), but you will have to do some work to assure us all of that, before sending in a cleanup patch." Do we have more evidence that this is indeed fine, vs. what we had when discussing this before? If so, we should talk about it explicitly in this commit message, I think. (Sorry if you've covered this and it's just going over my head. ;) ) > > > > Currently mfill_atomic_install_pte() has three callers: > > 1. shmem_mfill_atomic_pte > 2. mcopy_atomic_pte > 3. mcontinue_atomic_pte > > After the change: case (1) should have its SetPageDirty replaced by the dirty > bit on pte (so we unify them together, finally), case (2) should have no > functional change at all as it has page_in_cache==false, case (3) may add a > dirty bit to the pte. However since case (3) is UFFDIO_CONTINUE for shmem, > it's merely 100% sure the page is dirty after all, so should not make a real > difference either. > > This should make it much easier to follow on which case will set dirty for > uffd, as we'll simply set it all now for all uffd related ioctls. Meanwhile, > no special handling of SetPageDirty() if there's no need. > > Cc: Hugh Dickins > Cc: Axel Rasmussen > Cc: Andrea Arcangeli > Signed-off-by: Peter Xu > --- > mm/shmem.c | 1 - > mm/userfaultfd.c | 3 +-- > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index dacda7463d54..3f91c8ce4d02 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2437,7 +2437,6 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, > shmem_recalc_inode(inode); > spin_unlock_irq(&info->lock); > > - SetPageDirty(page); > unlock_page(page); > return 0; > out_delete_from_cache: > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index 0e2132834bc7..b30a3724c701 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -69,10 +69,9 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, > pgoff_t offset, max_off; > > _dst_pte = mk_pte(page, dst_vma->vm_page_prot); > + _dst_pte = pte_mkdirty(_dst_pte); > if (page_in_cache && !vm_shared) > writable = false; > - if (writable || !page_in_cache) > - _dst_pte = pte_mkdirty(_dst_pte); > if (writable) { > if (wp_copy) > _dst_pte = pte_mkuffd_wp(_dst_pte); > -- > 2.31.1 > 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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL 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 5E40BC432BE for ; Wed, 1 Sep 2021 21:49:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F1F0461026 for ; Wed, 1 Sep 2021 21:49:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org F1F0461026 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 7628D8D0002; Wed, 1 Sep 2021 17:49:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6E83B8D0001; Wed, 1 Sep 2021 17:49:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5B0868D0002; Wed, 1 Sep 2021 17:49:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0213.hostedemail.com [216.40.44.213]) by kanga.kvack.org (Postfix) with ESMTP id 45AE58D0001 for ; Wed, 1 Sep 2021 17:49:33 -0400 (EDT) Received: from smtpin03.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id E6FD218021CA1 for ; Wed, 1 Sep 2021 21:49:32 +0000 (UTC) X-FDA: 78540346584.03.23937C3 Received: from mail-il1-f179.google.com (mail-il1-f179.google.com [209.85.166.179]) by imf26.hostedemail.com (Postfix) with ESMTP id 9E99A20019C3 for ; Wed, 1 Sep 2021 21:49:32 +0000 (UTC) Received: by mail-il1-f179.google.com with SMTP id v2so756411ilg.12 for ; Wed, 01 Sep 2021 14:49:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0vcgS+hcrgfO3SRo6YAvjtrGWuBUKWLMcX0rbxzFDas=; b=T9JBlfYbYH+xAwEpJSgGrx1v6dAJCKRR0isXSU0LUpdPiBs4qyNZVyP9L5B6rpiwqC w2hv+agTuTNFY5yL5iejk3kumsOTzQ/yuOZ5wga0DK9Os+HZAUsPwDmaN3kenfnRr1yO eaNF8N1rlz8wqFgk2Q48EXeyjXv+ycUN7AgBQ2nXza2g+YnFFpnmPZEfoHR2ev1OVbYj UxqP+yujrbJfI4xXS9vjazUgYmWO4E2LS+I+Rvqciz6MGkfOugR1WVD6ZMSODx2ENmXq mCXl+B8MIyjdJyDMuojaQqxO5mmAY94VaMixD99IZwgbAeUFyBEv1neAJRqdxzT5CrOR 5K1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0vcgS+hcrgfO3SRo6YAvjtrGWuBUKWLMcX0rbxzFDas=; b=foiLyk4AS3lLpP8Us4iM/hXltY7Ec6TjXgONoF8feKnKO6pHn3ZLJ5mFYVxn7okMoM SHFD5iemM4+twzxluXqlS4gVeFC6EJqfAw2ssPLP5luQYHrty8xo19TQOSdHUY5DZRwe g3z9Vp6iCQhDCcu4+lXxCBW5OJU4td8NeRfaoknRPXCNoyA/KfpdRD3+DvNuF/GJuNTm 8I1QTxhPepadHtCMuBpnlfYBuNeE0YRSRnt6zb5wrUwSLXIsjBMbCcAnq3ARxM5jnPRo yXhaDEy1SV3DcNjUINwrPVC0da7oahYfpmRmvb8LaYTTCyjmm+mSUCLBAw3YkdwC/NZi WobQ== X-Gm-Message-State: AOAM530IakKZSxhJY47h9Danj/OjdJ2+guB+QntRtuW1bxmiDgAbvL/S kG1CMoqq8uiJA16HHVN+sasPR8qYFcbC2JBCZtycbQ== X-Google-Smtp-Source: ABdhPJwbczxunW48/7yrWaaT2VzPWO7NdmVc8dCnDzsQhqJbBoa9m05tpPfxGLBgpfN3kSgKHSD+lNKWYP53UBhSs+s= X-Received: by 2002:a05:6e02:5aa:: with SMTP id k10mr1100592ils.301.1630532971871; Wed, 01 Sep 2021 14:49:31 -0700 (PDT) MIME-Version: 1.0 References: <20210901205622.6935-1-peterx@redhat.com> <20210901205622.6935-2-peterx@redhat.com> In-Reply-To: <20210901205622.6935-2-peterx@redhat.com> From: Axel Rasmussen Date: Wed, 1 Sep 2021 14:48:53 -0700 Message-ID: Subject: Re: [PATCH 1/5] mm/shmem: Unconditionally set pte dirty in mfill_atomic_install_pte To: Peter Xu Cc: LKML , Linux MM , Andrea Arcangeli , Mike Rapoport , Jerome Glisse , Alistair Popple , Yang Shi , Andrew Morton , David Hildenbrand , Miaohe Lin , "Kirill A . Shutemov" , Matthew Wilcox , Hugh Dickins Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=T9JBlfYb; spf=pass (imf26.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.166.179 as permitted sender) smtp.mailfrom=axelrasmussen@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 9E99A20019C3 X-Stat-Signature: jm85pdkih4etfy8cay3podokmf5idr6j X-HE-Tag: 1630532972-531765 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 Wed, Sep 1, 2021 at 1:56 PM Peter Xu wrote: > > It was conditionally done previously, as there's one shmem special case that we > use SetPageDirty() instead. However that's not necessary and it should be > easier and cleaner to do it unconditionally in mfill_atomic_install_pte(). > > The most recent discussion about this is here, where Hugh explained the history > of SetPageDirty() and why it's possible that it's not required at all: > > https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ Thanks for the cleanup Peter! I think the discussion of whether or not the data can be marked dirty below is correct, and the code change looks good as well. But, I think we're missing an explanation why Hugh's concern is indeed not a problem? Specifically, this question: "Haha: I think Andrea is referring to exactly the dirty_accountable code in change_pte_protection() which worried me above. Now, I think that will turn out okay (shmem does not have a page_mkwrite(), and does not participate in dirty accounting), but you will have to do some work to assure us all of that, before sending in a cleanup patch." Do we have more evidence that this is indeed fine, vs. what we had when discussing this before? If so, we should talk about it explicitly in this commit message, I think. (Sorry if you've covered this and it's just going over my head. ;) ) > > > > Currently mfill_atomic_install_pte() has three callers: > > 1. shmem_mfill_atomic_pte > 2. mcopy_atomic_pte > 3. mcontinue_atomic_pte > > After the change: case (1) should have its SetPageDirty replaced by the dirty > bit on pte (so we unify them together, finally), case (2) should have no > functional change at all as it has page_in_cache==false, case (3) may add a > dirty bit to the pte. However since case (3) is UFFDIO_CONTINUE for shmem, > it's merely 100% sure the page is dirty after all, so should not make a real > difference either. > > This should make it much easier to follow on which case will set dirty for > uffd, as we'll simply set it all now for all uffd related ioctls. Meanwhile, > no special handling of SetPageDirty() if there's no need. > > Cc: Hugh Dickins > Cc: Axel Rasmussen > Cc: Andrea Arcangeli > Signed-off-by: Peter Xu > --- > mm/shmem.c | 1 - > mm/userfaultfd.c | 3 +-- > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index dacda7463d54..3f91c8ce4d02 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2437,7 +2437,6 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, > shmem_recalc_inode(inode); > spin_unlock_irq(&info->lock); > > - SetPageDirty(page); > unlock_page(page); > return 0; > out_delete_from_cache: > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index 0e2132834bc7..b30a3724c701 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -69,10 +69,9 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, > pgoff_t offset, max_off; > > _dst_pte = mk_pte(page, dst_vma->vm_page_prot); > + _dst_pte = pte_mkdirty(_dst_pte); > if (page_in_cache && !vm_shared) > writable = false; > - if (writable || !page_in_cache) > - _dst_pte = pte_mkdirty(_dst_pte); > if (writable) { > if (wp_copy) > _dst_pte = pte_mkuffd_wp(_dst_pte); > -- > 2.31.1 >