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;
Message-ID: <1bfec16f-76c6-9beb-26b2-ca508baa76a3@redhat.com>
Date:   Mon, 31 Jan 2022 10:56:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Content-Language: en-US
To:     Michel Lespinasse <michel@lespinasse.org>,
        Linux-MM <linux-mm@kvack.org>, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     kernel-team@fb.com, Laurent Dufour <ldufour@linux.ibm.com>,
        Jerome Glisse <jglisse@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Michal Hocko <mhocko@suse.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Matthew Wilcox <willy@infradead.org>,
        Liam Howlett <liam.howlett@oracle.com>,
        Rik van Riel <riel@surriel.com>,
        Paul McKenney <paulmck@kernel.org>,
        Song Liu <songliubraving@fb.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Minchan Kim <minchan@google.com>,
        Joel Fernandes <joelaf@google.com>,
        David Rientjes <rientjes@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Andy Lutomirski <luto@kernel.org>
References: <20220128131006.67712-1-michel@lespinasse.org>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v2 00/35] Speculative page faults
In-Reply-To: <20220128131006.67712-1-michel@lespinasse.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28.01.22 14:09, Michel Lespinasse wrote:

Hi Michel,

> This patchset is my take on speculative page faults (spf).
> It builds on ideas that have been previously proposed by Laurent Dufour,
> Peter Zijlstra and others before. While Laurent's previous proposal
> was rejected around the time of LSF/MM 2019, I am hoping we can revisit
> this now based on what I think is a simpler and more bisectable approach,
> much improved scaling numbers in the anonymous vma case, and the Android
> use case that has since emerged. I will expand on these points towards
> the end of this message.
> 
> The patch series applies on top of linux v5.17-rc1;
> a git tree is also available:
> git fetch https://github.com/lespinasse/linux.git v5.17-rc1-spf-anon
> 
> I would like these patches to be considered for inclusion into v5.18.

Just a general note: we certainly need (much more) review. And I think
we'll have to make a decision if the maintenance effort +  complexity
will be worth the benefit.

> Several android vendors are using Laurent Dufour's previous SPF work into
> their kernel tree in order to improve application startup performance,
> want to converge to an upstream accepted solution, and have reported good
> numbers with previous versions of this patchset. Also, there is a broader
> interest into reducing mmap lock dependencies in critical MM paths,
> and I think this patchset would be a good first step in that direction.
> 
> 
> This patchset follows the same overall structure as the v1 proposal,
> with the following differences:
> - Commit 12 (mm: separate mmap locked assertion from find_vma) is new.
> - The mmu notifier lock is new; this fixes a race in v1 patchset
>   between speculative COW faults and registering new MMU notifiers.
> - Speculative handling of swap-cache pages has been removed.
> - Commit 30 is new; this fixes build issues that showed in some configs.
> 
> 
> In principle it would also be possible to extend this work for handling
> file mapped vmas; I have pending work on such patches too but they are
> not mature enough to be submitted for inclusion at this point.
> 

I'd have expected a performance evaluation at this point, to highlight
the possible benefit and eventually also downsides, if any.

> 
> Patchset summary:
> 
> Classical page fault processing takes the mmap read lock in order to
> prevent races with mmap writers. In contrast, speculative fault
> processing does not take the mmap read lock, and instead verifies,
> when the results of the page fault are about to get committed and
> become visible to other threads, that no mmap writers have been
> running concurrently with the page fault. If the check fails,
> speculative updates do not get committed and the fault is retried
> in the usual, non-speculative way (with the mmap read lock held).
> 
> The concurrency check is implemented using a per-mm mmap sequence count.
> The counter is incremented at the beginning and end of each mmap write
> operation. If the counter is initially observed to have an even value,
> and has the same value later on, the observer can deduce that no mmap
> writers have been running concurrently with it between those two times.
> This is similar to a seqlock, except that readers never spin on the
> counter value (they would instead revert to taking the mmap read lock),
> and writers are allowed to sleep. One benefit of this approach is that
> it requires no writer side changes, just some hooks in the mmap write
> lock APIs that writers already use.
> 
> The first step of a speculative page fault is to look up the vma and
> read its contents (currently by making a copy of the vma, though in
> principle it would be sufficient to only read the vma attributes that
> are used in page faults). The mmap sequence count is used to verify
> that there were no mmap writers concurrent to the lookup and copy steps.
> Note that walking rbtrees while there may potentially be concurrent
> writers is not an entirely new idea in linux, as latched rbtrees
> are already doing this. This is safe as long as the lookup is
> followed by a sequence check to verify that concurrency did not
> actually occur (and abort the speculative fault if it did).
> 
> The next step is to walk down the existing page table tree to find the
> current pte entry. This is done with interrupts disabled to avoid
> races with munmap(). Again, not an entirely new idea, as this repeats
> a pattern already present in fast GUP. Similar precautions are also
> taken when taking the page table lock.
> 
> Breaking COW on an existing mapping may require firing MMU notifiers.
> Some care is required to avoid racing with registering new notifiers.
> This patchset adds a new per-cpu rwsem to handle this situation.

I have to admit that this sounds complicated and possibly dangerous to me.


Here is one of my concerns, I hope you can clarify:

GUP-fast only ever walks page tables and doesn't actually modify any
page table state, including, not taking page table locks which might not
reside in the memmap directly but in auxiliary data. It works because we
only ever drop the last reference to a page table (to free it) after we
synchronized against GUP-fast either via an IPI or synchronize_rcu(), as
GUP=fast disables interrupts.


I'd assume that taking page table locks on page tables that might no
longer be spanned by a VMA because of concurrent page table
deconstruction  is dangerous:


On munmap(), we do the VMA update under mmap_lock in write mode, to the
remove the page tables under mmap_lock in read mode.

Let's take a look at free_pte_range() on x86:

free_pte_range()
-> pte_free_tlb()
 -> tlb_flush_pmd_range()
  -> __tlb_adjust_range()
   /* Doesn't actually flush but only updates the tlb range */
 -> __pte_free_tlb()
  -> ___pte_free_tlb()
   -> pgtable_pte_page_dtor()
    -> ptlock_free()
    /* page table lock was freed */
   -> paravirt_tlb_remove_table()
    -> tlb_remove_page()
     -> tlb_remove_page_size()
      -> __tlb_remove_page_size()
       /* Page added to TLB batch flushing+freeing */

The later tlb_flush_mmu() via tlb_flush_mmu_free()->tlb_table_flush()
will the free the page tables, after synchronizing against GUP-fast. But
at that point we already deconstructed the page tables.

So just reading your summary here, what prevents in your approach taking
a page table lock with racing against page table lock freeing? I cannot
see how a seqcount would help.


IIUC, with what you propose we cannot easily have auxiliary data for a
page table, at least not via current pgtable_pte_page_dtor(), including
page locks, which is a drawback (and currently eventually a BUG in your
code?) at least for me. But I only read the cover letter, so I might be
missing something important :)

-- 
Thanks,

David / dhildenb