Message-ID: <Y1sMk30wS+1uH/hc@x1n>
Date:   Thu, 27 Oct 2022 18:56:19 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Yuanzheng Song <songyuanzheng@...wei.com>,
        akpm@...ux-foundation.org, gregkh@...uxfoundation.org,
        david@...hat.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH STABLE 5.10] mm/memory: add non-anonymous page check in
 the copy_present_page()

On Thu, Oct 27, 2022 at 02:58:02PM -0700, Hugh Dickins wrote:
> Let me delete stable from the Cc, this discussion is not for stable.
> 
> On Thu, 27 Oct 2022, Peter Xu wrote:
> > On Wed, Oct 26, 2022 at 06:48:29PM -0700, Hugh Dickins wrote:
> > > 
> > > And I imagined that the correct fix (short of going forward with David's
> > > full changes) would be to back out to a context where one could add an
> > > anon_vma_prepare(), then retry after that - involves dropping pt lock,
> > > maybe gets nasty (tedious, anyway).
> > 
> > Right, that looks a larger changeset with minimum benefit - the page is
> > still inconsistent before fork(), and also for users don't fork() at all
> > after the RO pin.
> 
> Sorry, I don't understand any of what you're saying there: but you appear
> to be saying ("larger changeset with minimum benefit") that my suggestion
> would not be worth the effort - fair enough, but...
> 
> > 
> > It looks to me Hugh's suggestion would be the best suite here for stable.
> > Yuanzheng, what do you think?
> 
> ... now you appear to be saying it would be worth the effort.  Oh,
> perhaps you're referring to just the change to check dst anon_vma:
> perhaps, but I'm really having to guess at what you mean.

Sorry for not being clear.  Yes I was referring to that original idea of
using dest->anon_vma.

> 
> But none of that matters as much as below...
> 
> > 
> > For the long term I think we should wait for David's further unshare work
> > so gup_must_unshare() will work for page caches too while mapped private.
> 
> I do wonder if in the long term we shall have to port all David's work
> back to 5.15 and 5.10 (but I think there's yet more to come from him).
> But set aside that thought for now...
> 
> More urgently, in the short term:
> 
> Peter, you've made no reference to David's mail, where he concludes that
> Yuanzheng's !PageAnon patch is the appropriate one; and
> David, you've made no reference to Peter's mail, where he concludes that
> my doubts were correct, and it needs a different patch.
> 
> You appear to disagree over whether a RO-pinned file page needs to
> be copied at fork() time.  I don't know, but I hope you can agree
> on that (in the 5.10 and 5.15 context: maybe David is thinking of
> the 6.0 context and Peter of the 5.10 context) before going further.
> 
> (I'm hoping David is right, and I was plain wrong, since that's easiest.)

For some reason I thought David was talking about the plan for the latest..

The major difference IIUC is whether we'll CoW for page caches during
fork() with the old kernels or not with the two approaches (PageAnon check,
or dst->anon_vma check).

After a re-read and 2nd thought, I think David has a valid point in that we
shouldn't have special handling of !anon pages on CoW during fork(),
because that seems to be against the fundamental concept of fork().

So now I think I agree the !Anon original check does look a bit cleaner,
and also make fork() behavior matching with the old/new kernels, irrelevant
of the pin mess.

Thanks,

-- 
Peter Xu

Powered by blists - more mailing lists