Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <a81dc390-8b10-4ce9-b72f-57f253e77af3@arm.com>
Date:   Tue, 5 Dec 2023 11:30:56 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 01/15] mm: Batch-copy PTE ranges during fork()
Content-Language: en-GB
To:     David Hildenbrand <david@redhat.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Marc Zyngier <maz@kernel.org>,
        Oliver Upton <oliver.upton@linux.dev>,
        James Morse <james.morse@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Matthew Wilcox <willy@infradead.org>,
        Yu Zhao <yuzhao@google.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        John Hubbard <jhubbard@nvidia.com>, Zi Yan <ziy@nvidia.com>,
        Barry Song <21cnbao@gmail.com>,
        Alistair Popple <apopple@nvidia.com>,
        Yang Shi <shy828301@gmail.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20231204105440.61448-1-ryan.roberts@arm.com>
 <20231204105440.61448-2-ryan.roberts@arm.com>
 <a12ce4f8-feb0-4e35-8f55-9270fe5a808b@redhat.com>
 <104de2d6-ecf9-4b0c-a982-5bd8e1aea758@redhat.com>
 <5b8b9f8c-8e9b-42a5-b8b2-9b96903f3ada@redhat.com>
From:   Ryan Roberts <ryan.roberts@arm.com>
In-Reply-To: <5b8b9f8c-8e9b-42a5-b8b2-9b96903f3ada@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 04/12/2023 17:27, David Hildenbrand wrote:
>>
>> With rmap batching from [1] -- rebased+changed on top of that -- we could turn
>> that into an effective (untested):
>>
>>           if (page && folio_test_anon(folio)) {
>> +               nr = folio_nr_pages_cont_mapped(folio, page, src_pte, addr, end,
>> +                                               pte, enforce_uffd_wp, &nr_dirty,
>> +                                               &nr_writable);
>>                   /*
>>                    * If this page may have been pinned by the parent process,
>>                    * copy the page immediately for the child so that we'll always
>>                    * guarantee the pinned page won't be randomly replaced in the
>>                    * future.
>>                    */
>> -               folio_get(folio);
>> -               if (unlikely(folio_try_dup_anon_rmap_pte(folio, page,
>> src_vma))) {
>> +               folio_ref_add(folio, nr);
>> +               if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page, nr,
>> src_vma))) {
>>                           /* Page may be pinned, we have to copy. */
>> -                       folio_put(folio);
>> -                       return copy_present_page(dst_vma, src_vma, dst_pte,
>> src_pte,
>> -                                                addr, rss, prealloc, page);
>> +                       folio_ref_sub(folio, nr);
>> +                       ret = copy_present_page(dst_vma, src_vma, dst_pte,
>> +                                               src_pte, addr, rss, prealloc,
>> +                                               page);
>> +                       return ret == 0 ? 1 : ret;
>>                   }
>> -               rss[MM_ANONPAGES]++;
>> +               rss[MM_ANONPAGES] += nr;
>>           } else if (page) {
>> -               folio_get(folio);
>> -               folio_dup_file_rmap_pte(folio, page);
>> -               rss[mm_counter_file(page)]++;
>> +               nr = folio_nr_pages_cont_mapped(folio, page, src_pte, addr, end,
>> +                                               pte, enforce_uffd_wp, &nr_dirty,
>> +                                               &nr_writable);
>> +               folio_ref_add(folio, nr);
>> +               folio_dup_file_rmap_ptes(folio, page, nr);
>> +               rss[mm_counter_file(page)] += nr;
>>           }
>>
>>
>> We'll have to test performance, but it could be that we want to specialize
>> more on !folio_test_large(). That code is very performance-sensitive.
>>
>>
>> [1] https://lkml.kernel.org/r/20231204142146.91437-1-david@redhat.com
> 
> So, on top of [1] without rmap batching but with a slightly modified version of

Can you clarify what you mean by "without rmap batching"? I thought [1]
implicitly adds rmap batching? (e.g. folio_dup_file_rmap_ptes(), which you've
added in the code snippet above).

> yours (that keeps the existing code structure as pointed out and e.g., updates
> counter updates), running my fork() microbenchmark with a 1 GiB of memory:
> 
> Compared to [1], with all order-0 pages it gets 13--14% _slower_ and with all
> PTE-mapped THP (order-9) it gets ~29--30% _faster_.

What test are you running - I'd like to reproduce if possible, since it sounds
like I've got some work to do to remove the order-0 regression.

> 
> So looks like we really want to have a completely seprate code path for
> "!folio_test_large()" to keep that case as fast as possible. And "Likely" we
> want to use "likely(!folio_test_large()". ;)

Yuk, but fair enough. If I can repro the perf numbers, I'll have a go a
reworking this.

I think you're also implicitly suggesting that this change needs to depend on
[1]? Which is a shame...

I guess I should also go through a similar exercise for patch 2 in this series.

> 
> Performing rmap batching on top of that code only slightly (another 1% or so)
> improves performance in the PTE-mapped THP (order-9) case right now, in contrast
> to other rmap batching. Reason is as all rmap code gets inlined here and we're
> only doing subpage mapcount updates + PAE handling.
>