[RFC,00/39] mm/rmap: interface overhaul

Message

David Hildenbrand Dec. 4, 2023, 2:21 p.m. UTC

Baed on mm-stable from a couple of days.

This series proposes an overhaul to our rmap interface, to get rid of the
"bool compound" / RMAP_COMPOUND parameter with the goal of making the
interface less error prone, more future proof, and more natural to extend
to "batching". Also, this converts the interface to always consume
folio+subpage, which speeds up operations on large folios.

Further, this series adds PTE-batching variants for 4 rmap functions,
whereby only folio_add_anon_rmap_ptes() is used for batching in this series
when PTE-remapping a PMD-mapped THP.

Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
-- he carries his own batching variant right now -- and
folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].

There is some overlap with both series (and some other work, like
multi-size THP [3]), so that will need some coordination, and likely a
stepwise inclusion.

I got that started [4], but it made sense to show the whole picture. The
patches of [4] are contained in here, with one additional patch added
("mm/rmap: introduce and use hugetlb_try_share_anon_rmap()") and some
slight patch description changes.

In general, RMAP batching is an important optimization for PTE-mapped
THP, especially once we want to move towards a total mapcount or further,
as shown with my WIP patches on "mapped shared vs. mapped exclusively" [5].

The rmap batching part of [5] is also contained here in a slightly reworked
fork [and I found a bug du to the "compound" parameter handling in these
patches that should be fixed here :) ].

This series performs a lot of folio conversion, that could be separated
if there is a good reason. Most of the added LOC in the diff are only due
to documentation.

As we're moving to a pte/pmd interface where we clearly express the
mapping granularity we are dealing with, we first get the remainder of
hugetlb out of the way, as it is special and expected to remain special: it
treats everything as a "single logical PTE" and only currently allows
entire mappings.

Even if we'd ever support partial mappings, I strongly
assume the interface and implementation will still differ heavily:
hopefull we can avoid working on subpages/subpage mapcounts completely and
only add a "count" parameter for them to enable batching.


New (extended) hugetlb interface that operate on entire folio:
 * hugetlb_add_new_anon_rmap() -> Already existed
 * hugetlb_add_anon_rmap() -> Already existed
 * hugetlb_try_dup_anon_rmap()
 * hugetlb_try_share_anon_rmap()
 * hugetlb_add_file_rmap()
 * hugetlb_remove_rmap()

New "ordinary" interface for small folios / THP::
 * folio_add_new_anon_rmap() -> Already existed
 * folio_add_anon_rmap_[pte|ptes|pmd]()
 * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
 * folio_try_share_anon_rmap_[pte|pmd]()
 * folio_add_file_rmap_[pte|ptes|pmd]()
 * folio_dup_file_rmap_[pte|ptes|pmd]()
 * folio_remove_rmap_[pte|ptes|pmd]()

folio_add_new_anon_rmap() will always map at the biggest granularity
possible (currently, a single PMD to cover a PMD-sized THP). Could be
extended if ever required.

In the future, we might want "_pud" variants and eventually "_pmds" variants
for batching. Further, if hugepd is ever a thing outside hugetlb code,
we might want some variants for that. All stuff for the distant future.


I ran some simple microbenchmarks from [5] on an Intel(R) Xeon(R) Silver
4210R: munmap(), fork(), cow, MADV_DONTNEED on each PTE ... and PTE
remapping PMD-mapped THPs on 1 GiB of memory.

For small folios, there is barely a change (< 1 % performance improvement),
whereby fork() still stands out with 0.74% performance improvement, but
it might be just some noise. Folio optimizations don't help that much
with small folios.

For PTE-mapped THP:
* PTE-remapping a PMD-mapped THP is more than 10% faster.
  -> RMAP batching
* fork() is more than 4% faster.
  -> folio conversion
* MADV_DONTNEED is 2% faster
  -> folio conversion
* COW by writing only a single byte on a COW-shared PTE
  -> folio conversion
* munmap() is only slightly faster (< 1%).

[1] https://lkml.kernel.org/r/[e-mail]
[2] https://lkml.kernel.org/r/[e-mail]
[3] https://lkml.kernel.org/r/[e-mail]
[4] https://lkml.kernel.org/r/[e-mail]
[5] https://lkml.kernel.org/r/[e-mail]

Cc: Andrew Morton <[e-mail]>
Cc: "Matthew Wilcox (Oracle)" <[e-mail]>
Cc: Hugh Dickins <[e-mail]>
Cc: Ryan Roberts <[e-mail]>
Cc: Yin Fengwei <[e-mail]>
Cc: Mike Kravetz <[e-mail]>
Cc: Muchun Song <[e-mail]>
Cc: Peter Xu <[e-mail]>

David Hildenbrand (39):
  mm/rmap: rename hugepage_add* to hugetlb_add*
  mm/rmap: introduce and use hugetlb_remove_rmap()
  mm/rmap: introduce and use hugetlb_add_file_rmap()
  mm/rmap: introduce and use hugetlb_try_dup_anon_rmap()
  mm/rmap: introduce and use hugetlb_try_share_anon_rmap()
  mm/rmap: add hugetlb sanity checks
  mm/rmap: convert folio_add_file_rmap_range() into
    folio_add_file_rmap_[pte|ptes|pmd]()
  mm/memory: page_add_file_rmap() -> folio_add_file_rmap_[pte|pmd]()
  mm/huge_memory: page_add_file_rmap() -> folio_add_file_rmap_pmd()
  mm/migrate: page_add_file_rmap() -> folio_add_file_rmap_pte()
  mm/userfaultfd: page_add_file_rmap() -> folio_add_file_rmap_pte()
  mm/rmap: remove page_add_file_rmap()
  mm/rmap: factor out adding folio mappings into __folio_add_rmap()
  mm/rmap: introduce folio_add_anon_rmap_[pte|ptes|pmd]()
  mm/huge_memory: batch rmap operations in __split_huge_pmd_locked()
  mm/huge_memory: page_add_anon_rmap() -> folio_add_anon_rmap_pmd()
  mm/migrate: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
  mm/ksm: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
  mm/swapfile: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
  mm/memory: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
  mm/rmap: remove page_add_anon_rmap()
  mm/rmap: remove RMAP_COMPOUND
  mm/rmap: introduce folio_remove_rmap_[pte|ptes|pmd]()
  kernel/events/uprobes: page_remove_rmap() -> folio_remove_rmap_pte()
  mm/huge_memory: page_remove_rmap() -> folio_remove_rmap_pmd()
  mm/khugepaged: page_remove_rmap() -> folio_remove_rmap_pte()
  mm/ksm: page_remove_rmap() -> folio_remove_rmap_pte()
  mm/memory: page_remove_rmap() -> folio_remove_rmap_pte()
  mm/migrate_device: page_remove_rmap() -> folio_remove_rmap_pte()
  mm/rmap: page_remove_rmap() -> folio_remove_rmap_pte()
  Documentation: stop referring to page_remove_rmap()
  mm/rmap: remove page_remove_rmap()
  mm/rmap: convert page_dup_file_rmap() to
    folio_dup_file_rmap_[pte|ptes|pmd]()
  mm/rmap: introduce folio_try_dup_anon_rmap_[pte|ptes|pmd]()
  mm/huge_memory: page_try_dup_anon_rmap() ->
    folio_try_dup_anon_rmap_pmd()
  mm/memory: page_try_dup_anon_rmap() -> folio_try_dup_anon_rmap_pte()
  mm/rmap: remove page_try_dup_anon_rmap()
  mm: convert page_try_share_anon_rmap() to
    folio_try_share_anon_rmap_[pte|pmd]()
  mm/rmap: rename COMPOUND_MAPPED to ENTIRELY_MAPPED

 Documentation/mm/transhuge.rst       |   4 +-
 Documentation/mm/unevictable-lru.rst |   4 +-
 include/linux/mm.h                   |   6 +-
 include/linux/rmap.h                 | 380 +++++++++++++++++++-----
 kernel/events/uprobes.c              |   2 +-
 mm/gup.c                             |   2 +-
 mm/huge_memory.c                     |  85 +++---
 mm/hugetlb.c                         |  21 +-
 mm/internal.h                        |  12 +-
 mm/khugepaged.c                      |  17 +-
 mm/ksm.c                             |  15 +-
 mm/memory-failure.c                  |   4 +-
 mm/memory.c                          |  60 ++--
 mm/migrate.c                         |  12 +-
 mm/migrate_device.c                  |  41 +--
 mm/mmu_gather.c                      |   2 +-
 mm/rmap.c                            | 422 ++++++++++++++++-----------
 mm/swapfile.c                        |   2 +-
 mm/userfaultfd.c                     |   2 +-
 19 files changed, 709 insertions(+), 384 deletions(-)

