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=-11.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS 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 3A1D8C432BE for ; Wed, 1 Sep 2021 23:01:16 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B91A660F12 for ; Wed, 1 Sep 2021 23:01:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B91A660F12 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 27CA48D0002; Wed, 1 Sep 2021 19:01:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 22C198D0001; Wed, 1 Sep 2021 19:01:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 11C418D0002; Wed, 1 Sep 2021 19:01:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0144.hostedemail.com [216.40.44.144]) by kanga.kvack.org (Postfix) with ESMTP id 022628D0001 for ; Wed, 1 Sep 2021 19:01:15 -0400 (EDT) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id B70748249980 for ; Wed, 1 Sep 2021 23:01:14 +0000 (UTC) X-FDA: 78540527268.26.52FAB2D Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf10.hostedemail.com (Postfix) with ESMTP id 237016001994 for ; Wed, 1 Sep 2021 23:00:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1630537211; 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=560IcZXSiBf9qDG9Tl/mVQRipFa5V94OKcciALLHTxI=; b=PaRcwsd4nmA+Kyf0Ej9bPSGDtam8XIPYv2J5ePXNCYEcWJb31ncP/PUmoWWo3fn81vGAHt TSYogHkos5PU/0yLvp867YTyBpqM65zZr7lNKXEErVYhJIQ43WTahDJnAwos7kaQsATZ7F Q8ql1x/C6R7pqFU1Y2NdSk/z8Ec5dUg= 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-505-v_SxsSAHPAaf8DTImRmmyQ-1; Wed, 01 Sep 2021 19:00:08 -0400 X-MC-Unique: v_SxsSAHPAaf8DTImRmmyQ-1 Received: by mail-qk1-f198.google.com with SMTP id q13-20020a05620a038d00b003d38f784161so42568qkm.8 for ; Wed, 01 Sep 2021 16:00:08 -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=560IcZXSiBf9qDG9Tl/mVQRipFa5V94OKcciALLHTxI=; b=aqjt+xHTk7CpdltEKIeaX9ckMbhqXd4W6Xiuh+Yh/DtIG52UVBLuZZkjFsQFU1Q0XV pN8khFZjFG4tjGueoFJ94AfuHftryh9HU1Lc1sSjd393BoK9M7LyZZbTacK0m3fE+mkX gnJOGFPaG/pO2woCyYkfzY6x/mX+ZSkkc19nXOWPZDYjBonmZetoU1L+1cFdYLgErn77 uxC1tfww4HJ5NvBJPvRz3hG4yeJKbyqLlp8SMyR4tMOS284FykLeRg5eL2wbVuCebULV Xz0nWadcUsiK1mpAK/Pm8KBDjDc073/X46OfIeX/SVwslVwUPkGMacn8Q9+uCFv49nG6 GIzw== X-Gm-Message-State: AOAM532HeILX21TJJSKybv2gSeiR5p0bG2hUz4JC+33j63YXa7+foPil Uv/T8qvyJZGJobU7+eHse/1yQeI3Cju53IBX+16Q8qP6JZ6R5WRU/lQq36OgTzDztHLMsEXd/SS 0CAYBYwC7jq0= X-Received: by 2002:ac8:6ec9:: with SMTP id f9mr202365qtv.2.1630537208442; Wed, 01 Sep 2021 16:00:08 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxbz8PSQQW0BjBZE+Ytxvd/XDrQximTXqbhGRrX8eSbVjhoHPq5Tgw6KPxDY7cuY+yd6RRDSA== X-Received: by 2002:ac8:6ec9:: with SMTP id f9mr202327qtv.2.1630537208115; Wed, 01 Sep 2021 16:00:08 -0700 (PDT) Received: from t490s ([2607:fea8:56a3:500::ad7f]) by smtp.gmail.com with ESMTPSA id g8sm74883qkm.25.2021.09.01.16.00.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Sep 2021 16:00:07 -0700 (PDT) Date: Wed, 1 Sep 2021 19:00:05 -0400 From: Peter Xu To: Axel Rasmussen 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 Subject: Re: [PATCH 1/5] mm/shmem: Unconditionally set pte dirty in mfill_atomic_install_pte Message-ID: References: <20210901205622.6935-1-peterx@redhat.com> <20210901205622.6935-2-peterx@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=PaRcwsd4; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf10.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 216.205.24.124) smtp.mailfrom=peterx@redhat.com X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 237016001994 X-Stat-Signature: wmhzaxqmcy34rmpmx1cbrgrcjzgzbopa X-HE-Tag: 1630537211-277163 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, Axel, On Wed, Sep 01, 2021 at 02:48:53PM -0700, Axel Rasmussen wrote: > 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! No problem. Obviously that special handling of SetPageDirty is still too tricky to me and I'd love to remove it. > > 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. ;) ) Thanks for looking into this. I thought Hugh's explanation should mostly have covered that. The previous worry is we may have mprotect() applying write bit errornously if we have some read-only pte marked dirty. But I don't think that'll happen just like Hugh stated in the thread I attached, as the dirty accountable flag is only set if vma_wants_writenotify() returns true. Take the first example within that helper: if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED))) return 0; So firstly it never applies to vma that doesn't have VM_WRITE|VM_SHARED. So far it even doesn't work for anonymous, but logically it may, like: https://github.com/aagit/aa/commit/05dc2c56ef79b3836c75fcf68c5b19b08f4e4c58 Peter Collingbourne originated that patch, due to some reason it didn't land which I forgot, however I still think it's doable even for anonymous. Sorry to have gone off-topic; let me go back to it. It also checks for e.g. page_mkwrite() needs, soft dirty tracking and so on to make sure it's okay to grant write bit when possible. Hugh mentioned "do some work to assure us all of that" - I did firstly went throught the code carefully myself so I'm more certain it's doing the right thing to me, secondly I did run quite some tests on the patch (actually on the whole uffd-wp shmem+hugetlbfs branch). Even if I'm going to switch the uffd-wp series to the pte marker format, this patch won't change. I also analysized three callers that may be affected by this change below, and explaining why it's okay. I hope that can also be counted as part of the "some work" that Hugh asked. Besides all these, I'm pretty happy too if anyone would help me to tell otherwise on whether there's still things missing so we can't do this. That's the "code review" part for every single patch, including this one, isn't it? :) Thanks, > > > > > > > > > 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. -- Peter Xu