Instead, let's just allow for specifying through flags whether we want
to have bits merged into the original PTE.
For the madvise() case, simplify by having only a single parameter for
merging young+dirty. For madvise_cold_or_pageout_pte_range() merging the
dirty bit is not required, but also not harmful. This code is not that
performance critical after all to really force all micro-optimizations.
As we now have two pte_t * parameters, use PageTable() to make sure we
are actually given a pointer at a copy of the PTE, not a pointer into
an actual page table.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
mm/internal.h | 65 +++++++++++++++++++++++++++++++++------------------
mm/madvise.c | 26 ++++-----------------
mm/memory.c | 8 ++-----
mm/util.c | 2 +-
4 files changed, 50 insertions(+), 51 deletions(-)
diff --git a/mm/internal.h b/mm/internal.h
index 6c92956ac4fd9..b7131bd3d1ad1 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -208,6 +208,18 @@ typedef int __bitwise fpb_t;
/* Compare PTEs respecting the soft-dirty bit. */
#define FPB_RESPECT_SOFT_DIRTY ((__force fpb_t)BIT(1))
+/*
+ * Merge PTE write bits: if any PTE in the batch is writable, modify the
+ * PTE at @ptentp to be writable.
+ */
+#define FPB_MERGE_WRITE ((__force fpb_t)BIT(2))
+
+/*
+ * Merge PTE young and dirty bits: if any PTE in the batch is young or dirty,
+ * modify the PTE at @ptentp to be young or dirty, respectively.
+ */
+#define FPB_MERGE_YOUNG_DIRTY ((__force fpb_t)BIT(3))
+
static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
{
if (!(flags & FPB_RESPECT_DIRTY))
@@ -220,16 +232,12 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
/**
* folio_pte_batch_flags - detect a PTE batch for a large folio
* @folio: The large folio to detect a PTE batch for.
+ * @vma: The VMA. Only relevant with FPB_MERGE_WRITE, otherwise can be NULL.
* @ptep: Page table pointer for the first entry.
- * @pte: Page table entry for the first page.
+ * @ptentp: Pointer to a COPY of the first page table entry whose flags this
+ * function updates based on @flags if appropriate.
* @max_nr: The maximum number of table entries to consider.
* @flags: Flags to modify the PTE batch semantics.
- * @any_writable: Optional pointer to indicate whether any entry except the
- * first one is writable.
- * @any_young: Optional pointer to indicate whether any entry except the
- * first one is young.
- * @any_dirty: Optional pointer to indicate whether any entry except the
- * first one is dirty.
*
* Detect a PTE batch: consecutive (present) PTEs that map consecutive
* pages of the same large folio in a single VMA and a single page table.
@@ -242,28 +250,32 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
* must be limited by the caller so scanning cannot exceed a single VMA and
* a single page table.
*
+ * Depending on the FPB_MERGE_* flags, the pte stored at @ptentp will
+ * be updated: it's crucial that a pointer to a COPY of the first
+ * page table entry, obtained through ptep_get(), is provided as @ptentp.
+ *
* This function will be inlined to optimize based on the input parameters;
* consider using folio_pte_batch() instead if applicable.
*
* Return: the number of table entries in the batch.
*/
static inline unsigned int folio_pte_batch_flags(struct folio *folio,
- pte_t *ptep, pte_t pte, unsigned int max_nr, fpb_t flags,
- bool *any_writable, bool *any_young, bool *any_dirty)
+ struct vm_area_struct *vma, pte_t *ptep, pte_t *ptentp,
+ unsigned int max_nr, fpb_t flags)
{
+ bool any_writable = false, any_young = false, any_dirty = false;
+ pte_t expected_pte, pte = *ptentp;
unsigned int nr, cur_nr;
- pte_t expected_pte;
-
- if (any_writable)
- *any_writable = false;
- if (any_young)
- *any_young = false;
- if (any_dirty)
- *any_dirty = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
VM_WARN_ON_FOLIO(!folio_test_large(folio) || max_nr < 1, folio);
VM_WARN_ON_FOLIO(page_folio(pfn_to_page(pte_pfn(pte))) != folio, folio);
+ /*
+ * Ensure this is a pointer to a copy not a pointer into a page table.
+ * If this is a stack value, it won't be a valid virtual address, but
+ * that's fine because it also cannot be pointing into the page table.
+ */
+ VM_WARN_ON(virt_addr_valid(ptentp) && PageTable(virt_to_page(ptentp)));
/* Limit max_nr to the actual remaining PFNs in the folio we could batch. */
max_nr = min_t(unsigned long, max_nr,
@@ -279,12 +291,12 @@ static inline unsigned int folio_pte_batch_flags(struct folio *folio,
if (!pte_same(__pte_batch_clear_ignored(pte, flags), expected_pte))
break;
- if (any_writable)
- *any_writable |= pte_write(pte);
- if (any_young)
- *any_young |= pte_young(pte);
- if (any_dirty)
- *any_dirty |= pte_dirty(pte);
+ if (flags & FPB_MERGE_WRITE)
+ any_writable |= pte_write(pte);
+ if (flags & FPB_MERGE_YOUNG_DIRTY) {
+ any_young |= pte_young(pte);
+ any_dirty |= pte_dirty(pte);
+ }
cur_nr = pte_batch_hint(ptep, pte);
expected_pte = pte_advance_pfn(expected_pte, cur_nr);
@@ -292,6 +304,13 @@ static inline unsigned int folio_pte_batch_flags(struct folio *folio,
nr += cur_nr;
}
+ if (any_writable)
+ *ptentp = pte_mkwrite(*ptentp, vma);
+ if (any_young)
+ *ptentp = pte_mkyoung(*ptentp);
+ if (any_dirty)
+ *ptentp = pte_mkdirty(*ptentp);
+
return min(nr, max_nr);
}
diff --git a/mm/madvise.c b/mm/madvise.c
index fe363a14daab3..9de9b7c797c63 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -344,13 +344,12 @@ static inline bool can_do_file_pageout(struct vm_area_struct *vma)
static inline int madvise_folio_pte_batch(unsigned long addr, unsigned long end,
struct folio *folio, pte_t *ptep,
- pte_t pte, bool *any_young,
- bool *any_dirty)
+ pte_t *ptentp)
{
int max_nr = (end - addr) / PAGE_SIZE;
- return folio_pte_batch_flags(folio, ptep, pte, max_nr, 0, NULL,
- any_young, any_dirty);
+ return folio_pte_batch_flags(folio, NULL, ptep, ptentp, max_nr,
+ FPB_MERGE_YOUNG_DIRTY);
}
static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
@@ -488,13 +487,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
* next pte in the range.
*/
if (folio_test_large(folio)) {
- bool any_young;
-
- nr = madvise_folio_pte_batch(addr, end, folio, pte,
- ptent, &any_young, NULL);
- if (any_young)
- ptent = pte_mkyoung(ptent);
-
+ nr = madvise_folio_pte_batch(addr, end, folio, pte, &ptent);
if (nr < folio_nr_pages(folio)) {
int err;
@@ -724,11 +717,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
* next pte in the range.
*/
if (folio_test_large(folio)) {
- bool any_young, any_dirty;
-
- nr = madvise_folio_pte_batch(addr, end, folio, pte,
- ptent, &any_young, &any_dirty);
-
+ nr = madvise_folio_pte_batch(addr, end, folio, pte, &ptent);
if (nr < folio_nr_pages(folio)) {
int err;
@@ -753,11 +742,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
nr = 0;
continue;
}
-
- if (any_young)
- ptent = pte_mkyoung(ptent);
- if (any_dirty)
- ptent = pte_mkdirty(ptent);
}
if (folio_test_swapcache(folio) || folio_test_dirty(folio)) {
diff --git a/mm/memory.c b/mm/memory.c
index 26a82b82863b0..0269edb520987 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -972,10 +972,9 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
int max_nr, int *rss, struct folio **prealloc)
{
+ fpb_t flags = FPB_MERGE_WRITE;
struct page *page;
struct folio *folio;
- bool any_writable;
- fpb_t flags = 0;
int err, nr;
page = vm_normal_page(src_vma, addr, pte);
@@ -995,8 +994,7 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
if (vma_soft_dirty_enabled(src_vma))
flags |= FPB_RESPECT_SOFT_DIRTY;
- nr = folio_pte_batch_flags(folio, src_pte, pte, max_nr, flags,
- &any_writable, NULL, NULL);
+ nr = folio_pte_batch_flags(folio, src_vma, src_pte, &pte, max_nr, flags);
folio_ref_add(folio, nr);
if (folio_test_anon(folio)) {
if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
@@ -1010,8 +1008,6 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
folio_dup_file_rmap_ptes(folio, page, nr, dst_vma);
rss[mm_counter_file(folio)] += nr;
}
- if (any_writable)
- pte = pte_mkwrite(pte, src_vma);
__copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte, pte,
addr, nr);
return nr;
diff --git a/mm/util.c b/mm/util.c
index cf41edceec7d2..ce826ca82a11d 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -1197,6 +1197,6 @@ EXPORT_SYMBOL(compat_vma_mmap_prepare);
unsigned int folio_pte_batch(struct folio *folio, pte_t *ptep, pte_t pte,
unsigned int max_nr)
{
- return folio_pte_batch_flags(folio, ptep, pte, max_nr, 0, NULL, NULL, NULL);
+ return folio_pte_batch_flags(folio, NULL, ptep, &pte, max_nr, 0);
}
#endif /* CONFIG_MMU */
--
2.49.0Nit in subject. We have renamed the function to folio_pte_batch_flags(). Not sure it is too late. -- Wei Yang Help you, Help me
On 04.08.25 10:22, Wei Yang wrote: > Nit in subject. > > We have renamed the function to folio_pte_batch_flags(). Right. > > Not sure it is too late. Already upsteam. -- Cheers, David / dhildenb
On 02/07/25 4:19 pm, David Hildenbrand wrote: > Instead, let's just allow for specifying through flags whether we want > to have bits merged into the original PTE. > > For the madvise() case, simplify by having only a single parameter for > merging young+dirty. For madvise_cold_or_pageout_pte_range() merging the > dirty bit is not required, but also not harmful. This code is not that > performance critical after all to really force all micro-optimizations. Makes sense. > > As we now have two pte_t * parameters, use PageTable() to make sure we > are actually given a pointer at a copy of the PTE, not a pointer into > an actual page table. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > Reviewed-by: Dev Jain <dev.jain@arm.com>
On Wed, Jul 02, 2025 at 12:49:26PM +0200, David Hildenbrand wrote: > Instead, let's just allow for specifying through flags whether we want > to have bits merged into the original PTE. > > For the madvise() case, simplify by having only a single parameter for > merging young+dirty. For madvise_cold_or_pageout_pte_range() merging the > dirty bit is not required, but also not harmful. This code is not that > performance critical after all to really force all micro-optimizations. > > As we now have two pte_t * parameters, use PageTable() to make sure we > are actually given a pointer at a copy of the PTE, not a pointer into > an actual page table. > > Signed-off-by: David Hildenbrand <david@redhat.com> Although I think it'd be nice to have a comment pointing out why dirty-young bites go together: Reviewed-by: Oscar Salvador <osalvador@suse.de> -- Oscar Salvador SUSE Labs
On Wed, Jul 02, 2025 at 12:49:26PM +0200, David Hildenbrand wrote: > Instead, let's just allow for specifying through flags whether we want > to have bits merged into the original PTE. > > For the madvise() case, simplify by having only a single parameter for > merging young+dirty. For madvise_cold_or_pageout_pte_range() merging the > dirty bit is not required, but also not harmful. This code is not that > performance critical after all to really force all micro-optimizations. Seems a bit odd to me to have the dirty-young bit being treat as "one". You mention that this is done because the only user of it doesn't really care about dirty vs non-dirty and it's not harmful eitherway, so micro-optimizing this isn't worth at this moment. But what if we grop another user which wants to make this distinction and where it matters dirty vs non-dirty/young vs not-young. Wouldn't be better to have it separated from the start? And I'm not talking about micro-optimizing, because that's clear that it doesn't matter, but for clarity purposes. It seems a lot more organic/natural/obvious to me. -- Oscar Salvador SUSE Labs
On 02.07.25 16:00, Oscar Salvador wrote: > On Wed, Jul 02, 2025 at 12:49:26PM +0200, David Hildenbrand wrote: >> Instead, let's just allow for specifying through flags whether we want >> to have bits merged into the original PTE. >> >> For the madvise() case, simplify by having only a single parameter for >> merging young+dirty. For madvise_cold_or_pageout_pte_range() merging the >> dirty bit is not required, but also not harmful. This code is not that >> performance critical after all to really force all micro-optimizations. > > Seems a bit odd to me to have the dirty-young bit being treat as "one". > You mention that this is done because the only user of it doesn't really > care about dirty vs non-dirty and it's not harmful eitherway, so micro-optimizing > this isn't worth at this moment. > > But what if we grop another user which wants to make this distinction and > where it matters dirty vs non-dirty/young vs not-young. > Wouldn't be better to have it separated from the start? I mean, it's easy to add later if ever required. :) But more importantly, usually young+dirty is a moving target as HW can usually update it asynchronously. So in the common case, you really have to rely on the young+dirty bits from get_and_clear_full_ptes(), and not on the current snapshot while the page remains mapped. The madvise() use case is rather special in that sense. -- Cheers, David / dhildenb
On Wed, Jul 02, 2025 at 04:40:30PM +0200, David Hildenbrand wrote: > But more importantly, usually young+dirty is a moving target as HW can > usually update it asynchronously. Ok, you mean they often get queried both at once? > So in the common case, you really have to rely on the young+dirty bits from > get_and_clear_full_ptes(), and not on the current snapshot while the page > remains mapped. > > The madvise() use case is rather special in that sense. I see. I mean, codewise this looks like an improvement, I was just puzzled by the fact that we're dealing with young+dirty together (while we didn't before), but given that get_and_clear_full_ptes() do that already, I guess it makes sense if that's the way we usually operate. I wasn't familiar with that, thinking about it makes sense, but I wonder whether we could place a comment either in the definition of FPB_MERGE_YOUNG_DIRTY, or in the handling of it in folio_pte_batch_flags(). I guess in the definition would make more sense. Whether you decide to move forward on the comment or not (could be squashed), I'm ok with this. -- Oscar Salvador SUSE Labs
© 2016 - 2025 Red Hat, Inc.