diff mbox series

[v2,3/7] fs/proc/page: respect folio head-page flag placement

Message ID 20231110033324.2455523-4-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 
  kpageflags reads page-flags directly from the page, even when the
respective flag is only updated on the headpage of a folio.

Since most flags are stored in head flags, make k = folio->flags,
and add new p = page->flags used for per-page flags.

Originally-from: Gregory Price <gregory.price@memverge.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/proc/page.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Matthew Wilcox Nov. 10, 2023, 6:19 p.m. UTC | #1 
On Fri, Nov 10, 2023 at 11:33:20AM +0800, Kefeng Wang wrote:
> kpageflags reads page-flags directly from the page, even when the
> respective flag is only updated on the headpage of a folio.
> 
> Since most flags are stored in head flags, make k = folio->flags,
> and add new p = page->flags used for per-page flags.

You'd do better to steal Greg's commit message.

> Originally-from: Gregory Price <gregory.price@memverge.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

> @@ -202,7 +202,7 @@ u64 stable_page_flags(struct page *page)
>  	u |= kpf_copy_bit(k, KPF_MLOCKED,	PG_mlocked);
>  
>  #ifdef CONFIG_MEMORY_FAILURE
> -	u |= kpf_copy_bit(k, KPF_HWPOISON,	PG_hwpoison);
> +	u |= kpf_copy_bit(p, KPF_HWPOISON,	PG_hwpoison);

This is correct.

> @@ -211,13 +211,13 @@ u64 stable_page_flags(struct page *page)
>  
>  	u |= kpf_copy_bit(k, KPF_RESERVED,	PG_reserved);
>  	u |= kpf_copy_bit(k, KPF_MAPPEDTODISK,	PG_mappedtodisk);
> -	u |= kpf_copy_bit(k, KPF_PRIVATE,	PG_private);
> -	u |= kpf_copy_bit(k, KPF_PRIVATE_2,	PG_private_2);
> -	u |= kpf_copy_bit(k, KPF_OWNER_PRIVATE,	PG_owner_priv_1);
> +	u |= kpf_copy_bit(p, KPF_PRIVATE,	PG_private);
> +	u |= kpf_copy_bit(p, KPF_PRIVATE_2,	PG_private_2);
> +	u |= kpf_copy_bit(p, KPF_OWNER_PRIVATE,	PG_owner_priv_1);

This is not.  PG_private is not, I believe, set on tail pages.
Ditto the other two.  If you know differently ... ?

>  #ifdef CONFIG_ARCH_USES_PG_ARCH_X
> -	u |= kpf_copy_bit(k, KPF_ARCH_2,	PG_arch_2);
> -	u |= kpf_copy_bit(k, KPF_ARCH_3,	PG_arch_3);
> +	u |= kpf_copy_bit(p, KPF_ARCH_2,	PG_arch_2);
> +	u |= kpf_copy_bit(p, KPF_ARCH_3,	PG_arch_3);
>  #endif

I also don't think this is correct, but there are many uses of
PG_arch* and I may have missed something.
Kefeng Wang Nov. 11, 2023, 9:49 a.m. UTC | #2 
On 2023/11/11 2:19, Matthew Wilcox wrote:
> On Fri, Nov 10, 2023 at 11:33:20AM +0800, Kefeng Wang wrote:
>> kpageflags reads page-flags directly from the page, even when the
>> respective flag is only updated on the headpage of a folio.
>>
>> Since most flags are stored in head flags, make k = folio->flags,
>> and add new p = page->flags used for per-page flags.
> 
> You'd do better to steal Greg's commit message.

Sure

> 
>> Originally-from: Gregory Price <gregory.price@memverge.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> 
>> @@ -202,7 +202,7 @@ u64 stable_page_flags(struct page *page)
>>   	u |= kpf_copy_bit(k, KPF_MLOCKED,	PG_mlocked);
>>   
>>   #ifdef CONFIG_MEMORY_FAILURE
>> -	u |= kpf_copy_bit(k, KPF_HWPOISON,	PG_hwpoison);
>> +	u |= kpf_copy_bit(p, KPF_HWPOISON,	PG_hwpoison);
> 
> This is correct.
> 
>> @@ -211,13 +211,13 @@ u64 stable_page_flags(struct page *page)
>>   
>>   	u |= kpf_copy_bit(k, KPF_RESERVED,	PG_reserved);
>>   	u |= kpf_copy_bit(k, KPF_MAPPEDTODISK,	PG_mappedtodisk);
>> -	u |= kpf_copy_bit(k, KPF_PRIVATE,	PG_private);
>> -	u |= kpf_copy_bit(k, KPF_PRIVATE_2,	PG_private_2);
>> -	u |= kpf_copy_bit(k, KPF_OWNER_PRIVATE,	PG_owner_priv_1);
>> +	u |= kpf_copy_bit(p, KPF_PRIVATE,	PG_private);
>> +	u |= kpf_copy_bit(p, KPF_PRIVATE_2,	PG_private_2);
>> +	u |= kpf_copy_bit(p, KPF_OWNER_PRIVATE,	PG_owner_priv_1);
> 
> This is not.  PG_private is not, I believe, set on tail pages.
> Ditto the other two.  If you know differently ... ?

