mm/debug: PageAnon() is true for PageKsm() pages
diff mbox series

Message ID 20191113000651.20677-1-rcampbell@nvidia.com
State New
Headers show
Series
  • mm/debug: PageAnon() is true for PageKsm() pages
Related show

Commit Message

Ralph Campbell Nov. 13, 2019, 12:06 a.m. UTC
PageAnon() and PageKsm() use the low two bits of the page->mapping pointer
to indicate the page type. PageAnon() only checks the LSB while PageKsm()
checks the least significant 2 bits are equal to 3. Therefore, PageAnon()
is true for KSM pages.
__dump_page() incorrectly will never print "ksm" because it checks
PageAnon() first. Fix this by checking PageKsm() first.

Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
---
 mm/debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Michal Hocko Nov. 14, 2019, 1:59 p.m. UTC | #1
On Tue 12-11-19 16:06:51, Ralph Campbell wrote:
> PageAnon() and PageKsm() use the low two bits of the page->mapping pointer
> to indicate the page type. PageAnon() only checks the LSB while PageKsm()
> checks the least significant 2 bits are equal to 3. Therefore, PageAnon()
> is true for KSM pages.
> __dump_page() incorrectly will never print "ksm" because it checks
> PageAnon() first. Fix this by checking PageKsm() first.

Thanks for spotting this

Fixes: 1c6fb1d89e73 ("mm: print more information about mapping in __dump_page")

> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/debug.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 772d4cf0691f..0461df1207cb 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -77,10 +77,10 @@ void __dump_page(struct page *page, const char *reason)
>  		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
>  			page, page_ref_count(page), mapcount,
>  			page->mapping, page_to_pgoff(page));
> -	if (PageAnon(page))
> -		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
> -	else if (PageKsm(page))
> +	if (PageKsm(page))
>  		pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
> +	else if (PageAnon(page))
> +		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
>  	else if (mapping) {
>  		if (mapping->host && mapping->host->i_dentry.first) {
>  			struct dentry *dentry;
> -- 
> 2.20.1
Li Xinhai Nov. 15, 2019, 2:32 a.m. UTC | #2
On 2019-11-14 at 21:59 Michal Hocko wrote:
>On Tue 12-11-19 16:06:51, Ralph Campbell wrote:
>> PageAnon() and PageKsm() use the low two bits of the page->mapping pointer
>> to indicate the page type. PageAnon() only checks the LSB while PageKsm()
>> checks the least significant 2 bits are equal to 3. Therefore, PageAnon()
>> is true for KSM pages.
>> __dump_page() incorrectly will never print "ksm" because it checks
>> PageAnon() first. Fix this by checking PageKsm() first.
>
>Thanks for spotting this
>
>Fixes: 1c6fb1d89e73 ("mm: print more information about mapping in __dump_page")
>
>> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
>
>Acked-by: Michal Hocko <mhocko@suse.com>
>
The four values from those two lowest bits have different meaning, so PageKsm is 
true does not mean we can consider PageAnon is also true or PageMovable is also 
true.
Improve PageAnon() would be better, so users of those APIs do not need to 
remember this implict checking sequence at other places.

- Xinhai
Li Xinhai Nov. 15, 2019, 5:47 a.m. UTC | #3
On 2019-11-15 at 10:32 lixinhai.lxh wrote:
>On 2019-11-14 at 21:59 Michal Hocko wrote:
>>On Tue 12-11-19 16:06:51, Ralph Campbell wrote:
>>> PageAnon() and PageKsm() use the low two bits of the page->mapping pointer
>>> to indicate the page type. PageAnon() only checks the LSB while PageKsm()
>>> checks the least significant 2 bits are equal to 3. Therefore, PageAnon()
>>> is true for KSM pages.
>>> __dump_page() incorrectly will never print "ksm" because it checks
>>> PageAnon() first. Fix this by checking PageKsm() first.
>>
>>Thanks for spotting this
>>
>>Fixes: 1c6fb1d89e73 ("mm: print more information about mapping in __dump_page")
>>
>>> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
>>
>>Acked-by: Michal Hocko <mhocko@suse.com>
>>
>The four values from those two lowest bits have different meaning, so PageKsm is
>true does not mean we can consider PageAnon is also true or PageMovable is also
>true.
>Improve PageAnon() would be better, so users of those APIs do not need to
>remember this implict checking sequence at other places.
>
>- Xinhai 

Looked the reference to PageAnon() again, there are many use of PageAnon() to 
cover cases for both Anon without KSM and Anon with KSM. Change PageAnon() 
implementation require change those places, so keep this patch for current issue is 
enough.

- Xinhai

Patch
diff mbox series

diff --git a/mm/debug.c b/mm/debug.c
index 772d4cf0691f..0461df1207cb 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -77,10 +77,10 @@  void __dump_page(struct page *page, const char *reason)
 		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
 			page, page_ref_count(page), mapcount,
 			page->mapping, page_to_pgoff(page));
-	if (PageAnon(page))
-		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
-	else if (PageKsm(page))
+	if (PageKsm(page))
 		pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
+	else if (PageAnon(page))
+		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
 	else if (mapping) {
 		if (mapping->host && mapping->host->i_dentry.first) {
 			struct dentry *dentry;