Comments

Ryan Roberts Dec. 4, 2023, 7:53 p.m. UTC | #1

On 04/12/2023 14:21, David Hildenbrand wrote:
> Baed on mm-stable from a couple of days.
> 
> This series proposes an overhaul to our rmap interface, to get rid of the
> "bool compound" / RMAP_COMPOUND parameter with the goal of making the
> interface less error prone, more future proof, and more natural to extend
> to "batching". Also, this converts the interface to always consume
> folio+subpage, which speeds up operations on large folios.
> 
> Further, this series adds PTE-batching variants for 4 rmap functions,
> whereby only folio_add_anon_rmap_ptes() is used for batching in this series
> when PTE-remapping a PMD-mapped THP.

I certainly support the objective you have here; making the interfaces clearer,
more consistent and more amenable to batching. I'll try to find some time this
week to review.

> 
> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
> -- he carries his own batching variant right now -- and
> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].

Note that the contpte series at [2] has a new patch in v3 (patch 2), which could
benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
on top of [2] once it is merged.

> 
> There is some overlap with both series (and some other work, like
> multi-size THP [3]), so that will need some coordination, and likely a
> stepwise inclusion.

Selfishly, I'd really like to get my stuff merged as soon as there is no
technical reason not to. I'd prefer not to add this as a dependency if we can
help it.

> 
> I got that started [4], but it made sense to show the whole picture. The
> patches of [4] are contained in here, with one additional patch added
> ("mm/rmap: introduce and use hugetlb_try_share_anon_rmap()") and some
> slight patch description changes.
> 
> In general, RMAP batching is an important optimization for PTE-mapped
> THP, especially once we want to move towards a total mapcount or further,
> as shown with my WIP patches on "mapped shared vs. mapped exclusively" [5].
> 
> The rmap batching part of [5] is also contained here in a slightly reworked
> fork [and I found a bug du to the "compound" parameter handling in these
> patches that should be fixed here :) ].
> 
> This series performs a lot of folio conversion, that could be separated
> if there is a good reason. Most of the added LOC in the diff are only due
> to documentation.
> 
> As we're moving to a pte/pmd interface where we clearly express the
> mapping granularity we are dealing with, we first get the remainder of
> hugetlb out of the way, as it is special and expected to remain special: it
> treats everything as a "single logical PTE" and only currently allows
> entire mappings.
> 
> Even if we'd ever support partial mappings, I strongly
> assume the interface and implementation will still differ heavily:
> hopefull we can avoid working on subpages/subpage mapcounts completely and
> only add a "count" parameter for them to enable batching.
> 
> 
> New (extended) hugetlb interface that operate on entire folio:
>  * hugetlb_add_new_anon_rmap() -> Already existed
>  * hugetlb_add_anon_rmap() -> Already existed
>  * hugetlb_try_dup_anon_rmap()
>  * hugetlb_try_share_anon_rmap()
>  * hugetlb_add_file_rmap()
>  * hugetlb_remove_rmap()
> 
> New "ordinary" interface for small folios / THP::
>  * folio_add_new_anon_rmap() -> Already existed
>  * folio_add_anon_rmap_[pte|ptes|pmd]()
>  * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>  * folio_try_share_anon_rmap_[pte|pmd]()
>  * folio_add_file_rmap_[pte|ptes|pmd]()
>  * folio_dup_file_rmap_[pte|ptes|pmd]()
>  * folio_remove_rmap_[pte|ptes|pmd]()

I'm not sure if there are official guidelines, but personally if we are
reworking the API, I'd take the opportunity to move "rmap" to the front of the
name, rather than having it burried in the middle as it is for some of these:

rmap_hugetlb_*()

rmap_folio_*()

I guess reading the patches will tell me, but what's the point of "ptes"? Surely
you're either mapping at pte or pmd level, and the number of pages is determined
by the folio size? (or presumably nr param passed in)

Thanks,
Ryan

> 
> folio_add_new_anon_rmap() will always map at the biggest granularity
> possible (currently, a single PMD to cover a PMD-sized THP). Could be
> extended if ever required.
> 
> In the future, we might want "_pud" variants and eventually "_pmds" variants
> for batching. Further, if hugepd is ever a thing outside hugetlb code,
> we might want some variants for that. All stuff for the distant future.
> 
> 
> I ran some simple microbenchmarks from [5] on an Intel(R) Xeon(R) Silver
> 4210R: munmap(), fork(), cow, MADV_DONTNEED on each PTE ... and PTE
> remapping PMD-mapped THPs on 1 GiB of memory.
> 
> For small folios, there is barely a change (< 1 % performance improvement),
> whereby fork() still stands out with 0.74% performance improvement, but
> it might be just some noise. Folio optimizations don't help that much
> with small folios.
> 
> For PTE-mapped THP:
> * PTE-remapping a PMD-mapped THP is more than 10% faster.
>   -> RMAP batching
> * fork() is more than 4% faster.
>   -> folio conversion
> * MADV_DONTNEED is 2% faster
>   -> folio conversion
> * COW by writing only a single byte on a COW-shared PTE
>   -> folio conversion
> * munmap() is only slightly faster (< 1%).
> 
> [1] https://lkml.kernel.org/r/[e-mail]
> [2] https://lkml.kernel.org/r/[e-mail]
> [3] https://lkml.kernel.org/r/[e-mail]
> [4] https://lkml.kernel.org/r/[e-mail]
> [5] https://lkml.kernel.org/r/[e-mail]
> 
> Cc: Andrew Morton <[e-mail]>
> Cc: "Matthew Wilcox (Oracle)" <[e-mail]>
> Cc: Hugh Dickins <[e-mail]>
> Cc: Ryan Roberts <[e-mail]>
> Cc: Yin Fengwei <[e-mail]>
> Cc: Mike Kravetz <[e-mail]>
> Cc: Muchun Song <[e-mail]>
> Cc: Peter Xu <[e-mail]>
> 
> David Hildenbrand (39):
>   mm/rmap: rename hugepage_add* to hugetlb_add*
>   mm/rmap: introduce and use hugetlb_remove_rmap()
>   mm/rmap: introduce and use hugetlb_add_file_rmap()
>   mm/rmap: introduce and use hugetlb_try_dup_anon_rmap()
>   mm/rmap: introduce and use hugetlb_try_share_anon_rmap()
>   mm/rmap: add hugetlb sanity checks
>   mm/rmap: convert folio_add_file_rmap_range() into
>     folio_add_file_rmap_[pte|ptes|pmd]()
>   mm/memory: page_add_file_rmap() -> folio_add_file_rmap_[pte|pmd]()
>   mm/huge_memory: page_add_file_rmap() -> folio_add_file_rmap_pmd()
>   mm/migrate: page_add_file_rmap() -> folio_add_file_rmap_pte()
>   mm/userfaultfd: page_add_file_rmap() -> folio_add_file_rmap_pte()
>   mm/rmap: remove page_add_file_rmap()
>   mm/rmap: factor out adding folio mappings into __folio_add_rmap()
>   mm/rmap: introduce folio_add_anon_rmap_[pte|ptes|pmd]()
>   mm/huge_memory: batch rmap operations in __split_huge_pmd_locked()
>   mm/huge_memory: page_add_anon_rmap() -> folio_add_anon_rmap_pmd()
>   mm/migrate: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>   mm/ksm: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>   mm/swapfile: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>   mm/memory: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>   mm/rmap: remove page_add_anon_rmap()
>   mm/rmap: remove RMAP_COMPOUND
>   mm/rmap: introduce folio_remove_rmap_[pte|ptes|pmd]()
>   kernel/events/uprobes: page_remove_rmap() -> folio_remove_rmap_pte()
>   mm/huge_memory: page_remove_rmap() -> folio_remove_rmap_pmd()
>   mm/khugepaged: page_remove_rmap() -> folio_remove_rmap_pte()
>   mm/ksm: page_remove_rmap() -> folio_remove_rmap_pte()
>   mm/memory: page_remove_rmap() -> folio_remove_rmap_pte()
>   mm/migrate_device: page_remove_rmap() -> folio_remove_rmap_pte()
>   mm/rmap: page_remove_rmap() -> folio_remove_rmap_pte()
>   Documentation: stop referring to page_remove_rmap()
>   mm/rmap: remove page_remove_rmap()
>   mm/rmap: convert page_dup_file_rmap() to
>     folio_dup_file_rmap_[pte|ptes|pmd]()
>   mm/rmap: introduce folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>   mm/huge_memory: page_try_dup_anon_rmap() ->
>     folio_try_dup_anon_rmap_pmd()
>   mm/memory: page_try_dup_anon_rmap() -> folio_try_dup_anon_rmap_pte()
>   mm/rmap: remove page_try_dup_anon_rmap()
>   mm: convert page_try_share_anon_rmap() to
>     folio_try_share_anon_rmap_[pte|pmd]()
>   mm/rmap: rename COMPOUND_MAPPED to ENTIRELY_MAPPED
> 
>  Documentation/mm/transhuge.rst       |   4 +-
>  Documentation/mm/unevictable-lru.rst |   4 +-
>  include/linux/mm.h                   |   6 +-
>  include/linux/rmap.h                 | 380 +++++++++++++++++++-----
>  kernel/events/uprobes.c              |   2 +-
>  mm/gup.c                             |   2 +-
>  mm/huge_memory.c                     |  85 +++---
>  mm/hugetlb.c                         |  21 +-
>  mm/internal.h                        |  12 +-
>  mm/khugepaged.c                      |  17 +-
>  mm/ksm.c                             |  15 +-
>  mm/memory-failure.c                  |   4 +-
>  mm/memory.c                          |  60 ++--
>  mm/migrate.c                         |  12 +-
>  mm/migrate_device.c                  |  41 +--
>  mm/mmu_gather.c                      |   2 +-
>  mm/rmap.c                            | 422 ++++++++++++++++-----------
>  mm/swapfile.c                        |   2 +-
>  mm/userfaultfd.c                     |   2 +-
>  19 files changed, 709 insertions(+), 384 deletions(-)
>

David Hildenbrand Dec. 5, 2023, 9:56 a.m. UTC | #2

>>
>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>> -- he carries his own batching variant right now -- and
>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
> 
> Note that the contpte series at [2] has a new patch in v3 (patch 2), which could
> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
> on top of [2] once it is merged.
> 
>>
>> There is some overlap with both series (and some other work, like
>> multi-size THP [3]), so that will need some coordination, and likely a
>> stepwise inclusion.
> 
> Selfishly, I'd really like to get my stuff merged as soon as there is no
> technical reason not to. I'd prefer not to add this as a dependency if we can
> help it.

It's easy to rework either series on top of each other. The mTHP series has highest priority,
no question, that will go in first.

Regarding the contpte, I think it needs more work. Especially, as raised, to not degrade
order-0 performance. Maybe we won't make the next merge window (and you already predicated
that in some cover letter :P ). Let's see.

But again, the conflicts are all trivial, so I'll happily rebase on top of whatever is
in mm-unstable. Or move the relevant rework to the front so you can just carry
them/base on them. (the batched variants for dup do make the contpte code much easier)

[...]

>>
>>
>> New (extended) hugetlb interface that operate on entire folio:
>>   * hugetlb_add_new_anon_rmap() -> Already existed
>>   * hugetlb_add_anon_rmap() -> Already existed
>>   * hugetlb_try_dup_anon_rmap()
>>   * hugetlb_try_share_anon_rmap()
>>   * hugetlb_add_file_rmap()
>>   * hugetlb_remove_rmap()
>>
>> New "ordinary" interface for small folios / THP::
>>   * folio_add_new_anon_rmap() -> Already existed
>>   * folio_add_anon_rmap_[pte|ptes|pmd]()
>>   * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>   * folio_try_share_anon_rmap_[pte|pmd]()
>>   * folio_add_file_rmap_[pte|ptes|pmd]()
>>   * folio_dup_file_rmap_[pte|ptes|pmd]()
>>   * folio_remove_rmap_[pte|ptes|pmd]()
> 
> I'm not sure if there are official guidelines, but personally if we are
> reworking the API, I'd take the opportunity to move "rmap" to the front of the
> name, rather than having it burried in the middle as it is for some of these:
> 
> rmap_hugetlb_*()
> 
> rmap_folio_*()

No strong opinion. But we might want slightly different names then. For example,
it's "bio_add_folio" and not "bio_folio_add":


rmap_add_new_anon_hugetlb()
rmap_add_anon_hugetlb()
...
rmap_remove_hugetlb()


rmap_add_new_anon_folio()
rmap_add_anon_folio_[pte|ptes|pmd]()
...
rmap_dup_file_folio_[pte|ptes|pmd]()
rmap_remove_folio_[pte|ptes|pmd]()

Thoughts?

> 
> I guess reading the patches will tell me, but what's the point of "ptes"? Surely
> you're either mapping at pte or pmd level, and the number of pages is determined
> by the folio size? (or presumably nr param passed in)

It's really (currently) one function to handle 1 vs. multiple PTEs. For example:

void folio_remove_rmap_ptes(struct folio *, struct page *, unsigned int nr,
		struct vm_area_struct *);
#define folio_remove_rmap_pte(folio, page, vma) \
	folio_remove_rmap_ptes(folio, page, 1, vma)
void folio_remove_rmap_pmd(struct folio *, struct page *,
		struct vm_area_struct *);


Once could let the compiler generate specialized variants for the single-pte
versions to make the order-0 case faster. For now it's just a helper macro.

Ryan Roberts Dec. 5, 2023, 12:29 p.m. UTC | #3

On 04/12/2023 19:53, Ryan Roberts wrote:
> On 04/12/2023 14:21, David Hildenbrand wrote:
>> Baed on mm-stable from a couple of days.
>>
>> This series proposes an overhaul to our rmap interface, to get rid of the
>> "bool compound" / RMAP_COMPOUND parameter with the goal of making the
>> interface less error prone, more future proof, and more natural to extend
>> to "batching". Also, this converts the interface to always consume
>> folio+subpage, which speeds up operations on large folios.
>>
>> Further, this series adds PTE-batching variants for 4 rmap functions,
>> whereby only folio_add_anon_rmap_ptes() is used for batching in this series
>> when PTE-remapping a PMD-mapped THP.
> 
> I certainly support the objective you have here; making the interfaces clearer,
> more consistent and more amenable to batching. I'll try to find some time this
> week to review.
> 
>>
>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>> -- he carries his own batching variant right now -- and
>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
> 
> Note that the contpte series at [2] has a new patch in v3 (patch 2), which could
> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
> on top of [2] once it is merged.
> 
>>
>> There is some overlap with both series (and some other work, like
>> multi-size THP [3]), so that will need some coordination, and likely a
>> stepwise inclusion.
> 
> Selfishly, I'd really like to get my stuff merged as soon as there is no
> technical reason not to. I'd prefer not to add this as a dependency if we can
> help it.
> 
>>
>> I got that started [4], but it made sense to show the whole picture. The
>> patches of [4] are contained in here, with one additional patch added
>> ("mm/rmap: introduce and use hugetlb_try_share_anon_rmap()") and some
>> slight patch description changes.
>>
>> In general, RMAP batching is an important optimization for PTE-mapped
>> THP, especially once we want to move towards a total mapcount or further,
>> as shown with my WIP patches on "mapped shared vs. mapped exclusively" [5].
>>
>> The rmap batching part of [5] is also contained here in a slightly reworked
>> fork [and I found a bug du to the "compound" parameter handling in these
>> patches that should be fixed here :) ].
>>
>> This series performs a lot of folio conversion, that could be separated
>> if there is a good reason. Most of the added LOC in the diff are only due
>> to documentation.
>>
>> As we're moving to a pte/pmd interface where we clearly express the
>> mapping granularity we are dealing with, we first get the remainder of
>> hugetlb out of the way, as it is special and expected to remain special: it
>> treats everything as a "single logical PTE" and only currently allows
>> entire mappings.
>>
>> Even if we'd ever support partial mappings, I strongly
>> assume the interface and implementation will still differ heavily:
>> hopefull we can avoid working on subpages/subpage mapcounts completely and
>> only add a "count" parameter for them to enable batching.
>>
>>
>> New (extended) hugetlb interface that operate on entire folio:
>>  * hugetlb_add_new_anon_rmap() -> Already existed
>>  * hugetlb_add_anon_rmap() -> Already existed
>>  * hugetlb_try_dup_anon_rmap()
>>  * hugetlb_try_share_anon_rmap()
>>  * hugetlb_add_file_rmap()
>>  * hugetlb_remove_rmap()
>>
>> New "ordinary" interface for small folios / THP::
>>  * folio_add_new_anon_rmap() -> Already existed
>>  * folio_add_anon_rmap_[pte|ptes|pmd]()
>>  * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>  * folio_try_share_anon_rmap_[pte|pmd]()
>>  * folio_add_file_rmap_[pte|ptes|pmd]()
>>  * folio_dup_file_rmap_[pte|ptes|pmd]()
>>  * folio_remove_rmap_[pte|ptes|pmd]()
> 
> I'm not sure if there are official guidelines, but personally if we are
> reworking the API, I'd take the opportunity to move "rmap" to the front of the
> name, rather than having it burried in the middle as it is for some of these:
> 
> rmap_hugetlb_*()
> 
> rmap_folio_*()

In fact, I'd be inclined to drop the "folio" to shorten the name; everything is
a folio, so its not telling us much. e.g.:

New (extended) hugetlb interface that operate on entire folio:
 * rmap_hugetlb_add_new_anon() -> Already existed
 * rmap_hugetlb_add_anon() -> Already existed
 * rmap_hugetlb_try_dup_anon()
 * rmap_hugetlb_try_share_anon()
 * rmap_hugetlb_add_file()
 * rmap_hugetlb_remove()

New "ordinary" interface for small folios / THP::
 * rmap_add_new_anon() -> Already existed
 * rmap_add_anon_[pte|ptes|pmd]()
 * rmap_try_dup_anon_[pte|ptes|pmd]()
 * rmap_try_share_anon_[pte|pmd]()
 * rmap_add_file_[pte|ptes|pmd]()
 * rmap_dup_file_[pte|ptes|pmd]()
 * rmap_remove_[pte|ptes|pmd]()


> 
> I guess reading the patches will tell me, but what's the point of "ptes"? Surely
> you're either mapping at pte or pmd level, and the number of pages is determined
> by the folio size? (or presumably nr param passed in)
> 
> Thanks,
> Ryan
> 
>>
>> folio_add_new_anon_rmap() will always map at the biggest granularity
>> possible (currently, a single PMD to cover a PMD-sized THP). Could be
>> extended if ever required.
>>
>> In the future, we might want "_pud" variants and eventually "_pmds" variants
>> for batching. Further, if hugepd is ever a thing outside hugetlb code,
>> we might want some variants for that. All stuff for the distant future.
>>
>>
>> I ran some simple microbenchmarks from [5] on an Intel(R) Xeon(R) Silver
>> 4210R: munmap(), fork(), cow, MADV_DONTNEED on each PTE ... and PTE
>> remapping PMD-mapped THPs on 1 GiB of memory.
>>
>> For small folios, there is barely a change (< 1 % performance improvement),
>> whereby fork() still stands out with 0.74% performance improvement, but
>> it might be just some noise. Folio optimizations don't help that much
>> with small folios.
>>
>> For PTE-mapped THP:
>> * PTE-remapping a PMD-mapped THP is more than 10% faster.
>>   -> RMAP batching
>> * fork() is more than 4% faster.
>>   -> folio conversion
>> * MADV_DONTNEED is 2% faster
>>   -> folio conversion
>> * COW by writing only a single byte on a COW-shared PTE
>>   -> folio conversion
>> * munmap() is only slightly faster (< 1%).
>>
>> [1] https://lkml.kernel.org/r/[e-mail]
>> [2] https://lkml.kernel.org/r/[e-mail]
>> [3] https://lkml.kernel.org/r/[e-mail]
>> [4] https://lkml.kernel.org/r/[e-mail]
>> [5] https://lkml.kernel.org/r/[e-mail]
>>
>> Cc: Andrew Morton <[e-mail]>
>> Cc: "Matthew Wilcox (Oracle)" <[e-mail]>
>> Cc: Hugh Dickins <[e-mail]>
>> Cc: Ryan Roberts <[e-mail]>
>> Cc: Yin Fengwei <[e-mail]>
>> Cc: Mike Kravetz <[e-mail]>
>> Cc: Muchun Song <[e-mail]>
>> Cc: Peter Xu <[e-mail]>
>>
>> David Hildenbrand (39):
>>   mm/rmap: rename hugepage_add* to hugetlb_add*
>>   mm/rmap: introduce and use hugetlb_remove_rmap()
>>   mm/rmap: introduce and use hugetlb_add_file_rmap()
>>   mm/rmap: introduce and use hugetlb_try_dup_anon_rmap()
>>   mm/rmap: introduce and use hugetlb_try_share_anon_rmap()
>>   mm/rmap: add hugetlb sanity checks
>>   mm/rmap: convert folio_add_file_rmap_range() into
>>     folio_add_file_rmap_[pte|ptes|pmd]()
>>   mm/memory: page_add_file_rmap() -> folio_add_file_rmap_[pte|pmd]()
>>   mm/huge_memory: page_add_file_rmap() -> folio_add_file_rmap_pmd()
>>   mm/migrate: page_add_file_rmap() -> folio_add_file_rmap_pte()
>>   mm/userfaultfd: page_add_file_rmap() -> folio_add_file_rmap_pte()
>>   mm/rmap: remove page_add_file_rmap()
>>   mm/rmap: factor out adding folio mappings into __folio_add_rmap()
>>   mm/rmap: introduce folio_add_anon_rmap_[pte|ptes|pmd]()
>>   mm/huge_memory: batch rmap operations in __split_huge_pmd_locked()
>>   mm/huge_memory: page_add_anon_rmap() -> folio_add_anon_rmap_pmd()
>>   mm/migrate: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>>   mm/ksm: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>>   mm/swapfile: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>>   mm/memory: page_add_anon_rmap() -> folio_add_anon_rmap_pte()
>>   mm/rmap: remove page_add_anon_rmap()
>>   mm/rmap: remove RMAP_COMPOUND
>>   mm/rmap: introduce folio_remove_rmap_[pte|ptes|pmd]()
>>   kernel/events/uprobes: page_remove_rmap() -> folio_remove_rmap_pte()
>>   mm/huge_memory: page_remove_rmap() -> folio_remove_rmap_pmd()
>>   mm/khugepaged: page_remove_rmap() -> folio_remove_rmap_pte()
>>   mm/ksm: page_remove_rmap() -> folio_remove_rmap_pte()
>>   mm/memory: page_remove_rmap() -> folio_remove_rmap_pte()
>>   mm/migrate_device: page_remove_rmap() -> folio_remove_rmap_pte()
>>   mm/rmap: page_remove_rmap() -> folio_remove_rmap_pte()
>>   Documentation: stop referring to page_remove_rmap()
>>   mm/rmap: remove page_remove_rmap()
>>   mm/rmap: convert page_dup_file_rmap() to
>>     folio_dup_file_rmap_[pte|ptes|pmd]()
>>   mm/rmap: introduce folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>   mm/huge_memory: page_try_dup_anon_rmap() ->
>>     folio_try_dup_anon_rmap_pmd()
>>   mm/memory: page_try_dup_anon_rmap() -> folio_try_dup_anon_rmap_pte()
>>   mm/rmap: remove page_try_dup_anon_rmap()
>>   mm: convert page_try_share_anon_rmap() to
>>     folio_try_share_anon_rmap_[pte|pmd]()
>>   mm/rmap: rename COMPOUND_MAPPED to ENTIRELY_MAPPED
>>
>>  Documentation/mm/transhuge.rst       |   4 +-
>>  Documentation/mm/unevictable-lru.rst |   4 +-
>>  include/linux/mm.h                   |   6 +-
>>  include/linux/rmap.h                 | 380 +++++++++++++++++++-----
>>  kernel/events/uprobes.c              |   2 +-
>>  mm/gup.c                             |   2 +-
>>  mm/huge_memory.c                     |  85 +++---
>>  mm/hugetlb.c                         |  21 +-
>>  mm/internal.h                        |  12 +-
>>  mm/khugepaged.c                      |  17 +-
>>  mm/ksm.c                             |  15 +-
>>  mm/memory-failure.c                  |   4 +-
>>  mm/memory.c                          |  60 ++--
>>  mm/migrate.c                         |  12 +-
>>  mm/migrate_device.c                  |  41 +--
>>  mm/mmu_gather.c                      |   2 +-
>>  mm/rmap.c                            | 422 ++++++++++++++++-----------
>>  mm/swapfile.c                        |   2 +-
>>  mm/userfaultfd.c                     |   2 +-
>>  19 files changed, 709 insertions(+), 384 deletions(-)
>>
>

Ryan Roberts Dec. 5, 2023, 1:31 p.m. UTC | #4

On 05/12/2023 09:56, David Hildenbrand wrote:
>>>
>>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>>> -- he carries his own batching variant right now -- and
>>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
>>
>> Note that the contpte series at [2] has a new patch in v3 (patch 2), which could
>> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
>> on top of [2] once it is merged.
>>
>>>
>>> There is some overlap with both series (and some other work, like
>>> multi-size THP [3]), so that will need some coordination, and likely a
>>> stepwise inclusion.
>>
>> Selfishly, I'd really like to get my stuff merged as soon as there is no
>> technical reason not to. I'd prefer not to add this as a dependency if we can
>> help it.
> 
> It's easy to rework either series on top of each other. The mTHP series has
> highest priority,
> no question, that will go in first.

Music to my ears! It would be great to either get a reviewed-by or feedback on
why not, for the key 2 patches in that series (3 & 4) and also your opinion on
whether we need to wait for compaction to land (see cover letter). It would be
great to get this into linux-next ASAP IMHO.

> 
> Regarding the contpte, I think it needs more work. Especially, as raised, to not
> degrade
> order-0 performance. Maybe we won't make the next merge window (and you already
> predicated
> that in some cover letter :P ). Let's see.

Yeah that's ok. I'll do the work to fix the order-0 perf. And also do the same
for patch 2 in that series - would also be really helpful if you had a chance to
look at patch 2 - its new for v3.

> 
> But again, the conflicts are all trivial, so I'll happily rebase on top of
> whatever is
> in mm-unstable. Or move the relevant rework to the front so you can just carry
> them/base on them. (the batched variants for dup do make the contpte code much
> easier)

So perhaps we should aim for mTHP, then this, then contpte last, benefiting from
the batching.

> 
> [...]
> 
>>>
>>>
>>> New (extended) hugetlb interface that operate on entire folio:
>>>   * hugetlb_add_new_anon_rmap() -> Already existed
>>>   * hugetlb_add_anon_rmap() -> Already existed
>>>   * hugetlb_try_dup_anon_rmap()
>>>   * hugetlb_try_share_anon_rmap()
>>>   * hugetlb_add_file_rmap()
>>>   * hugetlb_remove_rmap()
>>>
>>> New "ordinary" interface for small folios / THP::
>>>   * folio_add_new_anon_rmap() -> Already existed
>>>   * folio_add_anon_rmap_[pte|ptes|pmd]()
>>>   * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>>   * folio_try_share_anon_rmap_[pte|pmd]()
>>>   * folio_add_file_rmap_[pte|ptes|pmd]()
>>>   * folio_dup_file_rmap_[pte|ptes|pmd]()
>>>   * folio_remove_rmap_[pte|ptes|pmd]()
>>
>> I'm not sure if there are official guidelines, but personally if we are
>> reworking the API, I'd take the opportunity to move "rmap" to the front of the
>> name, rather than having it burried in the middle as it is for some of these:
>>
>> rmap_hugetlb_*()
>>
>> rmap_folio_*()
> 
> No strong opinion. But we might want slightly different names then. For example,
> it's "bio_add_folio" and not "bio_folio_add":
> 
> 
> rmap_add_new_anon_hugetlb()
> rmap_add_anon_hugetlb()
> ...
> rmap_remove_hugetlb()
> 
> 
> rmap_add_new_anon_folio()
> rmap_add_anon_folio_[pte|ptes|pmd]()
> ...
> rmap_dup_file_folio_[pte|ptes|pmd]()
> rmap_remove_folio_[pte|ptes|pmd]()
> 
> Thoughts?

Having now reviewed your series, I have a less strong opinion, perhaps it's
actually best with your original names; "folio" is actually the subject after
all; it's the thing being operated on.


> 
>>
>> I guess reading the patches will tell me, but what's the point of "ptes"? Surely
>> you're either mapping at pte or pmd level, and the number of pages is determined
>> by the folio size? (or presumably nr param passed in)
> 
> It's really (currently) one function to handle 1 vs. multiple PTEs. For example:
> 
> void folio_remove_rmap_ptes(struct folio *, struct page *, unsigned int nr,
>         struct vm_area_struct *);
> #define folio_remove_rmap_pte(folio, page, vma) \
>     folio_remove_rmap_ptes(folio, page, 1, vma)
> void folio_remove_rmap_pmd(struct folio *, struct page *,
>         struct vm_area_struct *);

Yeah now that I've looked at the series, this makes sense. "ptes" was originally
making me think of contpte, but I suspect I'll be the only one with that
association :)
> 
> 
> Once could let the compiler generate specialized variants for the single-pte
> versions to make the order-0 case faster. For now it's just a helper macro.
>

David Hildenbrand Dec. 5, 2023, 1:39 p.m. UTC | #5

On 05.12.23 14:31, Ryan Roberts wrote:
> On 05/12/2023 09:56, David Hildenbrand wrote:
>>>>
>>>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>>>> -- he carries his own batching variant right now -- and
>>>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
>>>
>>> Note that the contpte series at [2] has a new patch in v3 (patch 2), which could
>>> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
>>> on top of [2] once it is merged.
>>>
>>>>
>>>> There is some overlap with both series (and some other work, like
>>>> multi-size THP [3]), so that will need some coordination, and likely a
>>>> stepwise inclusion.
>>>
>>> Selfishly, I'd really like to get my stuff merged as soon as there is no
>>> technical reason not to. I'd prefer not to add this as a dependency if we can
>>> help it.
>>
>> It's easy to rework either series on top of each other. The mTHP series has
>> highest priority,
>> no question, that will go in first.
> 
> Music to my ears! It would be great to either get a reviewed-by or feedback on
> why not, for the key 2 patches in that series (3 & 4) and also your opinion on
> whether we need to wait for compaction to land (see cover letter). It would be
> great to get this into linux-next ASAP IMHO.

On it :)

> 
>>
>> Regarding the contpte, I think it needs more work. Especially, as raised, to not
>> degrade
>> order-0 performance. Maybe we won't make the next merge window (and you already
>> predicated
>> that in some cover letter :P ). Let's see.
> 
> Yeah that's ok. I'll do the work to fix the order-0 perf. And also do the same
> for patch 2 in that series - would also be really helpful if you had a chance to
> look at patch 2 - its new for v3.