k = folio->flags, p = page->flags
as PG_private/private_2/owner_priv_1 use PF_ANY page policy,
so they should per-page check, confused...

See checked page flags,

PG_error        PF_NO_TAIL
PG_dirty        PF_HEAD
PG_uptodate     PF_NO_TAIL
PG_writeback    PF_NO_TAIL
PG_lru          PF_HEAD
PG_referenced 	PF_HEAD
PG_active       PF_HEAD
PG_reclaim      PF_NO_TAIL
PG_swapbacked   PF_NO_TAIL
PG_unevictable  PF_HEAD
PG_mlocked      PF_NO_TAIL
PG_hwpoison     PF_ANY
PG_uncached     PF_NO_COMPOUND	
PG_reserved     PF_NO_COMPOUND
PG_mappedtodisk PF_NO_TAIL
PG_private      PF_ANY
PG_private_2    PF_ANY
PG_owner_priv_1 PF_ANY

above part has 4 types,

1) PF_HEAD        - should use k

2) PF_ANY         - should use p

3) PF_NO_TAIL
    - PageXXX will check head page flags, so suppose to use k,
      but here use bit check, I wonder it should use p?

4) PF_NO_COMPOUND
   - should per-page check
> 
>>   #ifdef CONFIG_ARCH_USES_PG_ARCH_X
>> -	u |= kpf_copy_bit(k, KPF_ARCH_2,	PG_arch_2);
>> -	u |= kpf_copy_bit(k, KPF_ARCH_3,	PG_arch_3);
>> +	u |= kpf_copy_bit(p, KPF_ARCH_2,	PG_arch_2);
>> +	u |= kpf_copy_bit(p, KPF_ARCH_3,	PG_arch_3);
>>   #endif
> 
> I also don't think this is correct, but there are many uses of
> PG_arch* and I may have missed something.
> 

And 3 arch page flags,

PG_arch_1
  - PG_dcache_clean, from achetlb.rst, I think it is per-folio, use k
	
PG_arch_2
  - only arm64 mte, PG_mte_tagged
PG_arch_3
  - only arm64 mte, PG_mte_lock

the two PG_arch only used by arm64 mte, they are per-page flag, use p

Correct me if I am wrong, thanks.
diff mbox series

Patch

diff --git a/fs/proc/page.c b/fs/proc/page.c
index dcef02471f91..553a7c921cb4 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -110,8 +110,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;
+	u64 k, p, u;
 
 	/*
 	 * pseudo flag: KPF_NOPAGE
@@ -121,7 +120,8 @@  u64 stable_page_flags(struct page *page)
 		return 1 << KPF_NOPAGE;
 
 	folio = page_folio(page);
-	k = page->flags;
+	k = folio->flags;
+	p = page->flags;
 	u = 0;
 
 	/*
@@ -202,7 +202,7 @@  u64 stable_page_flags(struct page *page)
 	u |= kpf_copy_bit(k, KPF_MLOCKED,	PG_mlocked);
 
 #ifdef CONFIG_MEMORY_FAILURE
-	u |= kpf_copy_bit(k, KPF_HWPOISON,	PG_hwpoison);
+	u |= kpf_copy_bit(p, KPF_HWPOISON,	PG_hwpoison);
 #endif
 
 #ifdef CONFIG_ARCH_USES_PG_UNCACHED
@@ -211,13 +211,13 @@  u64 stable_page_flags(struct page *page)
 
 	u |= kpf_copy_bit(k, KPF_RESERVED,	PG_reserved);
 	u |= kpf_copy_bit(k, KPF_MAPPEDTODISK,	PG_mappedtodisk);
-	u |= kpf_copy_bit(k, KPF_PRIVATE,	PG_private);
-	u |= kpf_copy_bit(k, KPF_PRIVATE_2,	PG_private_2);
-	u |= kpf_copy_bit(k, KPF_OWNER_PRIVATE,	PG_owner_priv_1);
+	u |= kpf_copy_bit(p, KPF_PRIVATE,	PG_private);
+	u |= kpf_copy_bit(p, KPF_PRIVATE_2,	PG_private_2);
+	u |= kpf_copy_bit(p, KPF_OWNER_PRIVATE,	PG_owner_priv_1);
 	u |= kpf_copy_bit(k, KPF_ARCH,		PG_arch_1);
 #ifdef CONFIG_ARCH_USES_PG_ARCH_X
-	u |= kpf_copy_bit(k, KPF_ARCH_2,	PG_arch_2);
-	u |= kpf_copy_bit(k, KPF_ARCH_3,	PG_arch_3);
+	u |= kpf_copy_bit(p, KPF_ARCH_2,	PG_arch_2);
+	u |= kpf_copy_bit(p, KPF_ARCH_3,	PG_arch_3);
 #endif
 
 	return u;