| Message ID | 20231110033324.2455523-3-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: remove page idle and young wrapper | expand |
On Fri, Nov 10, 2023 at 11:33:19AM +0800, Kefeng Wang wrote:
> Replace nine compound_head() calls with one page_folio().
But that's not all it does. Honestly, when you write these kind of
things, I wonder if you understand what you're doing.
After this patch, if we report on a tail page, we set (some of) the
flags according to how its head page is set. Before, we would have not
reported on it at all.
This was THE ENTIRE POINT of Greg's patch. And why his patch made sense
and yours is nonsense. Andrew, please drop this patch series.
On Fri, Nov 10, 2023 at 11:33:19AM +0800, Kefeng Wang wrote: > Replace nine compound_head() calls with one page_folio(). > Sorry to echo Matthew, but this commit message is extremely insufficient and just outright wrong. Single pass through, here's the real change list: 1) changes PageFLAG() calls to folio_test_FLAG calls 2) changes compound_head() flag checks to folio_test_FLAG checks 3) change page count to folio ref count -- without even looking... is this even correct? Need more explanation here. Is page count === folio ref count? So there are really 3 changes in this patch set that should be broken out separately, even if they all depend on folio flags, because they need separate explanation and validation for correctness. > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > fs/proc/page.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) >
On 2023/11/11 2:15, Matthew Wilcox wrote: > On Fri, Nov 10, 2023 at 11:33:19AM +0800, Kefeng Wang wrote: >> Replace nine compound_head() calls with one page_folio(). > > But that's not all it does. Honestly, when you write these kind of > things, I wonder if you understand what you're doing. Oh, yes, I total wrong for some change, the kpagelfags should report per-page. > > After this patch, if we report on a tail page, we set (some of) the > flags according to how its head page is set. Before, we would have not > reported on it at all. I should force on the following specific flags in this patch 1) PageKsm - KSM only normal anon page, also it is wrapper of folio_test_ksm() 2) struct page *head = compound_head(page); PageLRU(head) PageAnon(head) - they expect to check the head page flags 3) page_count(page) == 0 && is_free_buddy_page(page) - this is to identify free buddy page, also page_count is a wrapper of folio_ref_count 4) page_is_idle - a wrapper of folio_test_idle Matthew and Gregory, correct me if I am still misunderstanding, man thanks. > > This was THE ENTIRE POINT of Greg's patch. And why his patch made sense > and yours is nonsense. Andrew, please drop this patch series. >
diff --git a/fs/proc/page.c b/fs/proc/page.c index 466efb0dadf7..dcef02471f91 100644 --- a/fs/proc/page.c +++ b/fs/proc/page.c @@ -109,6 +109,7 @@ static inline u64 kpf_copy_bit(u64 kflags, int ubit, int kbit) u64 stable_page_flags(struct page *page) { + struct folio *folio; u64 k; u64 u; @@ -119,6 +120,7 @@ u64 stable_page_flags(struct page *page) if (!page) return 1 << KPF_NOPAGE; + folio = page_folio(page); k = page->flags; u = 0; @@ -128,11 +130,11 @@ u64 stable_page_flags(struct page *page) * Note that page->_mapcount is overloaded in SLAB, so the * simple test in page_mapped() is not enough. */ - if (!PageSlab(page) && page_mapped(page)) + if (!folio_test_slab(folio) && folio_mapped(folio)) u |= 1 << KPF_MMAP; - if (PageAnon(page)) + if (folio_test_anon(folio)) u |= 1 << KPF_ANON; - if (PageKsm(page)) + if (folio_test_ksm(folio)) u |= 1 << KPF_KSM; /* @@ -152,11 +154,9 @@ u64 stable_page_flags(struct page *page) * to make sure a given page is a thp, not a non-huge compound page. */ else if (PageTransCompound(page)) { - struct page *head = compound_head(page); - - if (PageLRU(head) || PageAnon(head)) + if (folio_test_lru(folio) || folio_test_anon(folio)) u |= 1 << KPF_THP; - else if (is_huge_zero_page(head)) { + else if (is_huge_zero_page(&folio->page)) { u |= 1 << KPF_ZERO_PAGE; u |= 1 << KPF_THP; } @@ -170,7 +170,7 @@ u64 stable_page_flags(struct page *page) */ if (PageBuddy(page)) u |= 1 << KPF_BUDDY; - else if (page_count(page) == 0 && is_free_buddy_page(page)) + else if (folio_ref_count(folio) == 0 && is_free_buddy_page(page)) u |= 1 << KPF_BUDDY; if (PageOffline(page)) @@ -178,7 +178,7 @@ u64 stable_page_flags(struct page *page) if (PageTable(page)) u |= 1 << KPF_PGTABLE; - if (page_is_idle(page)) + if (folio_test_idle(folio)) u |= 1 << KPF_IDLE; u |= kpf_copy_bit(k, KPF_LOCKED, PG_locked); @@ -194,7 +194,7 @@ u64 stable_page_flags(struct page *page) u |= kpf_copy_bit(k, KPF_ACTIVE, PG_active); u |= kpf_copy_bit(k, KPF_RECLAIM, PG_reclaim); - if (PageSwapCache(page)) + if (folio_test_swapcache(folio)) u |= 1 << KPF_SWAPCACHE; u |= kpf_copy_bit(k, KPF_SWAPBACKED, PG_swapbacked);
Replace nine compound_head() calls with one page_folio(). Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- fs/proc/page.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)