* [v2 linux-next PATCH 1/2] mm: khugepaged: add exceed_max_ptes_* helpers
@ 2020-04-30 20:41 Yang Shi
2020-04-30 20:41 ` [v2 linux-next PATCH 2/2] mm: khugepaged: don't have to put being freed page back to lru Yang Shi
0 siblings, 1 reply; 4+ messages in thread
From: Yang Shi @ 2020-04-30 20:41 UTC (permalink / raw)
To: kirill.shutemov, hughd, aarcange, akpm; +Cc: yang.shi, linux-mm, linux-kernel
The max_ptes_{swap|none|shared} are defined to tune the behavior of
khugepaged. The are checked at a couple of places with open coding.
Replace the opencoding to exceed_pax_ptes_{swap|none_shared} helpers to
improve the readability.
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
mm/khugepaged.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index a02a4c5..0c8d30b 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -339,6 +339,21 @@ struct attribute_group khugepaged_attr_group = {
};
#endif /* CONFIG_SYSFS */
+static inline bool exceed_max_ptes_none(unsigned int *nr_ptes)
+{
+ return (++(*nr_ptes) > khugepaged_max_ptes_none);
+}
+
+static inline bool exceed_max_ptes_swap(unsigned int *nr_ptes)
+{
+ return (++(*nr_ptes) > khugepaged_max_ptes_swap);
+}
+
+static inline bool exceed_max_ptes_shared(unsigned int *nr_ptes)
+{
+ return (++(*nr_ptes) > khugepaged_max_ptes_shared);
+}
+
int hugepage_madvise(struct vm_area_struct *vma,
unsigned long *vm_flags, int advice)
{
@@ -604,7 +619,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
if (pte_none(pteval) || (pte_present(pteval) &&
is_zero_pfn(pte_pfn(pteval)))) {
if (!userfaultfd_armed(vma) &&
- ++none_or_zero <= khugepaged_max_ptes_none) {
+ !exceed_max_ptes_none(&none_or_zero)) {
continue;
} else {
result = SCAN_EXCEED_NONE_PTE;
@@ -624,7 +639,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
VM_BUG_ON_PAGE(!PageAnon(page), page);
if (page_mapcount(page) > 1 &&
- ++shared > khugepaged_max_ptes_shared) {
+ exceed_max_ptes_shared(&shared)) {
result = SCAN_EXCEED_SHARED_PTE;
goto out;
}
@@ -1234,7 +1249,7 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
_pte++, _address += PAGE_SIZE) {
pte_t pteval = *_pte;
if (is_swap_pte(pteval)) {
- if (++unmapped <= khugepaged_max_ptes_swap) {
+ if (!exceed_max_ptes_swap(&unmapped)) {
/*
* Always be strict with uffd-wp
* enabled swap entries. Please see
@@ -1252,7 +1267,7 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
}
if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
if (!userfaultfd_armed(vma) &&
- ++none_or_zero <= khugepaged_max_ptes_none) {
+ !exceed_max_ptes_none(&none_or_zero)) {
continue;
} else {
result = SCAN_EXCEED_NONE_PTE;
@@ -1286,7 +1301,7 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
}
if (page_mapcount(page) > 1 &&
- ++shared > khugepaged_max_ptes_shared) {
+ exceed_max_ptes_shared(&shared)) {
result = SCAN_EXCEED_SHARED_PTE;
goto out_unmap;
}
@@ -1961,7 +1976,7 @@ static void khugepaged_scan_file(struct mm_struct *mm,
continue;
if (xa_is_value(page)) {
- if (++swap > khugepaged_max_ptes_swap) {
+ if (exceed_max_ptes_swap(&swap)) {
result = SCAN_EXCEED_SWAP_PTE;
break;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [v2 linux-next PATCH 2/2] mm: khugepaged: don't have to put being freed page back to lru 2020-04-30 20:41 [v2 linux-next PATCH 1/2] mm: khugepaged: add exceed_max_ptes_* helpers Yang Shi @ 2020-04-30 20:41 ` Yang Shi 2020-05-01 7:04 ` Kirill A. Shutemov 0 siblings, 1 reply; 4+ messages in thread From: Yang Shi @ 2020-04-30 20:41 UTC (permalink / raw) To: kirill.shutemov, hughd, aarcange, akpm; +Cc: yang.shi, linux-mm, linux-kernel When khugepaged successfully isolated and copied data from old page to collapsed THP, the old page is about to be freed if its last mapcount is gone. So putting the page back to lru sounds not that productive in this case since the page might be isolated by vmscan but it can't be reclaimed by vmscan since it can't be unmapped by try_to_unmap() at all. Actually if khugepaged is the last user of this page so it can be freed directly. So, clearing active and unevictable flags, unlocking and dropping refcount from isolate instead of calling putback_lru_page(). Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Cc: Hugh Dickins <hughd@google.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- v2: Check mapcount and skip putback lru if the last mapcount is gone mm/khugepaged.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 0c8d30b..1fdd677 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -559,10 +559,18 @@ void __khugepaged_exit(struct mm_struct *mm) static void release_pte_page(struct page *page) { mod_node_page_state(page_pgdat(page), - NR_ISOLATED_ANON + page_is_file_lru(page), - -compound_nr(page)); - unlock_page(page); - putback_lru_page(page); + NR_ISOLATED_ANON + page_is_file_lru(page), -compound_nr(page)); + + if (total_mapcount(page)) { + unlock_page(page); + putback_lru_page(page); + } else { + ClearPageActive(page); + ClearPageUnevictable(page); + unlock_page(page); + /* Drop refcount from isolate */ + put_page(page); + } } static void release_pte_pages(pte_t *pte, pte_t *_pte, @@ -771,8 +779,6 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, } else { src_page = pte_page(pteval); copy_user_highpage(page, src_page, address, vma); - if (!PageCompound(src_page)) - release_pte_page(src_page); /* * ptl mostly unnecessary, but preempt has to * be disabled to update the per-cpu stats @@ -786,6 +792,8 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, pte_clear(vma->vm_mm, address, _pte); page_remove_rmap(src_page, false); spin_unlock(ptl); + if (!PageCompound(src_page)) + release_pte_page(src_page); free_page_and_swap_cache(src_page); } } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [v2 linux-next PATCH 2/2] mm: khugepaged: don't have to put being freed page back to lru 2020-04-30 20:41 ` [v2 linux-next PATCH 2/2] mm: khugepaged: don't have to put being freed page back to lru Yang Shi @ 2020-05-01 7:04 ` Kirill A. Shutemov 2020-05-01 17:00 ` Yang Shi 0 siblings, 1 reply; 4+ messages in thread From: Kirill A. Shutemov @ 2020-05-01 7:04 UTC (permalink / raw) To: Yang Shi; +Cc: kirill.shutemov, hughd, aarcange, akpm, linux-mm, linux-kernel On Fri, May 01, 2020 at 04:41:19AM +0800, Yang Shi wrote: > When khugepaged successfully isolated and copied data from old page to > collapsed THP, the old page is about to be freed if its last mapcount > is gone. So putting the page back to lru sounds not that productive in > this case since the page might be isolated by vmscan but it can't be > reclaimed by vmscan since it can't be unmapped by try_to_unmap() at all. > > Actually if khugepaged is the last user of this page so it can be freed > directly. So, clearing active and unevictable flags, unlocking and > dropping refcount from isolate instead of calling putback_lru_page(). Any reason putback_lru_page() cannot do it internally? I mean if it is page_count() == 1, free the page. > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Cc: Hugh Dickins <hughd@google.com> > Cc: Andrea Arcangeli <aarcange@redhat.com> > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > --- > v2: Check mapcount and skip putback lru if the last mapcount is gone > > mm/khugepaged.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 0c8d30b..1fdd677 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -559,10 +559,18 @@ void __khugepaged_exit(struct mm_struct *mm) > static void release_pte_page(struct page *page) > { > mod_node_page_state(page_pgdat(page), > - NR_ISOLATED_ANON + page_is_file_lru(page), > - -compound_nr(page)); > - unlock_page(page); > - putback_lru_page(page); > + NR_ISOLATED_ANON + page_is_file_lru(page), -compound_nr(page)); > + > + if (total_mapcount(page)) { > + unlock_page(page); > + putback_lru_page(page); > + } else { > + ClearPageActive(page); > + ClearPageUnevictable(page); > + unlock_page(page); > + /* Drop refcount from isolate */ > + put_page(page); > + } > } > > static void release_pte_pages(pte_t *pte, pte_t *_pte, > @@ -771,8 +779,6 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, > } else { > src_page = pte_page(pteval); > copy_user_highpage(page, src_page, address, vma); > - if (!PageCompound(src_page)) > - release_pte_page(src_page); > /* > * ptl mostly unnecessary, but preempt has to > * be disabled to update the per-cpu stats > @@ -786,6 +792,8 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, > pte_clear(vma->vm_mm, address, _pte); > page_remove_rmap(src_page, false); > spin_unlock(ptl); > + if (!PageCompound(src_page)) > + release_pte_page(src_page); > free_page_and_swap_cache(src_page); > } > } > -- > 1.8.3.1 > > -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v2 linux-next PATCH 2/2] mm: khugepaged: don't have to put being freed page back to lru 2020-05-01 7:04 ` Kirill A. Shutemov @ 2020-05-01 17:00 ` Yang Shi 0 siblings, 0 replies; 4+ messages in thread From: Yang Shi @ 2020-05-01 17:00 UTC (permalink / raw) To: Kirill A. Shutemov Cc: kirill.shutemov, hughd, aarcange, akpm, linux-mm, linux-kernel On 5/1/20 12:04 AM, Kirill A. Shutemov wrote: > On Fri, May 01, 2020 at 04:41:19AM +0800, Yang Shi wrote: >> When khugepaged successfully isolated and copied data from old page to >> collapsed THP, the old page is about to be freed if its last mapcount >> is gone. So putting the page back to lru sounds not that productive in >> this case since the page might be isolated by vmscan but it can't be >> reclaimed by vmscan since it can't be unmapped by try_to_unmap() at all. >> >> Actually if khugepaged is the last user of this page so it can be freed >> directly. So, clearing active and unevictable flags, unlocking and >> dropping refcount from isolate instead of calling putback_lru_page(). > Any reason putback_lru_page() cannot do it internally? I mean if it is > page_count() == 1, free the page. Because it seems putback_lru_page() is just called when the page is *not* about to be freed, for example compaction is aborted, migration is failed on some pages, etc. I checked a couple of places, i.e. compaction, migration, vmscan. All these places free the page (i.e. old page is migrated to new page successfully) via put_page() without putting back to lru. So, it sounds free page in putback_lru_page() may never happen. We could add this, but it sounds khugepaged would be the only path that may free page in putback_lru_page(). So, it sounds less confusing to follow other users IMHO. >> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> Cc: Hugh Dickins <hughd@google.com> >> Cc: Andrea Arcangeli <aarcange@redhat.com> >> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >> --- >> v2: Check mapcount and skip putback lru if the last mapcount is gone >> >> mm/khugepaged.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >> index 0c8d30b..1fdd677 100644 >> --- a/mm/khugepaged.c >> +++ b/mm/khugepaged.c >> @@ -559,10 +559,18 @@ void __khugepaged_exit(struct mm_struct *mm) >> static void release_pte_page(struct page *page) >> { >> mod_node_page_state(page_pgdat(page), >> - NR_ISOLATED_ANON + page_is_file_lru(page), >> - -compound_nr(page)); >> - unlock_page(page); >> - putback_lru_page(page); >> + NR_ISOLATED_ANON + page_is_file_lru(page), -compound_nr(page)); >> + >> + if (total_mapcount(page)) { >> + unlock_page(page); >> + putback_lru_page(page); >> + } else { >> + ClearPageActive(page); >> + ClearPageUnevictable(page); >> + unlock_page(page); >> + /* Drop refcount from isolate */ >> + put_page(page); >> + } >> } >> >> static void release_pte_pages(pte_t *pte, pte_t *_pte, >> @@ -771,8 +779,6 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, >> } else { >> src_page = pte_page(pteval); >> copy_user_highpage(page, src_page, address, vma); >> - if (!PageCompound(src_page)) >> - release_pte_page(src_page); >> /* >> * ptl mostly unnecessary, but preempt has to >> * be disabled to update the per-cpu stats >> @@ -786,6 +792,8 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, >> pte_clear(vma->vm_mm, address, _pte); >> page_remove_rmap(src_page, false); >> spin_unlock(ptl); >> + if (!PageCompound(src_page)) >> + release_pte_page(src_page); >> free_page_and_swap_cache(src_page); >> } >> } >> -- >> 1.8.3.1 >> >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-01 17:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-30 20:41 [v2 linux-next PATCH 1/2] mm: khugepaged: add exceed_max_ptes_* helpers Yang Shi 2020-04-30 20:41 ` [v2 linux-next PATCH 2/2] mm: khugepaged: don't have to put being freed page back to lru Yang Shi 2020-05-01 7:04 ` Kirill A. Shutemov 2020-05-01 17:00 ` Yang Shi
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).