[PATCH] trace/events/page_ref: add page info to page_ref trace event

[PATCH] trace/events/page_ref: add page info to page_ref trace event

[Kassey Li]

This followed
commit 53d884a6675b ("mm, tracing: unify PFN format strings")
to add page info.

In many kernel code we are talking with page other than pfn,
here we added page algin with pfn.

Signed-off-by: Kassey Li <quic_yingangl@quicinc.com>
---
 include/trace/events/page_ref.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h
index fe33a255b7d0..76df13b2a5b3 100644
--- a/include/trace/events/page_ref.h
+++ b/include/trace/events/page_ref.h
@@ -18,6 +18,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
 
 	TP_STRUCT__entry(
 		__field(unsigned long, pfn)
+		__field(const struct page *, page)
 		__field(unsigned long, flags)
 		__field(int, count)
 		__field(int, mapcount)
@@ -28,6 +29,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
 
 	TP_fast_assign(
 		__entry->pfn = page_to_pfn(page);
+		__entry->page = page;
 		__entry->flags = page->flags;
 		__entry->count = page_ref_count(page);
 		__entry->mapcount = atomic_read(&page->_mapcount);
@@ -36,8 +38,9 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
 		__entry->val = v;
 	),
 
-	TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d",
+	TP_printk("pfn=0x%lx page=%p flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d",
 		__entry->pfn,
+		__entry->page,
 		show_page_flags(__entry->flags & PAGEFLAGS_MASK),
 		__entry->count,
 		__entry->mapcount, __entry->mapping, __entry->mt,
@@ -66,6 +69,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
 
 	TP_STRUCT__entry(
 		__field(unsigned long, pfn)
+		__field(const struct page *, page)
 		__field(unsigned long, flags)
 		__field(int, count)
 		__field(int, mapcount)
@@ -77,6 +81,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
 
 	TP_fast_assign(
 		__entry->pfn = page_to_pfn(page);
+		__entry->page = page;
 		__entry->flags = page->flags;
 		__entry->count = page_ref_count(page);
 		__entry->mapcount = atomic_read(&page->_mapcount);
@@ -86,8 +91,9 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
 		__entry->ret = ret;
 	),
 
-	TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d",
+	TP_printk("pfn=0x%lx page=%p flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d",
 		__entry->pfn,
+		__entry->page,
 		show_page_flags(__entry->flags & PAGEFLAGS_MASK),
 		__entry->count,
 		__entry->mapcount, __entry->mapping, __entry->mt,
-- 
2.25.1

Re: [PATCH] trace/events/page_ref: add page info to page_ref trace event

[Matthew Wilcox]

On Thu, Oct 31, 2024 at 10:42:22AM +0800, Kassey Li wrote:
> This followed
> commit 53d884a6675b ("mm, tracing: unify PFN format strings")
> to add page info.
> 
> In many kernel code we are talking with page other than pfn,
> here we added page algin with pfn.

It's generally a bad idea to print pointers to kernel memory, so no I
don't like this patch.

Re: [PATCH] trace/events/page_ref: add page info to page_ref trace event

[Vlastimil Babka (SUSE)]

On 10/31/24 03:42, Kassey Li wrote:
> This followed
> commit 53d884a6675b ("mm, tracing: unify PFN format strings")
> to add page info.
> 
> In many kernel code we are talking with page other than pfn,
> here we added page algin with pfn.

How exactly would this help you, what are you doing with the trace?

> Signed-off-by: Kassey Li <quic_yingangl@quicinc.com>
> ---
>  include/trace/events/page_ref.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h
> index fe33a255b7d0..76df13b2a5b3 100644
> --- a/include/trace/events/page_ref.h
> +++ b/include/trace/events/page_ref.h
> @@ -18,6 +18,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
>  
>  	TP_STRUCT__entry(
>  		__field(unsigned long, pfn)
> +		__field(const struct page *, page)
>  		__field(unsigned long, flags)
>  		__field(int, count)
>  		__field(int, mapcount)
> @@ -28,6 +29,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
>  
>  	TP_fast_assign(
>  		__entry->pfn = page_to_pfn(page);

pfn is derived from the page, but not subject to KASLR, so in that sense is
better.
If you need to correlate the trace with some other data you obtained that
does contains page pointers, you will have to postprocess the trace with a
pfn_to_page() step, which is rather simple (but you'll need to obtain and
supply the randomized base) or have that other data source give you pfn too.

The tracepoints should not reveal the randomized base as trivially as they
would do after this patch.

> +		__entry->page = page;
>  		__entry->flags = page->flags;
>  		__entry->count = page_ref_count(page);
>  		__entry->mapcount = atomic_read(&page->_mapcount);
> @@ -36,8 +38,9 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
>  		__entry->val = v;
>  	),
>  
> -	TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d",
> +	TP_printk("pfn=0x%lx page=%p flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d",
>  		__entry->pfn,
> +		__entry->page,
>  		show_page_flags(__entry->flags & PAGEFLAGS_MASK),
>  		__entry->count,
>  		__entry->mapcount, __entry->mapping, __entry->mt,
> @@ -66,6 +69,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
>  
>  	TP_STRUCT__entry(
>  		__field(unsigned long, pfn)
> +		__field(const struct page *, page)
>  		__field(unsigned long, flags)
>  		__field(int, count)
>  		__field(int, mapcount)
> @@ -77,6 +81,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
>  
>  	TP_fast_assign(
>  		__entry->pfn = page_to_pfn(page);
> +		__entry->page = page;
>  		__entry->flags = page->flags;
>  		__entry->count = page_ref_count(page);
>  		__entry->mapcount = atomic_read(&page->_mapcount);
> @@ -86,8 +91,9 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
>  		__entry->ret = ret;
>  	),
>  
> -	TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d",
> +	TP_printk("pfn=0x%lx page=%p flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d",
>  		__entry->pfn,
> +		__entry->page,
>  		show_page_flags(__entry->flags & PAGEFLAGS_MASK),
>  		__entry->count,
>  		__entry->mapcount, __entry->mapping, __entry->mt,

Re: [PATCH] trace/events/page_ref: add page info to page_ref trace event

[Kassey Li quic]

On 2024/10/31 15:24, Vlastimil Babka (SUSE) wrote:
> On 10/31/24 03:42, Kassey Li wrote:
>> This followed
>> commit 53d884a6675b ("mm, tracing: unify PFN format strings")
>> to add page info.
>>
>> In many kernel code we are talking with page other than pfn,
>> here we added page algin with pfn.
> How exactly would this help you, what are you doing with the trace?

we met some problem where filesystem held the page.

we can show the page pointer other than pfn in filesystem code.

>
>> Signed-off-by: Kassey Li <quic_yingangl@quicinc.com>
>> ---
>>   include/trace/events/page_ref.h | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/trace/events/page_ref.h b/include/trace/events/page_ref.h
>> index fe33a255b7d0..76df13b2a5b3 100644
>> --- a/include/trace/events/page_ref.h
>> +++ b/include/trace/events/page_ref.h
>> @@ -18,6 +18,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
>>   
>>   	TP_STRUCT__entry(
>>   		__field(unsigned long, pfn)
>> +		__field(const struct page *, page)
>>   		__field(unsigned long, flags)
>>   		__field(int, count)
>>   		__field(int, mapcount)
>> @@ -28,6 +29,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
>>   
>>   	TP_fast_assign(
>>   		__entry->pfn = page_to_pfn(page);
> pfn is derived from the page, but not subject to KASLR, so in that sense is
> better.
> If you need to correlate the trace with some other data you obtained that
> does contains page pointers, you will have to postprocess the trace with a
> pfn_to_page() step, which is rather simple (but you'll need to obtain and
> supply the randomized base) or have that other data source give you pfn too.
>
> The tracepoints should not reveal the randomized base as trivially as they
> would do after this patch.

ok, thanks for the suggest, I think generally we can try to handle the 
debug code in our local only.

>
>> +		__entry->page = page;
>>   		__entry->flags = page->flags;
>>   		__entry->count = page_ref_count(page);
>>   		__entry->mapcount = atomic_read(&page->_mapcount);
>> @@ -36,8 +38,9 @@ DECLARE_EVENT_CLASS(page_ref_mod_template,
>>   		__entry->val = v;
>>   	),
>>   
>> -	TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d",
>> +	TP_printk("pfn=0x%lx page=%p flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d",
>>   		__entry->pfn,
>> +		__entry->page,
>>   		show_page_flags(__entry->flags & PAGEFLAGS_MASK),
>>   		__entry->count,
>>   		__entry->mapcount, __entry->mapping, __entry->mt,
>> @@ -66,6 +69,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
>>   
>>   	TP_STRUCT__entry(
>>   		__field(unsigned long, pfn)
>> +		__field(const struct page *, page)
>>   		__field(unsigned long, flags)
>>   		__field(int, count)
>>   		__field(int, mapcount)
>> @@ -77,6 +81,7 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
>>   
>>   	TP_fast_assign(
>>   		__entry->pfn = page_to_pfn(page);
>> +		__entry->page = page;
>>   		__entry->flags = page->flags;
>>   		__entry->count = page_ref_count(page);
>>   		__entry->mapcount = atomic_read(&page->_mapcount);
>> @@ -86,8 +91,9 @@ DECLARE_EVENT_CLASS(page_ref_mod_and_test_template,
>>   		__entry->ret = ret;
>>   	),
>>   
>> -	TP_printk("pfn=0x%lx flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d",
>> +	TP_printk("pfn=0x%lx page=%p flags=%s count=%d mapcount=%d mapping=%p mt=%d val=%d ret=%d",
>>   		__entry->pfn,
>> +		__entry->page,
>>   		show_page_flags(__entry->flags & PAGEFLAGS_MASK),
>>   		__entry->count,
>>   		__entry->mapcount, __entry->mapping, __entry->mt,

Re: [PATCH] trace/events/page_ref: add page info to page_ref trace event

[Matthew Wilcox]

On Mon, Nov 04, 2024 at 03:35:37PM +0800, Kassey Li quic wrote:
> > > In many kernel code we are talking with page other than pfn,
> > > here we added page algin with pfn.
> > How exactly would this help you, what are you doing with the trace?
> 
> we met some problem where filesystem held the page.
> 
> we can show the page pointer other than pfn in filesystem code.

Don't do that.  Change the filesystem code to show the pfn, and send
*that* patch instead.

Re: [PATCH] trace/events/page_ref: add page info to page_ref trace event

[Kassey Li quic]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

thanks for the suggest from Matthew and Vlastimil.

On 2024/11/4 21:39, Matthew Wilcox wrote:
> On Mon, Nov 04, 2024 at 03:35:37PM +0800, Kassey Li quic wrote:
>>>> In many kernel code we are talking with page other than pfn,
>>>> here we added page algin with pfn.
>>> How exactly would this help you, what are you doing with the trace?
>> we met some problem where filesystem held the page.
>>
>> we can show the page pointer other than pfn in filesystem code.
> Don't do that.  Change the filesystem code to show the pfn, and send
> *that* patch instead.
thanks for the suggest from Matthew and Vlastimil.

[-- Attachment #2: Type: text/html, Size: 36929 bytes --]

Re: [PATCH] trace/events/page_ref: add page info to page_ref trace event

[Kassey Li quic]

On 2024/11/4 21:39, Matthew Wilcox wrote:
> On Mon, Nov 04, 2024 at 03:35:37PM +0800, Kassey Li quic wrote:
>>>> In many kernel code we are talking with page other than pfn,
>>>> here we added page algin with pfn.
>>> How exactly would this help you, what are you doing with the trace?
>>
>> we met some problem where filesystem held the page.
>>
>> we can show the page pointer other than pfn in filesystem code.
> 
> Don't do that.  Change the filesystem code to show the pfn, and send
> *that* patch instead.

thanks for the suggest from Matthew and Vlastimil.  On 2024/11/4 21:39, 
Matthew Wilcox wrote: