diff mbox series

[v2,2/7] fs/proc/page: use a folio in stable_page_flags()

Message ID 20231110033324.2455523-3-wangkefeng.wang@huawei.com
State New
Headers
Series [v2,1/7] fs/proc/page: remove unneeded PageTail && PageSlab check |

Commit Message

Kefeng Wang Nov. 10, 2023, 3:33 a.m. UTC 
  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(-)

Comments

Matthew Wilcox Nov. 10, 2023, 6:15 p.m. UTC | #1 
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.
Gregory Price Nov. 10, 2023, 9:50 p.m. UTC | #2 
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(-)
>
Kefeng Wang Nov. 11, 2023, 3:21 a.m. UTC | #3 
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 mbox series

Patch

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);