I only skimmed over it, but it seems to go into the direction we'll 
need. Keeping order-0 performance unharmed should have highest priority. 
Hopefully my microbenchmarks are helpful.

> 
>>
>> But again, the conflicts are all trivial, so I'll happily rebase on top of
>> whatever is
>> in mm-unstable. Or move the relevant rework to the front so you can just carry
>> them/base on them. (the batched variants for dup do make the contpte code much
>> easier)
> 
> So perhaps we should aim for mTHP, then this, then contpte last, benefiting from
> the batching.

Yeah. And again, I don't care too much if I have to rebase on top of 
your work if this here takes longer. It's all a fairly trivial conversion.

>>
>> [...]
>>
>>>>
>>>>
>>>> New (extended) hugetlb interface that operate on entire folio:
>>>>    * hugetlb_add_new_anon_rmap() -> Already existed
>>>>    * hugetlb_add_anon_rmap() -> Already existed
>>>>    * hugetlb_try_dup_anon_rmap()
>>>>    * hugetlb_try_share_anon_rmap()
>>>>    * hugetlb_add_file_rmap()
>>>>    * hugetlb_remove_rmap()
>>>>
>>>> New "ordinary" interface for small folios / THP::
>>>>    * folio_add_new_anon_rmap() -> Already existed
>>>>    * folio_add_anon_rmap_[pte|ptes|pmd]()
>>>>    * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>>>    * folio_try_share_anon_rmap_[pte|pmd]()
>>>>    * folio_add_file_rmap_[pte|ptes|pmd]()
>>>>    * folio_dup_file_rmap_[pte|ptes|pmd]()
>>>>    * folio_remove_rmap_[pte|ptes|pmd]()
>>>
>>> I'm not sure if there are official guidelines, but personally if we are
>>> reworking the API, I'd take the opportunity to move "rmap" to the front of the
>>> name, rather than having it burried in the middle as it is for some of these:
>>>
>>> rmap_hugetlb_*()
>>>
>>> rmap_folio_*()
>>
>> No strong opinion. But we might want slightly different names then. For example,
>> it's "bio_add_folio" and not "bio_folio_add":
>>
>>
>> rmap_add_new_anon_hugetlb()
>> rmap_add_anon_hugetlb()
>> ...
>> rmap_remove_hugetlb()
>>
>>
>> rmap_add_new_anon_folio()
>> rmap_add_anon_folio_[pte|ptes|pmd]()
>> ...
>> rmap_dup_file_folio_[pte|ptes|pmd]()
>> rmap_remove_folio_[pte|ptes|pmd]()
>>
>> Thoughts?
> 
> Having now reviewed your series, I have a less strong opinion, perhaps it's
> actually best with your original names; "folio" is actually the subject after
> all; it's the thing being operated on.
> 

I think having "folio" in there looks cleaner and more consistent to 
other functions.

I tend to like "rmap_dup_file_folio_[pte|ptes|pmd]()", because then we 
have "file folio" and "anon folio" as one word.

But then I wonder about the hugetlb part. Maybe simply 
"hugetlb_rmap_remove_folio()" etc.

Having the "hugetlb_" prefix at the beginning feels like the right thing 
to do, looking at orher hugetlb special-handlings.

But I'll wait a bit until I go crazy on renaming :)

> 
>>
>>>
>>> I guess reading the patches will tell me, but what's the point of "ptes"? Surely
>>> you're either mapping at pte or pmd level, and the number of pages is determined
>>> by the folio size? (or presumably nr param passed in)
>>
>> It's really (currently) one function to handle 1 vs. multiple PTEs. For example:
>>
>> void folio_remove_rmap_ptes(struct folio *, struct page *, unsigned int nr,
>>          struct vm_area_struct *);
>> #define folio_remove_rmap_pte(folio, page, vma) \
>>      folio_remove_rmap_ptes(folio, page, 1, vma)
>> void folio_remove_rmap_pmd(struct folio *, struct page *,
>>          struct vm_area_struct *);
> 
> Yeah now that I've looked at the series, this makes sense. "ptes" was originally
> making me think of contpte, but I suspect I'll be the only one with that
> association :)

Ah, yes :)

Ryan Roberts Dec. 5, 2023, 1:49 p.m. UTC | #6

On 05/12/2023 13:39, David Hildenbrand wrote:
> On 05.12.23 14:31, Ryan Roberts wrote:
>> On 05/12/2023 09:56, David Hildenbrand wrote:
>>>>>
>>>>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>>>>> -- he carries his own batching variant right now -- and
>>>>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
>>>>
>>>> Note that the contpte series at [2] has a new patch in v3 (patch 2), which
>>>> could
>>>> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
>>>> on top of [2] once it is merged.
>>>>
>>>>>
>>>>> There is some overlap with both series (and some other work, like
>>>>> multi-size THP [3]), so that will need some coordination, and likely a
>>>>> stepwise inclusion.
>>>>
>>>> Selfishly, I'd really like to get my stuff merged as soon as there is no
>>>> technical reason not to. I'd prefer not to add this as a dependency if we can
>>>> help it.
>>>
>>> It's easy to rework either series on top of each other. The mTHP series has
>>> highest priority,
>>> no question, that will go in first.
>>
>> Music to my ears! It would be great to either get a reviewed-by or feedback on
>> why not, for the key 2 patches in that series (3 & 4) and also your opinion on
>> whether we need to wait for compaction to land (see cover letter). It would be
>> great to get this into linux-next ASAP IMHO.
> 
> On it :)
> 
>>
>>>
>>> Regarding the contpte, I think it needs more work. Especially, as raised, to not
>>> degrade
>>> order-0 performance. Maybe we won't make the next merge window (and you already
>>> predicated
>>> that in some cover letter :P ). Let's see.
>>
>> Yeah that's ok. I'll do the work to fix the order-0 perf. And also do the same
>> for patch 2 in that series - would also be really helpful if you had a chance to
>> look at patch 2 - its new for v3.
> 
> I only skimmed over it, but it seems to go into the direction we'll need.
> Keeping order-0 performance unharmed should have highest priority. Hopefully my
> microbenchmarks are helpful.

Yes absolutely - are you able to share them??

> 
>>
>>>
>>> But again, the conflicts are all trivial, so I'll happily rebase on top of
>>> whatever is
>>> in mm-unstable. Or move the relevant rework to the front so you can just carry
>>> them/base on them. (the batched variants for dup do make the contpte code much
>>> easier)
>>
>> So perhaps we should aim for mTHP, then this, then contpte last, benefiting from
>> the batching.
> 
> Yeah. And again, I don't care too much if I have to rebase on top of your work
> if this here takes longer. It's all a fairly trivial conversion.
> 
>>>
>>> [...]
>>>
>>>>>
>>>>>
>>>>> New (extended) hugetlb interface that operate on entire folio:
>>>>>    * hugetlb_add_new_anon_rmap() -> Already existed
>>>>>    * hugetlb_add_anon_rmap() -> Already existed
>>>>>    * hugetlb_try_dup_anon_rmap()
>>>>>    * hugetlb_try_share_anon_rmap()
>>>>>    * hugetlb_add_file_rmap()
>>>>>    * hugetlb_remove_rmap()
>>>>>
>>>>> New "ordinary" interface for small folios / THP::
>>>>>    * folio_add_new_anon_rmap() -> Already existed
>>>>>    * folio_add_anon_rmap_[pte|ptes|pmd]()
>>>>>    * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>>>>    * folio_try_share_anon_rmap_[pte|pmd]()
>>>>>    * folio_add_file_rmap_[pte|ptes|pmd]()
>>>>>    * folio_dup_file_rmap_[pte|ptes|pmd]()
>>>>>    * folio_remove_rmap_[pte|ptes|pmd]()
>>>>
>>>> I'm not sure if there are official guidelines, but personally if we are
>>>> reworking the API, I'd take the opportunity to move "rmap" to the front of the
>>>> name, rather than having it burried in the middle as it is for some of these:
>>>>
>>>> rmap_hugetlb_*()
>>>>
>>>> rmap_folio_*()
>>>
>>> No strong opinion. But we might want slightly different names then. For example,
>>> it's "bio_add_folio" and not "bio_folio_add":
>>>
>>>
>>> rmap_add_new_anon_hugetlb()
>>> rmap_add_anon_hugetlb()
>>> ...
>>> rmap_remove_hugetlb()
>>>
>>>
>>> rmap_add_new_anon_folio()
>>> rmap_add_anon_folio_[pte|ptes|pmd]()
>>> ...
>>> rmap_dup_file_folio_[pte|ptes|pmd]()
>>> rmap_remove_folio_[pte|ptes|pmd]()
>>>
>>> Thoughts?
>>
>> Having now reviewed your series, I have a less strong opinion, perhaps it's
>> actually best with your original names; "folio" is actually the subject after
>> all; it's the thing being operated on.
>>
> 
> I think having "folio" in there looks cleaner and more consistent to other
> functions.
> 
> I tend to like "rmap_dup_file_folio_[pte|ptes|pmd]()", because then we have
> "file folio" and "anon folio" as one word.
> 
> But then I wonder about the hugetlb part. Maybe simply
> "hugetlb_rmap_remove_folio()" etc.
> 
> Having the "hugetlb_" prefix at the beginning feels like the right thing to do,
> looking at orher hugetlb special-handlings.
> 
> But I'll wait a bit until I go crazy on renaming :)

