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;
MIME-Version: 1.0
References: <20201209163950.8494-1-will@kernel.org> <20201209163950.8494-2-will@kernel.org>
 <CAHk-=wgos=vgteG52=J=rVSeq6-Y2g2+Kn1=xV=wYjVzM6O9UQ@mail.gmail.com>
 <20201209184049.GA8778@willie-the-truck> <CAHk-=wgVqGh402dxfhR=bx2QSH=+4kq9doarNmD77baqDKdiUg@mail.gmail.com>
 <20201210150828.4b7pg5lx666r7l2u@black.fi.intel.com>
In-Reply-To: <20201210150828.4b7pg5lx666r7l2u@black.fi.intel.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 10 Dec 2020 09:23:53 -0800
Message-ID: <CAHk-=wiU8ktvak2hCj2TWJ6wMSwVsUSvi5Bjf4i1JGvpGmyUZw@mail.gmail.com>
Subject: Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     Will Deacon <will@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Jan Kara <jack@suse.cz>, Minchan Kim <minchan@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vinayak Menon <vinmenon@codeaurora.org>,
        Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 10, 2020 at 7:08 AM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> See lightly tested patch below. Is it something you had in mind?

This is closer, in that at least it removes the ostensibly blocking
allocation (that can't happen) from the prefault path.

But the main issue remains:

> > At that point, I think the current very special and odd
> > do_fault_around() pre-allocation could be made into just a _regular_
> > "allocate the pmd if it doesn't exist". And then the pte locking could
> > be moved into filemap_map_pages(), and suddenly the semantics and
> > rules around all that would be a whole lot more obvious.
>
> No. It would stop faultaround code from mapping huge pages. We had to
> defer pte page table mapping until we know we don't have huge pages in
> page cache.

Can we please move that part to the callers too - possibly with a
separate helper function?

Because the real issue remains: as long the map_set_pte() function
takes the pte lock, the caller cannot rely on it.

And the filemap_map_pages() code really would like to rely on it.
Because if the lock is taken there *above* the loop - or even in the
loop iteration at the top, the code can now do things that rely on "I
know I hold the page table lock".

In particular, we can get rid of that very very expensive page locking.

Which is the reason I know about the horrid current issue with
"pre-allocate in one place, lock in another, and know we are atomic in
a third place" issue. Because I had to walk down these paths and
realize that "this loop is run under the page table lock, EXCEPT for
the first iteration, where it's taken by the first time we do that
non-allocating alloc_set_pte()".

See?

             Linus