Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting

From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: Android Kernel Team <kernel-team@android.com>,
	Jan Kara <jack@suse.cz>, Minchan Kim <minchan@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Hugh Dickins <hughd@google.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Linux-MM <linux-mm@kvack.org>,
	Vinayak Menon <vinmenon@codeaurora.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Will Deacon <will@kernel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting
Date: Tue, 29 Dec 2020 12:52:58 -0800	[thread overview]
Message-ID: <CAHk-=wiyzbEEUF7PH4P2pufS60E0HSKs3Wdho7-gh-mGnOVwvg@mail.gmail.com> (raw)
In-Reply-To: <20201229132819.najtavneutnf7ajp@box>

On Tue, Dec 29, 2020 at 5:28 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> I reworked do_fault_around() so it doesn't touch vmf->address and pass
> original address down to ->map_pages(). No need in the new argument in
> ->map_pages. filemap_map_pages() calculates address based on pgoff of the
> page handled.

So I really like how the end result looks, but it worries me how many
problems this patch has had, and I'd love to try to make it more
incremental.

In particular, I really liked your re-write of the loop in
filemap_map_pages(), and how you separated out the pmd case to be a
separate early case. I think that was some of the nicest part of the
patch.

And I really like how you changed the vmf->address meaning in this
latest version, and pass the real address down, and pass the range
just in the start/end_pgoff. That just looks like the RightThing(tm)
to do.

But I think these nice cleanups could and should be independent of
some of the other changes.

The unlocking change, for example, that changed the return type from
void to vm_fault_t, also looks like a nice cleanup, but that's also
the one that caused the last series of odd problems for Hugh.

The fix for that then also caused that ugly "goto out" mess, which
undid some of the "this looks really nice" effects.

I don't even see the need for that "goto out". Yes, it resets the
'vmf->address' back to what it should be (so that __do_fault()
fallback can work right), but the two first "goto out" cases haven't
actually changed it (or ther mmap_miss), so why did they need to do
that?

Anyway, I really like how it all looks (apart from that odd "goto out"
detail - nitpicking here) but it does worry me that it changes so much
and had so many small problems..

         Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel