Re: [PATCH v3 01/15] mm: Batch-copy PTE ranges during fork()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05.12.23 12:30, Ryan Roberts wrote:
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@xxxxxxxxxx

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).

Not calling the batched variants but essentially doing what your code does (with some minor improvements, like updating the rss counters only once).

The snipped above is only linked below. I had the performance numbers for [1] ready, so I gave it a test on top of that.

To keep it simple, you might just benchmark w and w/o your patches.


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.

Essentially just allocating 1 GiB of memory an measuring how long it takes to call fork().

order-0 benchmarks:

https://gitlab.com/davidhildenbrand/scratchspace/-/raw/main/order-0-benchmarks.c?ref_type=heads

e.g.,: $ ./order-0-benchmarks fork 100


pte-mapped-thp benchmarks:

https://gitlab.com/davidhildenbrand/scratchspace/-/raw/main/pte-mapped-thp-benchmarks.c?ref_type=heads

e.g.,: $ ./pte-mapped-thp-benchmarks fork 100


Ideally, pin to one CPU and get stable performance numbers by disabling SMT+turbo etc.



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...

Not necessarily. It certainly cleans up the code, but we can do that in any order reasonable.


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


Yes. There are "unmap" and "pte-dontneed" benchmarks contained in both files above.

--
Cheers,

David / dhildenb





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux