Re: [RFC v2 PATCH 00/17] variable-order, large folios for anonymous memory

[Ryan Roberts <ryan.roberts@arm.com>]

On 17/05/2023 14:58, David Hildenbrand wrote:
> On 26.04.23 12:41, Ryan Roberts wrote:
>> Hi David,
>>
>> On 17/04/2023 16:44, David Hildenbrand wrote:
>>
>>>>>>>
>>>>>>> So what should be safe is replacing all sub-pages of a folio that are marked
>>>>>>> "maybe shared" by a new folio under PT lock. However, I wonder if it's
>>>>>>> really
>>>>>>> worth the complexity. For THP we were happy so far to *not* optimize this,
>>>>>>> implying that maybe we shouldn't worry about optimizing the fork() case for
>>>>>>> now
>>>>>>> that heavily.
>>>>>>
>>>>>> I don't have the exact numbers to hand, but I'm pretty sure I remember
>>>>>> enabling
>>>>>> large copies was contributing a measurable amount to the performance
>>>>>> improvement. (Certainly, the zero-page copy case, is definitely a big
>>>>>> contributer). I don't have access to the HW at the moment but can rerun later
>>>>>> with and without to double check.
>>>>>
>>>>> In which test exactly? Some micro-benchmark?
>>>>
>>>> The kernel compile benchmark that I quoted numbers for in the cover letter. I
>>>> have some trace points (not part of the submitted series) that tell me how many
>>>> mappings of each order we get for each code path. I'm pretty sure I remember
>>>> all
>>>> of these 4 code paths contributing non-negligible amounts.
>>>
>>> Interesting! It would be great to see if there is an actual difference after
>>> patch #10 was applied without the other COW replacement.
>>>
>>
>> Sorry about the delay. I now have some numbers for this...
>>
> 
> Dito, I'm swamped :) Thanks for running these benchmarks!
> 
> As LSF/MM reminded me again of this topic ...
> 
>> I rearranged the patch order so that all the "utility" stuff (new rmap
>> functions, etc) are first (1, 2, 3, 4, 5, 8, 9, 11, 12, 13), followed by a
>> couple of general improvements (7, 17), which should be dormant until we have
>> the final patches, then finally (6, 10, 14, 15), which implement large anon
>> folios the allocate, reuse, copy-non-zero and copy-zero paths respectively. I've
>> dropped patch 16 and fixed the copy-exclusive bug you spotted (by ensuring we
>> never replace an exclusive page).
>>
>> I've measured performance at the following locations in the patch set:
>>
>> - baseline: none of my patches applied
>> - utility: has utility and general improvement patches applied
>> - alloc: utility + 6
>> - reuse: utility + 6 + 10
>> - copy: utility + 6 + 10 + 14
>> - zero-alloc: utility + 6 + 19 + 14 + 15
>>
>> The test is `make defconfig && time make -jN Image` for a clean checkout of
>> v6.3-rc3. The first result is thrown away, and the next 3 are kept. I saw some
>> per-boot variance (probably down to kaslr, etc). So have booted each kernel 7
>> times for a total of 3x7=21 samples per kernel. Then I've taken the mean:
>>
>>
>> jobs=8:
>>
>> | label      |   real |   user |   kernel |
>> |:-----------|-------:|-------:|---------:|
>> | baseline   |   0.0% |   0.0% |     0.0% |
>> | utility    |  -2.7% |  -2.8% |    -3.1% |
>> | alloc      |  -6.0% |  -2.3% |   -24.1% |
>> | reuse      |  -9.5% |  -5.8% |   -28.5% |
>> | copy       | -10.6% |  -6.9% |   -29.4% |
>> | zero-alloc |  -9.2% |  -5.1% |   -29.8% |
>>
>>
>> jobs=160:
>>
>> | label      |   real |   user |   kernel |
>> |:-----------|-------:|-------:|---------:|
>> | baseline   |   0.0% |   0.0% |     0.0% |
>> | utility    |  -1.8% |  -0.0% |    -7.7% |
>> | alloc      |  -6.0% |   1.8% |   -20.9% |
>> | reuse      |  -7.8% |  -1.6% |   -24.1% |
>> | copy       |  -7.8% |  -2.5% |   -26.8% |
>> | zero-alloc |  -7.7% |   1.5% |   -29.4% |
>>
>>
>> So it looks like patch 10 (reuse) is making a difference, but copy and
>> zero-alloc are not adding a huge amount, as you hypothesized. Personally I would
>> prefer not to drop those patches though, as it will all help towards utilization
>> of contiguous PTEs on arm64, which is the second part of the change that I'm now
>> working on.
> 
> Yes, pretty much what I expected :) I can only suggest to
> 
> (1) Make the initial support as simple and minimal as possible. That
>     means, strip anything that is not absolutely required. That is,
>     exclude *at least* copy and zero-alloc. We can always add selected
>     optimizations on top later.
> 
>     You'll do yourself a favor to get as much review coverage, faster
>     review for inclusion, and less chances for nasty BUGs.

