Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 24 Jan 2022 16:54:29 +0800
From:   Peter Xu <peterx@redhat.com>
To:     Hugh Dickins <hughd@google.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        David Hildenbrand <david@redhat.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Yang Shi <shy828301@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Andrew Morton <akpm@linux-foundation.org>,
        Alistair Popple <apopple@nvidia.com>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details
 specified
Message-ID: <Ye5pRQNRv53HWmSZ@xz-m1.local>
References: <20211115134951.85286-1-peterx@redhat.com>
 <20211115134951.85286-2-peterx@redhat.com>
 <9937aaa-d9ab-2839-b0b7-691d85c9141@google.com>
 <Yd7VEXI+/Lu8MVOo@xz-m1.local>
 <Yd7W6ndSPkXQjurY@xz-m1.local>
 <391aa58d-ce84-9d4-d68d-d98a9c533255@google.com>
 <Yek6PaW8fAXY4Bft@xz-m1.local>
 <93dd745c-5e8b-a50-4ec5-b3f3728ad8b@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <93dd745c-5e8b-a50-4ec5-b3f3728ad8b@google.com>
Precedence: bulk

On Sun, Jan 23, 2022 at 10:29:40PM -0800, Hugh Dickins wrote:
> > However, as I stated before, all these use cases always have another step to
> > take the lock and redo the range.  Then even if some migration entry got
> > wrongly skipped it'll always be fixed.  What we need to find is some caller
> > that calls zap_pte_range() without later taking the page lock and redo that.
> > That's the only possibility to trigger a real issue on the shmem accounting.
> 
> I agree that the fallback "if (folio_mapped() unmap_mapping_folio()",
> while holding folio lock, ensures that there cannot be a migration entry
> substituted for present pte at that time, so no problem if migration entry
> was wrongly skipped on the earlier unlocked pass.
> 
> But you're forgetting the complementary mistake: that the earlier unlocked
> pass might have zapped a migration entry (corresponding to an anon COWed
> page) when it should have skipped it (while punching a hole).

IMHO we won't wrongly zap a migration entry because when it's file backed we've
got non-NULL zap_details, so we'll skip all migration entries.  IOW, we can
only wrongly skip some entries, not wrongly zap some.

But I get your point, and thanks for pointing out what I missed - I think I
forgot the private mappings completely somehow when writting that up..

I have a quick idea on reproducer now (perhaps file size shrinking on private
pages being swapped out), I'll try to write a real reproducer and update later.

[...]

> I did not understand what you were asking there; but in your followup
> mail, I think you came to understand what I meant better.  Yes, I
> believe you could safely replace struct address_space *zap_mapping
> by a more understandable boolean (skip_cows? its inverse would be
> easier to understand, but we don't want almost everyone to have to
> pass a zap_details initialized to true there).

The only even_cows==true for zap_details is with unmap_mapping_range(), where
its caller passed over even_cows==true as parameter.  So IMHO that helper
helped to construct the zap_details anyway.

I'll try it out starting with naming it zap_details.even_cows; I'll make it the
last patch as a cleanup.

> > > > > > >  			rss[mm_counter(page)]--;
> > > > > > >  		}
> > > 
> > > I have given no thought as to whether more "else"s are needed there.
> > 
> > It's hwpoison that's in the else.  Nothing else should.
> > 
> > I didn't mention it probably because I forgot.  I did think about it when
> > drafting, and IMHO we should simply zap that hwpoison entry because:
> > 
> >   (1) Zap means the user knows this data is meaningless, so at least we
> >       shouldn't SIGBUS for that anymore.
> > 
> >   (2) If we keep it there, it could errornously trigger SIGBUS later if the
> >       guest accessed that pte again somehow.
> > 
> > I plan to mention that in the commit message, but I can also add some comments
> > directly into the code.  Let me know your thoughts.
> 
> It's comes down, again, to what punching a hole in a file should do
> to the private data that has been COWed from it.  Strictly, it should
> not interfere with it, even when the COWed page has become poisonous:
> the entry should be left in to generate SIGBUS.  Whereas ordinary
> unmapping or truncating or MADV_DONTNEEDing would zap it.

Makes sense, I'll take care of that in the new version too.  Thanks,

-- 
Peter Xu