I suspect we could argue in multiple directions for hours :)

Let's see if others have opinions.

FWIW, I've looked through all the patches; I like what I see! This is a really
nice clean up and will definitely help with the various patch sets I've been
working on. Apart from the comments I've already raised, looks in pretty good
shape to me.

> 
>>
>>>
>>>>
>>>> I guess reading the patches will tell me, but what's the point of "ptes"?
>>>> Surely
>>>> you're either mapping at pte or pmd level, and the number of pages is
>>>> determined
>>>> by the folio size? (or presumably nr param passed in)
>>>
>>> It's really (currently) one function to handle 1 vs. multiple PTEs. For example:
>>>
>>> void folio_remove_rmap_ptes(struct folio *, struct page *, unsigned int nr,
>>>          struct vm_area_struct *);
>>> #define folio_remove_rmap_pte(folio, page, vma) \
>>>      folio_remove_rmap_ptes(folio, page, 1, vma)
>>> void folio_remove_rmap_pmd(struct folio *, struct page *,
>>>          struct vm_area_struct *);
>>
>> Yeah now that I've looked at the series, this makes sense. "ptes" was originally
>> making me think of contpte, but I suspect I'll be the only one with that
>> association :)
> 
> Ah, yes :)
>

David Hildenbrand Dec. 5, 2023, 1:55 p.m. UTC | #7

>>>>>>>
>>>> Regarding the contpte, I think it needs more work. Especially, as raised, to not
>>>> degrade
>>>> order-0 performance. Maybe we won't make the next merge window (and you already
>>>> predicated
>>>> that in some cover letter :P ). Let's see.
>>>
>>> Yeah that's ok. I'll do the work to fix the order-0 perf. And also do the same
>>> for patch 2 in that series - would also be really helpful if you had a chance to
>>> look at patch 2 - its new for v3.
>>
>> I only skimmed over it, but it seems to go into the direction we'll need.
>> Keeping order-0 performance unharmed should have highest priority. Hopefully my
>> microbenchmarks are helpful.
> 
> Yes absolutely - are you able to share them??

I shared them in the reply to your patchset. Let me know if you can't 
find them.

[...]

>>> Having now reviewed your series, I have a less strong opinion, perhaps it's
>>> actually best with your original names; "folio" is actually the subject after
>>> all; it's the thing being operated on.
>>>
>>
>> I think having "folio" in there looks cleaner and more consistent to other
>> functions.
>>
>> I tend to like "rmap_dup_file_folio_[pte|ptes|pmd]()", because then we have
>> "file folio" and "anon folio" as one word.
>>
>> But then I wonder about the hugetlb part. Maybe simply
>> "hugetlb_rmap_remove_folio()" etc.
>>
>> Having the "hugetlb_" prefix at the beginning feels like the right thing to do,
>> looking at orher hugetlb special-handlings.
>>
>> But I'll wait a bit until I go crazy on renaming :)
> 
> I suspect we could argue in multiple directions for hours :)

:)

> 
> Let's see if others have opinions.
> 
> FWIW, I've looked through all the patches; I like what I see! This is a really
> nice clean up and will definitely help with the various patch sets I've been
> working on. Apart from the comments I've already raised, looks in pretty good
> shape to me.

Thanks!

David Hildenbrand Dec. 8, 2023, 11:24 a.m. UTC | #8

On 05.12.23 14:49, Ryan Roberts wrote:
> On 05/12/2023 13:39, David Hildenbrand wrote:
>> On 05.12.23 14:31, Ryan Roberts wrote:
>>> On 05/12/2023 09:56, David Hildenbrand wrote:
>>>>>>
>>>>>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>>>>>> -- he carries his own batching variant right now -- and
>>>>>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
>>>>>
>>>>> Note that the contpte series at [2] has a new patch in v3 (patch 2), which
>>>>> could
>>>>> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive [1]
>>>>> on top of [2] once it is merged.
>>>>>
>>>>>>
>>>>>> There is some overlap with both series (and some other work, like
>>>>>> multi-size THP [3]), so that will need some coordination, and likely a
>>>>>> stepwise inclusion.
>>>>>
>>>>> Selfishly, I'd really like to get my stuff merged as soon as there is no
>>>>> technical reason not to. I'd prefer not to add this as a dependency if we can
>>>>> help it.
>>>>
>>>> It's easy to rework either series on top of each other. The mTHP series has
>>>> highest priority,
>>>> no question, that will go in first.
>>>
>>> Music to my ears! It would be great to either get a reviewed-by or feedback on
>>> why not, for the key 2 patches in that series (3 & 4) and also your opinion on
>>> whether we need to wait for compaction to land (see cover letter). It would be
>>> great to get this into linux-next ASAP IMHO.
>>
>> On it :)
>>
>>>
>>>>
>>>> Regarding the contpte, I think it needs more work. Especially, as raised, to not
>>>> degrade
>>>> order-0 performance. Maybe we won't make the next merge window (and you already
>>>> predicated
>>>> that in some cover letter :P ). Let's see.
>>>
>>> Yeah that's ok. I'll do the work to fix the order-0 perf. And also do the same
>>> for patch 2 in that series - would also be really helpful if you had a chance to
>>> look at patch 2 - its new for v3.
>>
>> I only skimmed over it, but it seems to go into the direction we'll need.
>> Keeping order-0 performance unharmed should have highest priority. Hopefully my
>> microbenchmarks are helpful.
> 
> Yes absolutely - are you able to share them??
> 
>>
>>>
>>>>
>>>> But again, the conflicts are all trivial, so I'll happily rebase on top of
>>>> whatever is
>>>> in mm-unstable. Or move the relevant rework to the front so you can just carry
>>>> them/base on them. (the batched variants for dup do make the contpte code much
>>>> easier)
>>>
>>> So perhaps we should aim for mTHP, then this, then contpte last, benefiting from
>>> the batching.
>>
>> Yeah. And again, I don't care too much if I have to rebase on top of your work
>> if this here takes longer. It's all a fairly trivial conversion.
>>
>>>>
>>>> [...]
>>>>
>>>>>>
>>>>>>
>>>>>> New (extended) hugetlb interface that operate on entire folio:
>>>>>>     * hugetlb_add_new_anon_rmap() -> Already existed
>>>>>>     * hugetlb_add_anon_rmap() -> Already existed
>>>>>>     * hugetlb_try_dup_anon_rmap()
>>>>>>     * hugetlb_try_share_anon_rmap()
>>>>>>     * hugetlb_add_file_rmap()
>>>>>>     * hugetlb_remove_rmap()
>>>>>>
>>>>>> New "ordinary" interface for small folios / THP::
>>>>>>     * folio_add_new_anon_rmap() -> Already existed
>>>>>>     * folio_add_anon_rmap_[pte|ptes|pmd]()
>>>>>>     * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>>>>>     * folio_try_share_anon_rmap_[pte|pmd]()
>>>>>>     * folio_add_file_rmap_[pte|ptes|pmd]()
>>>>>>     * folio_dup_file_rmap_[pte|ptes|pmd]()
>>>>>>     * folio_remove_rmap_[pte|ptes|pmd]()
>>>>>
>>>>> I'm not sure if there are official guidelines, but personally if we are
>>>>> reworking the API, I'd take the opportunity to move "rmap" to the front of the
>>>>> name, rather than having it burried in the middle as it is for some of these:
>>>>>
>>>>> rmap_hugetlb_*()
>>>>>
>>>>> rmap_folio_*()
>>>>
>>>> No strong opinion. But we might want slightly different names then. For example,
>>>> it's "bio_add_folio" and not "bio_folio_add":
>>>>
>>>>
>>>> rmap_add_new_anon_hugetlb()
>>>> rmap_add_anon_hugetlb()
>>>> ...
>>>> rmap_remove_hugetlb()
>>>>
>>>>
>>>> rmap_add_new_anon_folio()
>>>> rmap_add_anon_folio_[pte|ptes|pmd]()
>>>> ...
>>>> rmap_dup_file_folio_[pte|ptes|pmd]()
>>>> rmap_remove_folio_[pte|ptes|pmd]()
>>>>
>>>> Thoughts?
>>>
>>> Having now reviewed your series, I have a less strong opinion, perhaps it's
>>> actually best with your original names; "folio" is actually the subject after
>>> all; it's the thing being operated on.