As I'm building out the testing capability, I'm seeing that with different HW
configs and workloads, things move a bit and zero-alloc can account for up to 1%
in some cases. Copy is still pretty marginal, but I wonder if we might see more
value from it on Android where the Zygote is constantly forked?

Regardless, I appreciate your point about making the initial contribution as
small and simple as possible, so as I get closer to posting a v1, I'll keep it
in mind and make sure I follow your advice.

Thanks,
Ryan

> 
> (2) Keep the COW logic simple. We've had too many issues
>     in that area for my taste already. As 09854ba94c6a ("mm:
>     do_wp_page() simplification") from Linus puts it: "Simplify,
>     simplify, simplify.". If it doesn't add significant benefit, rather
>     keep it simple.
> 
>>
>>
>> For the final config ("zero-alloc") I also collected stats on how many
>> operations each of the 4 paths was performing, using ftrace and histograms.
>> "pnr" is the number of pages allocated/reused/copied, and "fnr" is the number of
>> pages in the source folio):
>>
>>
>> do_anonymous_page:
>>
>> { pnr:          1 } hitcount:    2749722
>> { pnr:          4 } hitcount:     387832
>> { pnr:          8 } hitcount:     409628
>> { pnr:         16 } hitcount:    4296115
>>
>> pages: 76315914
>> faults: 7843297
>> pages per fault: 9.7
>>
>>
>> wp_page_reuse (anon):
>>
>> { pnr:          1, fnr:          1 } hitcount:      47887
>> { pnr:          3, fnr:          4 } hitcount:          2
>> { pnr:          4, fnr:          4 } hitcount:       6131
>> { pnr:          6, fnr:          8 } hitcount:          1
>> { pnr:          7, fnr:          8 } hitcount:         10
>> { pnr:          8, fnr:          8 } hitcount:       3794
>> { pnr:          1, fnr:         16 } hitcount:         36
>> { pnr:          2, fnr:         16 } hitcount:         23
>> { pnr:          3, fnr:         16 } hitcount:          5
>> { pnr:          4, fnr:         16 } hitcount:          9
>> { pnr:          5, fnr:         16 } hitcount:          8
>> { pnr:          6, fnr:         16 } hitcount:          9
>> { pnr:          7, fnr:         16 } hitcount:          3
>> { pnr:          8, fnr:         16 } hitcount:         24
>> { pnr:          9, fnr:         16 } hitcount:          2
>> { pnr:         10, fnr:         16 } hitcount:          1
>> { pnr:         11, fnr:         16 } hitcount:          9
>> { pnr:         12, fnr:         16 } hitcount:          2
>> { pnr:         13, fnr:         16 } hitcount:         27
>> { pnr:         14, fnr:         16 } hitcount:          2
>> { pnr:         15, fnr:         16 } hitcount:         54
>> { pnr:         16, fnr:         16 } hitcount:       6673
>>
>> pages: 211393
>> faults: 64712
>> pages per fault: 3.3
>>
>>
>> wp_page_copy (anon):
>>
>> { pnr:          1, fnr:          1 } hitcount:      81242
>> { pnr:          4, fnr:          4 } hitcount:       5974
>> { pnr:          1, fnr:          8 } hitcount:          1
>> { pnr:          4, fnr:          8 } hitcount:          1
>> { pnr:          8, fnr:          8 } hitcount:      12933
>> { pnr:          1, fnr:         16 } hitcount:         19
>> { pnr:          4, fnr:         16 } hitcount:          3
>> { pnr:          8, fnr:         16 } hitcount:          7
>> { pnr:         16, fnr:         16 } hitcount:       4106
>>
>> pages: 274390
>> faults: 104286
>> pages per fault: 2.6
>>
>>
>> wp_page_copy (zero):
>>
>> { pnr:          1 } hitcount:     178699
>> { pnr:          4 } hitcount:      14498
>> { pnr:          8 } hitcount:      23644
>> { pnr:         16 } hitcount:     257940
>>
>> pages: 4552883
>> faults: 474781
>> pages per fault: 9.6
> 
> I'll have to set aside more time to digest these values :)
>