reuse_swap_page() currently indicates if we can write to an anon page
without COW. A COW is required if the page is shared by multiple
processes (either already mapped or via swap entries) or if there is
concurrent writeback that cannot tolerate concurrent page modifications.
However, in the context of khugepaged we're not actually going to write
to a read-only mapped page, we'll copy the page content to our newly
allocated THP and map that THP writable. All we have to make sure
is that the read-only mapped page we're about to copy won't get reused
by another process sharing the page, otherwise, page content would
get modified. But that is already guaranteed via multiple mechanisms
(e.g., holding a reference, holding the page lock, removing the rmap after
copying the page).
The swapcache handling was introduced in commit 10359213d05a ("mm:
incorporate read-only pages into transparent huge pages") and it sounds
like it merely wanted to mimic what do_swap_page() would do when trying
to map a page obtained via the swapcache writable.
As that logic is unnecessary, let's just remove it, removing the last
user of reuse_swap_page().
Signed-off-by: David Hildenbrand <[email protected]>
---
include/trace/events/huge_memory.h | 1 -
mm/khugepaged.c | 11 -----------
2 files changed, 12 deletions(-)
diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
index 4fdb14a81108..d651f3437367 100644
--- a/include/trace/events/huge_memory.h
+++ b/include/trace/events/huge_memory.h
@@ -29,7 +29,6 @@
EM( SCAN_VMA_NULL, "vma_null") \
EM( SCAN_VMA_CHECK, "vma_check_failed") \
EM( SCAN_ADDRESS_RANGE, "not_suitable_address_range") \
- EM( SCAN_SWAP_CACHE_PAGE, "page_swap_cache") \
EM( SCAN_DEL_PAGE_LRU, "could_not_delete_page_from_lru")\
EM( SCAN_ALLOC_HUGE_PAGE_FAIL, "alloc_huge_page_failed") \
EM( SCAN_CGROUP_CHARGE_FAIL, "ccgroup_charge_failed") \
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 35f14d0a00a6..9da9325ab4d4 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -45,7 +45,6 @@ enum scan_result {
SCAN_VMA_NULL,
SCAN_VMA_CHECK,
SCAN_ADDRESS_RANGE,
- SCAN_SWAP_CACHE_PAGE,
SCAN_DEL_PAGE_LRU,
SCAN_ALLOC_HUGE_PAGE_FAIL,
SCAN_CGROUP_CHARGE_FAIL,
@@ -682,16 +681,6 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
result = SCAN_PAGE_COUNT;
goto out;
}
- if (!pte_write(pteval) && PageSwapCache(page) &&
- !reuse_swap_page(page)) {
- /*
- * Page is in the swap cache and cannot be re-used.
- * It cannot be collapsed into a THP.
- */
- unlock_page(page);
- result = SCAN_SWAP_CACHE_PAGE;
- goto out;
- }
/*
* Isolate the page to avoid collapsing an hugepage
--
2.34.1
On Mon, Jan 31, 2022 at 8:33 AM David Hildenbrand <[email protected]> wrote:
>
> reuse_swap_page() currently indicates if we can write to an anon page
> without COW. A COW is required if the page is shared by multiple
> processes (either already mapped or via swap entries) or if there is
> concurrent writeback that cannot tolerate concurrent page modifications.
>
> However, in the context of khugepaged we're not actually going to write
> to a read-only mapped page, we'll copy the page content to our newly
> allocated THP and map that THP writable. All we have to make sure
> is that the read-only mapped page we're about to copy won't get reused
> by another process sharing the page, otherwise, page content would
> get modified. But that is already guaranteed via multiple mechanisms
> (e.g., holding a reference, holding the page lock, removing the rmap after
> copying the page).
>
> The swapcache handling was introduced in commit 10359213d05a ("mm:
> incorporate read-only pages into transparent huge pages") and it sounds
> like it merely wanted to mimic what do_swap_page() would do when trying
> to map a page obtained via the swapcache writable.
>
> As that logic is unnecessary, let's just remove it, removing the last
> user of reuse_swap_page().
Thanks for cleaning this up. I didn't spot anything wrong. You could
add Reviewed-by: Yang Shi <[email protected]>
>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> include/trace/events/huge_memory.h | 1 -
> mm/khugepaged.c | 11 -----------
> 2 files changed, 12 deletions(-)
>
> diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> index 4fdb14a81108..d651f3437367 100644
> --- a/include/trace/events/huge_memory.h
> +++ b/include/trace/events/huge_memory.h
> @@ -29,7 +29,6 @@
> EM( SCAN_VMA_NULL, "vma_null") \
> EM( SCAN_VMA_CHECK, "vma_check_failed") \
> EM( SCAN_ADDRESS_RANGE, "not_suitable_address_range") \
> - EM( SCAN_SWAP_CACHE_PAGE, "page_swap_cache") \
> EM( SCAN_DEL_PAGE_LRU, "could_not_delete_page_from_lru")\
> EM( SCAN_ALLOC_HUGE_PAGE_FAIL, "alloc_huge_page_failed") \
> EM( SCAN_CGROUP_CHARGE_FAIL, "ccgroup_charge_failed") \
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 35f14d0a00a6..9da9325ab4d4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -45,7 +45,6 @@ enum scan_result {
> SCAN_VMA_NULL,
> SCAN_VMA_CHECK,
> SCAN_ADDRESS_RANGE,
> - SCAN_SWAP_CACHE_PAGE,
> SCAN_DEL_PAGE_LRU,
> SCAN_ALLOC_HUGE_PAGE_FAIL,
> SCAN_CGROUP_CHARGE_FAIL,
> @@ -682,16 +681,6 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> result = SCAN_PAGE_COUNT;
> goto out;
> }
> - if (!pte_write(pteval) && PageSwapCache(page) &&
> - !reuse_swap_page(page)) {
> - /*
> - * Page is in the swap cache and cannot be re-used.
> - * It cannot be collapsed into a THP.
> - */
> - unlock_page(page);
> - result = SCAN_SWAP_CACHE_PAGE;
> - goto out;
> - }
>
> /*
> * Isolate the page to avoid collapsing an hugepage
> --
> 2.34.1
>
On 1/31/22 17:29, David Hildenbrand wrote:
> reuse_swap_page() currently indicates if we can write to an anon page
> without COW. A COW is required if the page is shared by multiple
> processes (either already mapped or via swap entries) or if there is
> concurrent writeback that cannot tolerate concurrent page modifications.
>
> However, in the context of khugepaged we're not actually going to write
> to a read-only mapped page, we'll copy the page content to our newly
> allocated THP and map that THP writable. All we have to make sure
> is that the read-only mapped page we're about to copy won't get reused
> by another process sharing the page, otherwise, page content would
> get modified. But that is already guaranteed via multiple mechanisms
> (e.g., holding a reference, holding the page lock, removing the rmap after
> copying the page).
>
> The swapcache handling was introduced in commit 10359213d05a ("mm:
> incorporate read-only pages into transparent huge pages") and it sounds
> like it merely wanted to mimic what do_swap_page() would do when trying
> to map a page obtained via the swapcache writable.
>
> As that logic is unnecessary, let's just remove it, removing the last
> user of reuse_swap_page().
>
> Signed-off-by: David Hildenbrand <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
> ---
> include/trace/events/huge_memory.h | 1 -
> mm/khugepaged.c | 11 -----------
> 2 files changed, 12 deletions(-)
>
> diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> index 4fdb14a81108..d651f3437367 100644
> --- a/include/trace/events/huge_memory.h
> +++ b/include/trace/events/huge_memory.h
> @@ -29,7 +29,6 @@
> EM( SCAN_VMA_NULL, "vma_null") \
> EM( SCAN_VMA_CHECK, "vma_check_failed") \
> EM( SCAN_ADDRESS_RANGE, "not_suitable_address_range") \
> - EM( SCAN_SWAP_CACHE_PAGE, "page_swap_cache") \
> EM( SCAN_DEL_PAGE_LRU, "could_not_delete_page_from_lru")\
> EM( SCAN_ALLOC_HUGE_PAGE_FAIL, "alloc_huge_page_failed") \
> EM( SCAN_CGROUP_CHARGE_FAIL, "ccgroup_charge_failed") \
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 35f14d0a00a6..9da9325ab4d4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -45,7 +45,6 @@ enum scan_result {
> SCAN_VMA_NULL,
> SCAN_VMA_CHECK,
> SCAN_ADDRESS_RANGE,
> - SCAN_SWAP_CACHE_PAGE,
> SCAN_DEL_PAGE_LRU,
> SCAN_ALLOC_HUGE_PAGE_FAIL,
> SCAN_CGROUP_CHARGE_FAIL,
> @@ -682,16 +681,6 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> result = SCAN_PAGE_COUNT;
> goto out;
> }
> - if (!pte_write(pteval) && PageSwapCache(page) &&
> - !reuse_swap_page(page)) {
> - /*
> - * Page is in the swap cache and cannot be re-used.
> - * It cannot be collapsed into a THP.
> - */
> - unlock_page(page);
> - result = SCAN_SWAP_CACHE_PAGE;
> - goto out;
> - }
>
> /*
> * Isolate the page to avoid collapsing an hugepage