| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87jyyjv5zy.fsf@oracle.com>
Date: Thu, 18 Dec 2025 13:23:45 -0800
From: Ankur Arora <ankur.a.arora@...cle.com>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: "David Hildenbrand (Red Hat)" <david@...nel.org>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
akpm@...ux-foundation.org, bp@...en8.de, dave.hansen@...ux.intel.com,
hpa@...or.com, mingo@...hat.com, mjguzik@...il.com, luto@...nel.org,
peterz@...radead.org, tglx@...utronix.de, willy@...radead.org,
raghavendra.kt@....com, chleroy@...nel.org, ioworker0@...il.com,
boris.ostrovsky@...cle.com, konrad.wilk@...cle.com
Subject: Re: [PATCH v10 8/8] mm: folio_zero_user: cache neighbouring pages
Ankur Arora <ankur.a.arora@...cle.com> writes:
> David Hildenbrand (Red Hat) <david@...nel.org> writes:
>
>> On 12/15/25 21:49, Ankur Arora wrote:
>>> folio_zero_user() does straight zeroing without caring about
>>> temporal locality for caches.
>>> This replaced commit c6ddfb6c5890 ("mm, clear_huge_page: move order
>>> algorithm into a separate function") where we cleared a page at a
>>> time converging to the faulting page from the left and the right.
>>> To retain limited temporal locality, split the clearing in three
>>> parts: the faulting page and its immediate neighbourhood, and, the
>>> remaining regions on the left and the right. The local neighbourhood
>>> will be cleared last.
>>> Do this only when zeroing small folios (< MAX_ORDER_NR_PAGES) since
>>> there isn't much expectation of cache locality for large folios.
>>> Performance
>>> ===
>>> AMD Genoa (EPYC 9J14, cpus=2 sockets * 96 cores * 2 threads,
>>> memory=2.2 TB, L1d= 16K/thread, L2=512K/thread, L3=2MB/thread)
>>> anon-w-seq (vm-scalability):
>>> stime utime
>>> page-at-a-time 1654.63 ( +- 3.84% ) 811.00 ( +- 3.84% )
>>> contiguous clearing 1602.86 ( +- 3.00% ) 970.75 ( +- 4.68% )
>>> neighbourhood-last 1630.32 ( +- 2.73% ) 886.37 ( +- 5.19% )
>>> Both stime and utime respond in expected ways. stime drops for both
>>> contiguous clearing (-3.14%) and neighbourhood-last (-1.46%)
>>> approaches. However, utime increases for both contiguous clearing
>>> (+19.7%) and neighbourhood-last (+9.28%).
>>> In part this is because anon-w-seq runs with 384 processes zeroing
>>> anonymously mapped memory which they then access sequentially. As
>>> such this is likely an uncommon pattern where the memory bandwidth
>>> is saturated while also being cache limited because we access the
>>> entire region.
>>> Kernel make workload (make -j 12 bzImage):
>>> stime utime
>>> page-at-a-time 138.16 ( +- 0.31% ) 1015.11 ( +- 0.05% )
>>> contiguous clearing 133.42 ( +- 0.90% ) 1013.49 ( +- 0.05% )
>>> neighbourhood-last 131.20 ( +- 0.76% ) 1011.36 ( +- 0.07% )
>>> For make the utime stays relatively flat with an up to 4.9% improvement
>>> in the stime.
>>
>> Nice evaluation!
>>
>>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.com>
>>> Reviewed-by: Raghavendra K T <raghavendra.kt@....com>
>>> Tested-by: Raghavendra K T <raghavendra.kt@....com>
>>> ---
>>> mm/memory.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 974c48db6089..d22348b95227 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -7268,13 +7268,53 @@ static void clear_contig_highpages(struct page *page, unsigned long addr,
>>> * @addr_hint: The address accessed by the user or the base address.
>>> *
>>> * Uses architectural support to clear page ranges.
>>> + *
>>> + * Clearing of small folios (< MAX_ORDER_NR_PAGES) is split in three parts:
>>> + * pages in the immediate locality of the faulting page, and its left, right
>>> + * regions; the local neighbourhood is cleared last in order to keep cache
>>> + * lines of the faulting region hot.
>>> + *
>>> + * For larger folios we assume that there is no expectation of cache locality
>>> + * and just do a straight zero.
>>
>> Just wondering: why not do the same thing here as well? Probably shouldn't hurt
>> and would get rid of some code?
>
> That's a good point. With only a three way split, there's no reason to
> treat large folios specially.
A bit more on this: this change makes sense but I'll retain the current
split between patches-7, 8.
Where patch-7, is used to justify using contiguous clearing (and the
choice of value for PROCESS_PAGES_NON_PREEMPT_BATCH), unit based on
preemption model etc and patch-8, for the neighbourhood optimization.
>>> */
>>> void folio_zero_user(struct folio *folio, unsigned long addr_hint)
>>> {
>>> unsigned long base_addr = ALIGN_DOWN(addr_hint, folio_size(folio));
>>
>> While at it you could turn that const as well.
>
> Ack.
>
>>> + const long fault_idx = (addr_hint - base_addr) / PAGE_SIZE;
>>> + const struct range pg = DEFINE_RANGE(0, folio_nr_pages(folio) - 1);
>>> + const int width = 2; /* number of pages cleared last on either side */
>>
>> Is "width" really the right terminology? (the way you describe it, it's more
>> like diameter?)
>
> I like diameter. Will make that a define.
I'll make that radius since that's how I'm using it.
Thanks
Ankur
>> Wondering whether we should turn that into a define to make it clearer that we
>> are dealing with a magic value.
>>
>> Speaking of magic values, why 2 and not 3? :)
>
> I think I had tried both :). The performance was pretty much the same.
>
> But also, this is probably a function of the benchmark used. And I'm
> not sure I have a good one (unless kernel build with THP counts which
> isn't very responsive).
>
>>> + struct range r[3];
>>> + int i;
>>> - clear_contig_highpages(folio_page(folio, 0),
>>> - base_addr, folio_nr_pages(folio));
>>> + if (folio_nr_pages(folio) > MAX_ORDER_NR_PAGES) {
>>> + clear_contig_highpages(folio_page(folio, 0),
>>> + base_addr, folio_nr_pages(folio));
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * Faulting page and its immediate neighbourhood. Cleared at the end to
>>> + * ensure it sticks around in the cache.
>>> + */
>>> + r[2] = DEFINE_RANGE(clamp_t(s64, fault_idx - width, pg.start, pg.end),
>>> + clamp_t(s64, fault_idx + width, pg.start, pg.end));
>>> +
>>> + /* Region to the left of the fault */
>>> + r[1] = DEFINE_RANGE(pg.start,
>>> + clamp_t(s64, r[2].start-1, pg.start-1, r[2].start));
>>
>> "start-1" -> "start - 1" etc.
>>
>>> +
>>> + /* Region to the right of the fault: always valid for the common fault_idx=0 case. */
>>> + r[0] = DEFINE_RANGE(clamp_t(s64, r[2].end+1, r[2].end, pg.end+1),
>>> + pg.end);
>>
>> Same here.
>>
>>> +
>>> + for (i = 0; i <= 2; i++) {
>>
>> Can we use ARRAY_SIZE instead of "2" ?
>
> Ack to all of the these.
>
>>> + unsigned int npages = range_len(&r[i]);
>>> + struct page *page = folio_page(folio, r[i].start);
>>> + unsigned long addr = base_addr + folio_page_idx(folio, page) * PAGE_SIZE;
>>
>> Can't you compute that from r[i].start) instead? The folio_page_idx() seems
>> avoidable unless I am missing something.
>>
>> Could make npages and addr const.
>>
>> const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
>> const unsigned int npages = range_len(&r[i]);
>> struct page *page = folio_page(folio, r[i].start);
>
> Thanks. Yeah, all of this makes sense.
>
> Will fix.
>
>>> +
>>> + if (npages > 0)
>>> + clear_contig_highpages(page, addr, npages);
>>> + }
>>> }
>>> static int copy_user_gigantic_page(struct folio *dst, struct folio *src,
>
> Thanks for the review!
--
ankur
Powered by blists - more mailing lists