+CC Huang Ying,
On 2024/10/23 6:56, Barry Song wrote:
On Wed, Oct 23, 2024 at 4:10 AM Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:
...
On 2024/10/17 23:09, Matthew Wilcox wrote:
On Thu, Oct 17, 2024 at 10:25:04PM +0800, Kefeng Wang wrote:
Directly use folio_zero_range() to cleanup code.
Are you sure there's no performance regression introduced by this?
clear_highpage() is often optimised in ways that we can't optimise for
a plain memset(). On the other hand, if the folio is large, maybe a
modern CPU will be able to do better than clear-one-page-at-a-time.
Right, I missing this, clear_page might be better than memset, I change
this one when look at the shmem_writepage(), which already convert to
use folio_zero_range() from clear_highpage(), also I grep
folio_zero_range(), there are some other to use folio_zero_range().
fs/bcachefs/fs-io-buffered.c: folio_zero_range(folio, 0,
folio_size(folio));
fs/bcachefs/fs-io-buffered.c: folio_zero_range(f,
0, folio_size(f));
fs/bcachefs/fs-io-buffered.c: folio_zero_range(f,
0, folio_size(f));
fs/libfs.c: folio_zero_range(folio, 0, folio_size(folio));
fs/ntfs3/frecord.c: folio_zero_range(folio, 0,
folio_size(folio));
mm/page_io.c: folio_zero_range(folio, 0, folio_size(folio));
mm/shmem.c: folio_zero_range(folio, 0, folio_size(folio));
IOW, what performance testing have you done with this patch?
No performance test before, but I write a testcase,
1) allocate N large folios (folio_alloc(PMD_ORDER))
2) then calculate the diff(us) when clear all N folios
clear_highpage/folio_zero_range/folio_zero_user
3) release N folios
the result(run 5 times) shown below on my machine,
N=1,
clear_highpage folio_zero_range folio_zero_user
1 69 74 177
2 57 62 168
3 54 58 234
4 54 58 157
5 56 62 148
avg 58 62.8 176.8
N=100
clear_highpage folio_zero_range folio_zero_user
1 11015 11309 32833
2 10385 11110 49751
3 10369 11056 33095
4 10332 11017 33106
5 10483 11000 49032
avg 10516.8 11098.4 39563.4
N=512
clear_highpage folio_zero_range folio_zero_user
1 55560 60055 156876
2 55485 60024 157132
3 55474 60129 156658
4 55555 59867 157259
5 55528 59932 157108
avg 55520.4 60001.4 157006.6
folio_zero_user with many cond_resched(), so time fluctuates a lot,
clear_highpage is better folio_zero_range as you said.
Maybe add a new helper to convert all folio_zero_range(folio, 0,
folio_size(folio))
to use clear_highpage + flush_dcache_folio?
If this also improves performance for other existing callers of
folio_zero_range(), then that's a positive outcome.
...
hi Kefeng,
what's your point? providing a helper like clear_highfolio() or similar?
Yes, from above test, using clear_highpage/flush_dcache_folio is better
than using folio_zero_range() for folio zero(especially for large
folio), so I'd like to add a new helper, maybe name it folio_zero()
since it zero the whole folio.
we already have a helper like folio_zero_user()?
it is not good enough?
Since it is with many cond_resched(), the performance is worst...
Not exactly? It should have zero cost for a preemptible kernel.
For a non-preemptible kernel, it helps avoid clearing the folio
from occupying the CPU and starving other processes, right?
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2393,10 +2393,7 @@ static int shmem_get_folio_gfp(struct inode
*inode, pgoff_t index,
* it now, lest undo on failure cancel our earlier guarantee.
*/
if (sgp != SGP_WRITE && !folio_test_uptodate(folio)) {
- long i, n = folio_nr_pages(folio);
-
- for (i = 0; i < n; i++)
- clear_highpage(folio_page(folio, i));
+ folio_zero_user(folio, vmf->address);
flush_dcache_folio(folio);
folio_mark_uptodate(folio);
}
Do we perform better or worse with the following?
Here is for SGP_FALLOC, vmf = NULL, we could use folio_zero_user(folio,
0), I think the performance is worse, will retest once I can access
hardware.
Perhaps, since the current code uses clear_hugepage(). Does using
index << PAGE_SHIFT as the addr_hint offer any benefit?
when use folio_zero_user(), the performance is vary bad with above
fallocate test(mount huge=always),
folio_zero_range clear_highpage folio_zero_user
real 0m1.214s 0m1.111s 0m3.159s
user 0m0.000s 0m0.000s 0m0.000s
sys 0m1.210s 0m1.109s 0m3.152s
I tried with addr_hint = 0/index << PAGE_SHIFT, no obvious different.
Interesting. Does your kernel have preemption disabled or
preemption_debug enabled?
ARM64 server, CONFIG_PREEMPT_NONE=y
this explains why the performance is much worse.
If not, it makes me wonder whether folio_zero_user() in
alloc_anon_folio() is actually improving performance as expected,
compared to the simpler folio_zero() you plan to implement. :-)
Yes, maybe, the folio_zero_user(was clear_huge_page) is from
47ad8475c000 ("thp: clear_copy_huge_page"), so original clear_huge_page
is used in HugeTLB, clear PUD size maybe spend many time, but for PMD or
other size of large folio, cond_resched is not necessary since we
already have some folio_zero_range() to clear large folio, and no issue
was reported.
probably worth an optimization. calling cond_resched() for each page
seems too aggressive and useless.
After some test, I think the cond_resched() is not the root cause,
no performance gained with batched cond_resched(), even I kill
cond_resched() from process_huge_page, no improvement.
But when I unconditionally use clear_gigantic_page() in
folio_zero_user(patched), there is big improvement with above
fallocate on tmpfs(mount huge=always), also I test some other testcase,
1) case-anon-w-seq-mt: (2M PMD THP)
base:
real 0m2.490s 0m2.254s 0m2.272s
user 1m59.980s 2m23.431s 2m18.739s
sys 1m3.675s 1m15.462s 1m15.030s
patched:
real 0m2.234s 0m2.225s 0m2.159s
user 2m56.105s 2m57.117s 3m0.489s
sys 0m17.064s 0m17.564s 0m16.150s
Patched kernel win on sys and bad in user, but real is almost same,
maybe a little better than base.
2) case-anon-w-seq-hugetlb:(2M PMD HugeTLB)
base:
real 0m5.175s 0m5.117s 0m4.856s
user 5m15.943s 5m7.567s 4m29.273s
sys 2m38.503s 2m21.949s 2m21.252s
patched:
real 0m4.966s 0m4.841s 0m4.561s
user 6m30.123s 6m9.516s 5m49.733s
sys 0m58.503s 0m47.847s 0m46.785s
This case is similar to the case1.
3) fallocate hugetlb 20G (2M PMD HugeTLB)
base:
real 0m3.016s 0m3.019s 0m3.018s
user 0m0.000s 0m0.000s 0m0.000s
sys 0m3.009s 0m3.012s 0m3.010s
patched:
real 0m1.136s 0m1.136s 0m1.136s
user 0m0.000s 0m0.000s 0m0.004s
sys 0m1.133s 0m1.133s 0m1.129s
There is big win on patched kernel, and it is similar to above tmpfs
test, so maybe we could revert the commit c79b57e462b5 ("mm: hugetlb:
clear target sub-page last when clearing huge page").