So far I sticked to the original names used in this RFC. I'm testing a 
new series that is based on current mm/unstable (especially, mTHP) and 
contains all changes discussed here.

If I don't here anything else, I'll send that out as v1 on Monday.

Thanks!

Ryan Roberts Dec. 8, 2023, 11:38 a.m. UTC | #9

On 08/12/2023 11:24, David Hildenbrand wrote:
> On 05.12.23 14:49, Ryan Roberts wrote:
>> On 05/12/2023 13:39, David Hildenbrand wrote:
>>> On 05.12.23 14:31, Ryan Roberts wrote:
>>>> On 05/12/2023 09:56, David Hildenbrand wrote:
>>>>>>>
>>>>>>> Ryan has series where we would make use of folio_remove_rmap_ptes() [1]
>>>>>>> -- he carries his own batching variant right now -- and
>>>>>>> folio_try_dup_anon_rmap_ptes()/folio_dup_file_rmap_ptes() [2].
>>>>>>
>>>>>> Note that the contpte series at [2] has a new patch in v3 (patch 2), which
>>>>>> could
>>>>>> benefit from folio_remove_rmap_ptes() or equivalent. My plan was to revive
>>>>>> [1]
>>>>>> on top of [2] once it is merged.
>>>>>>
>>>>>>>
>>>>>>> There is some overlap with both series (and some other work, like
>>>>>>> multi-size THP [3]), so that will need some coordination, and likely a
>>>>>>> stepwise inclusion.
>>>>>>
>>>>>> Selfishly, I'd really like to get my stuff merged as soon as there is no
>>>>>> technical reason not to. I'd prefer not to add this as a dependency if we can
>>>>>> help it.
>>>>>
>>>>> It's easy to rework either series on top of each other. The mTHP series has
>>>>> highest priority,
>>>>> no question, that will go in first.
>>>>
>>>> Music to my ears! It would be great to either get a reviewed-by or feedback on
>>>> why not, for the key 2 patches in that series (3 & 4) and also your opinion on
>>>> whether we need to wait for compaction to land (see cover letter). It would be
>>>> great to get this into linux-next ASAP IMHO.
>>>
>>> On it :)
>>>
>>>>
>>>>>
>>>>> Regarding the contpte, I think it needs more work. Especially, as raised,
>>>>> to not
>>>>> degrade
>>>>> order-0 performance. Maybe we won't make the next merge window (and you
>>>>> already
>>>>> predicated
>>>>> that in some cover letter :P ). Let's see.
>>>>
>>>> Yeah that's ok. I'll do the work to fix the order-0 perf. And also do the same
>>>> for patch 2 in that series - would also be really helpful if you had a
>>>> chance to
>>>> look at patch 2 - its new for v3.
>>>
>>> I only skimmed over it, but it seems to go into the direction we'll need.
>>> Keeping order-0 performance unharmed should have highest priority. Hopefully my
>>> microbenchmarks are helpful.
>>
>> Yes absolutely - are you able to share them??
>>
>>>
>>>>
>>>>>
>>>>> But again, the conflicts are all trivial, so I'll happily rebase on top of
>>>>> whatever is
>>>>> in mm-unstable. Or move the relevant rework to the front so you can just carry
>>>>> them/base on them. (the batched variants for dup do make the contpte code much
>>>>> easier)
>>>>
>>>> So perhaps we should aim for mTHP, then this, then contpte last, benefiting
>>>> from
>>>> the batching.
>>>
>>> Yeah. And again, I don't care too much if I have to rebase on top of your work
>>> if this here takes longer. It's all a fairly trivial conversion.
>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> New (extended) hugetlb interface that operate on entire folio:
>>>>>>>     * hugetlb_add_new_anon_rmap() -> Already existed
>>>>>>>     * hugetlb_add_anon_rmap() -> Already existed
>>>>>>>     * hugetlb_try_dup_anon_rmap()
>>>>>>>     * hugetlb_try_share_anon_rmap()
>>>>>>>     * hugetlb_add_file_rmap()
>>>>>>>     * hugetlb_remove_rmap()
>>>>>>>
>>>>>>> New "ordinary" interface for small folios / THP::
>>>>>>>     * folio_add_new_anon_rmap() -> Already existed
>>>>>>>     * folio_add_anon_rmap_[pte|ptes|pmd]()
>>>>>>>     * folio_try_dup_anon_rmap_[pte|ptes|pmd]()
>>>>>>>     * folio_try_share_anon_rmap_[pte|pmd]()
>>>>>>>     * folio_add_file_rmap_[pte|ptes|pmd]()
>>>>>>>     * folio_dup_file_rmap_[pte|ptes|pmd]()
>>>>>>>     * folio_remove_rmap_[pte|ptes|pmd]()
>>>>>>
>>>>>> I'm not sure if there are official guidelines, but personally if we are
>>>>>> reworking the API, I'd take the opportunity to move "rmap" to the front of
>>>>>> the
>>>>>> name, rather than having it burried in the middle as it is for some of these:
>>>>>>
>>>>>> rmap_hugetlb_*()
>>>>>>
>>>>>> rmap_folio_*()
>>>>>
>>>>> No strong opinion. But we might want slightly different names then. For
>>>>> example,
>>>>> it's "bio_add_folio" and not "bio_folio_add":
>>>>>
>>>>>
>>>>> rmap_add_new_anon_hugetlb()
>>>>> rmap_add_anon_hugetlb()
>>>>> ...
>>>>> rmap_remove_hugetlb()
>>>>>
>>>>>
>>>>> rmap_add_new_anon_folio()
>>>>> rmap_add_anon_folio_[pte|ptes|pmd]()
>>>>> ...
>>>>> rmap_dup_file_folio_[pte|ptes|pmd]()
>>>>> rmap_remove_folio_[pte|ptes|pmd]()
>>>>>
>>>>> Thoughts?
>>>>
>>>> Having now reviewed your series, I have a less strong opinion, perhaps it's
>>>> actually best with your original names; "folio" is actually the subject after
>>>> all; it's the thing being operated on.
> 
> So far I sticked to the original names used in this RFC. I'm testing a new
> series that is based on current mm/unstable (especially, mTHP) and contains all
> changes discussed here.
> 
> If I don't here anything else, I'll send that out as v1 on Monday.

Get's my vote!

> 
> Thanks!
>