From: Ryan Roberts <ryan.roberts@arm.com>
To: John Hubbard <jhubbard@nvidia.com>,
Andrew Morton <akpm@linux-foundation.org>,
Matthew Wilcox <willy@infradead.org>,
Yin Fengwei <fengwei.yin@intel.com>,
David Hildenbrand <david@redhat.com>, Yu Zhao <yuzhao@google.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Anshuman Khandual <anshuman.khandual@arm.com>,
Yang Shi <shy828301@gmail.com>,
"Huang, Ying" <ying.huang@intel.com>, Zi Yan <ziy@nvidia.com>,
Luis Chamberlain <mcgrof@kernel.org>,
Itaru Kitayama <itaru.kitayama@gmail.com>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
David Rientjes <rientjes@google.com>,
Vlastimil Babka <vbabka@suse.cz>, Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v6 5/9] mm: thp: Extend THP to allocate anonymous large folios
Date: Wed, 1 Nov 2023 13:56:32 +0000 [thread overview]
Message-ID: <a6fa0847-a950-4044-972c-e5dc8cbc7922@arm.com> (raw)
In-Reply-To: <d07c45bb-4eba-484c-bba1-b586136f8cdb@nvidia.com>
On 30/10/2023 23:25, John Hubbard wrote:
> On 10/30/23 04:43, Ryan Roberts wrote:
>> On 28/10/2023 00:04, John Hubbard wrote:
>>> On 9/29/23 04:44, Ryan Roberts wrote:
> ...
>>>> +static bool vmf_pte_range_changed(struct vm_fault *vmf, int nr_pages)
>>>> +{
>>>> + int i;
>>>> +
>>>> + if (nr_pages == 1)
>>>> + return vmf_pte_changed(vmf);
>>>> +
>>>> + for (i = 0; i < nr_pages; i++) {
>>>> + if (!pte_none(ptep_get_lockless(vmf->pte + i)))
>>>> + return true;
>>>
>>> This seems like something different than the function name implies.
>>> It's really confusing: for a single page case, return true if the
>>> pte in the page tables has changed, yes that is very clear.
>>>
>>> But then for multiple page cases, which is really the main
>>> focus here--for that, claim that the range has changed if any
>>> pte is present (!pte_none). Can you please help me understand
>>> what this means?
>>
>> Yes I understand your confusion. Although I'm confident that the code is
>> correct, its a bad name - I'll make the excuse that this has evolved through
>> rebasing to cope with additions to UFFD. Perhaps something like
>> vmf_is_large_folio_suitable() is a better name.
>>
>> It used to be that we would only take the do_anonymous_page() path if the pte
>> was none; i.e. this is the first time we are faulting on an address covered by
>> an anon VMA and we need to allocate some memory. But more recently we also end
>> up here if the pte is a uffd_wp marker. So for a single pte, instead of checking
>> none, we can check if the pte has changed from our original check (where we
>> determined it was a uffd_wp marker or none). But for multiple ptes, we don't
>> have storage to store all the original ptes from the first check.
>>
>> Fortunately, if uffd is in use for a vma, then we don't want to use a large
>> folio anyway (this would break uffd semantics because we would no longer get a
>> fault for every page). So we only care about the "same but not none" case for
>> nr_pages=1.
>>
>> Would changing the name to vmf_is_large_folio_suitable() help here?
>
> Yes it would! And adding in a sentence or two from above about the uffd, as
> a function-level comment might be just the right of demystification for
> the code.
Actually I don't think the name I proposed it quite right either - this gets
called for small folios too.
I think its cleaner to change the name to vmf_pte_range_none() and strip out the
nr_pages==1 case. The checking-for-none part is required by alloc_anon_folio()
and needs to be safe without holding the PTL. vmf_pte_changed() is not safe in
without the lock. So I've just hoisted the nr_pages==1 case directly into
do_anonymous_page(). Shout if you think we can do better:
diff --git a/mm/memory.c b/mm/memory.c
index 569c828b1cdc..b48e4de1bf20 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4117,19 +4117,16 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
return ret;
}
-static bool vmf_pte_range_changed(struct vm_fault *vmf, int nr_pages)
+static bool pte_range_none(pte_t *pte, int nr_pages)
{
int i;
- if (nr_pages == 1)
- return vmf_pte_changed(vmf);
-
for (i = 0; i < nr_pages; i++) {
- if (!pte_none(ptep_get_lockless(vmf->pte + i)))
- return true;
+ if (!pte_none(ptep_get_lockless(pte + i)))
+ return false;
}
- return false;
+ return true;
}
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
@@ -4170,7 +4167,7 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf)
while (orders) {
addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << order);
vmf->pte = pte + pte_index(addr);
- if (!vmf_pte_range_changed(vmf, 1 << order))
+ if (pte_range_none(vmf->pte, 1 << order))
break;
order = next_order(&orders, order);
}
@@ -4280,7 +4277,8 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
if (!vmf->pte)
goto release;
- if (vmf_pte_range_changed(vmf, nr_pages)) {
+ if ((nr_pages == 1 && vmf_pte_changed(vmf)) ||
+ (nr_pages > 1 && !pte_range_none(vmf->pte, nr_pages))) {
for (i = 0; i < nr_pages; i++)
update_mmu_tlb(vma, addr + PAGE_SIZE * i, vmf->pte + i);
goto release;
next prev parent reply other threads:[~2023-11-01 13:56 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-29 11:44 [PATCH v6 0/9] variable-order, large folios for anonymous memory Ryan Roberts
2023-09-29 11:44 ` [PATCH v6 1/9] mm: Allow deferred splitting of arbitrary anon large folios Ryan Roberts
2023-10-05 8:19 ` David Hildenbrand
2023-09-29 11:44 ` [PATCH v6 2/9] mm: Non-pmd-mappable, large folios for folio_add_new_anon_rmap() Ryan Roberts
2023-09-29 13:45 ` Kirill A. Shutemov
2023-09-29 14:39 ` Ryan Roberts
2023-09-29 11:44 ` [PATCH v6 3/9] mm: thp: Account pte-mapped anonymous THP usage Ryan Roberts
2023-09-29 11:44 ` [PATCH v6 4/9] mm: thp: Introduce anon_orders and anon_always_mask sysfs files Ryan Roberts
2023-09-29 22:55 ` Andrew Morton
2023-10-02 10:15 ` Ryan Roberts
2023-10-07 22:54 ` Michael Ellerman
2023-10-10 0:20 ` Andrew Morton
2023-10-12 9:31 ` David Hildenbrand
2023-10-12 11:07 ` Michael Ellerman
2023-10-11 6:02 ` kernel test robot
2023-09-29 11:44 ` [PATCH v6 5/9] mm: thp: Extend THP to allocate anonymous large folios Ryan Roberts
[not found] ` <CGME20231005120507eucas1p13f50fa99f52808818840ee7db194e12e@eucas1p1.samsung.com>
2023-10-05 12:05 ` Daniel Gomez
2023-10-05 12:49 ` Ryan Roberts
2023-10-05 14:59 ` Daniel Gomez
2023-10-27 23:04 ` John Hubbard
2023-10-30 11:43 ` Ryan Roberts
2023-10-30 23:25 ` John Hubbard
2023-11-01 13:56 ` Ryan Roberts [this message]
2023-09-29 11:44 ` [PATCH v6 6/9] mm: thp: Add "recommend" option for anon_orders Ryan Roberts
2023-10-06 20:08 ` David Hildenbrand
2023-10-06 22:28 ` Yu Zhao
2023-10-09 11:45 ` Ryan Roberts
2023-10-09 14:43 ` David Hildenbrand
2023-10-09 20:04 ` Yu Zhao
2023-10-10 10:16 ` Ryan Roberts
2023-09-29 11:44 ` [PATCH v6 7/9] arm64/mm: Override arch_wants_pte_order() Ryan Roberts
2023-10-02 15:21 ` Catalin Marinas
2023-10-03 7:32 ` Ryan Roberts
2023-10-03 12:05 ` Catalin Marinas
2023-09-29 11:44 ` [PATCH v6 8/9] selftests/mm/cow: Generalize do_run_with_thp() helper Ryan Roberts
2023-09-29 11:44 ` [PATCH v6 9/9] selftests/mm/cow: Add tests for small-order anon THP Ryan Roberts
2023-10-06 20:06 ` [PATCH v6 0/9] variable-order, large folios for anonymous memory David Hildenbrand
2023-10-09 11:28 ` Ryan Roberts
2023-10-09 16:22 ` David Hildenbrand
2023-10-10 10:47 ` Ryan Roberts
2023-10-13 20:14 ` David Hildenbrand
2023-10-20 12:33 ` Ryan Roberts
2023-10-25 16:24 ` Ryan Roberts
2023-10-25 18:47 ` David Hildenbrand
2023-10-25 19:11 ` Yu Zhao
2023-10-26 9:53 ` Ryan Roberts
2023-10-26 15:19 ` David Hildenbrand
2023-10-25 19:10 ` John Hubbard
2023-10-31 11:50 ` Ryan Roberts
2023-10-31 11:55 ` Ryan Roberts
2023-10-31 12:03 ` David Hildenbrand
2023-10-31 13:13 ` Ryan Roberts
2023-10-31 18:29 ` Yang Shi
2023-11-01 14:02 ` Ryan Roberts
2023-11-01 18:11 ` Yang Shi
2023-10-31 11:58 ` David Hildenbrand
2023-10-31 13:12 ` Ryan Roberts
2023-11-13 3:57 ` John Hubbard
2023-11-13 5:18 ` Matthew Wilcox
2023-11-13 10:19 ` Ryan Roberts
2023-11-13 11:52 ` Kefeng Wang
2023-11-13 12:12 ` Ryan Roberts
2023-11-13 14:52 ` Kefeng Wang
2023-11-13 14:52 ` John Hubbard
2023-11-13 15:04 ` Matthew Wilcox
2023-11-14 10:57 ` Ryan Roberts
2023-12-05 16:05 ` Matthew Wilcox
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a6fa0847-a950-4044-972c-e5dc8cbc7922@arm.com \
--to=ryan.roberts@arm.com \
--cc=akpm@linux-foundation.org \
--cc=anshuman.khandual@arm.com \
--cc=catalin.marinas@arm.com \
--cc=david@redhat.com \
--cc=fengwei.yin@intel.com \
--cc=hughd@google.com \
--cc=itaru.kitayama@gmail.com \
--cc=jhubbard@nvidia.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mcgrof@kernel.org \
--cc=rientjes@google.com \
--cc=shy828301@gmail.com \
--cc=vbabka@suse.cz \
--cc=willy@infradead.org \
--cc=ying.huang@intel.com \
--cc=yuzhao@google.com \
--cc=ziy@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).