[v2] mm/page_owner: Record timestamp and pid

Commit Message

Georgi Djakov Dec. 9, 2020, 12:51 p.m. UTC

From: Liam Mark <[e-mail]>

Collect the time for each allocation recorded in page owner so that
allocation "surges" can be measured.

Record the pid for each allocation recorded in page owner so that
the source of allocation "surges" can be better identified.

The above is very useful when doing memory analysis. On a crash for
example, we can get this information from kdump (or ramdump) and parse
it to figure out memory allocation problems.

Please note that on x86_64 this increases the size of struct page_owner
from 16 bytes to 32.

Signed-off-by: Liam Mark <[e-mail]>
Signed-off-by: Georgi Djakov <[e-mail]>
---

v2:
- Improve the commit message (Andrew and Vlastimil)
- Update page_owner.rst with more recent object size information (Andrew)
- Use pid_t for the pid (Andrew)
- Print the info also in __dump_page_owner() (Vlastimil)

v1: https://lore.kernel.org/r/[e-mail]/


 Documentation/vm/page_owner.rst | 12 ++++++------
 mm/page_owner.c                 | 17 +++++++++++++----
 2 files changed, 19 insertions(+), 10 deletions(-)

Comments

Vlastimil Babka Dec. 9, 2020, 4:38 p.m. UTC | #1

On 12/9/20 1:51 PM, Georgi Djakov wrote:
> From: Liam Mark <[e-mail]>
> 
> Collect the time for each allocation recorded in page owner so that
> allocation "surges" can be measured.
> 
> Record the pid for each allocation recorded in page owner so that
> the source of allocation "surges" can be better identified.
> 
> The above is very useful when doing memory analysis. On a crash for
> example, we can get this information from kdump (or ramdump) and parse
> it to figure out memory allocation problems.

Yes, I can imagine this to be useful.

> Please note that on x86_64 this increases the size of struct page_owner
> from 16 bytes to 32.

That's the tradeoff, but it's not a functionality intended for production, so
unless somebody says they need to enable page_owner for debugging and this
increase prevents them from fitting into available memory, let's not complicate
things with making this optional.

> Signed-off-by: Liam Mark <[e-mail]>
> Signed-off-by: Georgi Djakov <[e-mail]>

Acked-by: Vlastimil Babka <[e-mail]>

> ---
> 
> v2:
> - Improve the commit message (Andrew and Vlastimil)
> - Update page_owner.rst with more recent object size information (Andrew)
> - Use pid_t for the pid (Andrew)
> - Print the info also in __dump_page_owner() (Vlastimil)
> 
> v1: https://lore.kernel.org/r/[e-mail]/
> 
> 
>  Documentation/vm/page_owner.rst | 12 ++++++------
>  mm/page_owner.c                 | 17 +++++++++++++----
>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/vm/page_owner.rst b/Documentation/vm/page_owner.rst
> index 02deac76673f..cf7c0c361621 100644
> --- a/Documentation/vm/page_owner.rst
> +++ b/Documentation/vm/page_owner.rst
> @@ -41,17 +41,17 @@ size change due to this facility.
>  - Without page owner::
>  
>     text    data     bss     dec     hex filename
> -   40662   1493     644   42799    a72f mm/page_alloc.o
> +  48392    2333     644   51369    c8a9 mm/page_alloc.o
>  
>  - With page owner::
>  
>     text    data     bss     dec     hex filename
> -   40892   1493     644   43029    a815 mm/page_alloc.o
> -   1427      24       8    1459     5b3 mm/page_ext.o
> -   2722      50       0    2772     ad4 mm/page_owner.o
> +  48800    2445     644   51889    cab1 mm/page_alloc.o
> +   6574     108      29    6711    1a37 mm/page_owner.o
> +   1025       8       8    1041     411 mm/page_ext.o
>  
> -Although, roughly, 4 KB code is added in total, page_alloc.o increase by
> -230 bytes and only half of it is in hotpath. Building the kernel with
> +Although, roughly, 8 KB code is added in total, page_alloc.o increase by
> +520 bytes and less than half of it is in hotpath. Building the kernel with
>  page owner and turning it on if needed would be great option to debug
>  kernel memory problem.
>  
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index b735a8eafcdb..af464bb7fbe7 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -10,6 +10,7 @@
>  #include <linux/migrate.h>
>  #include <linux/stackdepot.h>
>  #include <linux/seq_file.h>
> +#include <linux/sched/clock.h>
>  
>  #include "internal.h"
>  
> @@ -25,6 +26,8 @@ struct page_owner {
>  	gfp_t gfp_mask;
>  	depot_stack_handle_t handle;
>  	depot_stack_handle_t free_handle;
> +	u64 ts_nsec;
> +	pid_t pid;
>  };
>  
>  static bool page_owner_enabled = false;
> @@ -172,6 +175,8 @@ static inline void __set_page_owner_handle(struct page *page,
>  		page_owner->order = order;
>  		page_owner->gfp_mask = gfp_mask;
>  		page_owner->last_migrate_reason = -1;
> +		page_owner->pid = current->pid;
> +		page_owner->ts_nsec = local_clock();
>  		__set_bit(PAGE_EXT_OWNER, &page_ext->flags);
>  		__set_bit(PAGE_EXT_OWNER_ALLOCATED, &page_ext->flags);
>  
> @@ -236,6 +241,8 @@ void __copy_page_owner(struct page *oldpage, struct page *newpage)
>  	new_page_owner->last_migrate_reason =
>  		old_page_owner->last_migrate_reason;
>  	new_page_owner->handle = old_page_owner->handle;
> +	new_page_owner->pid = old_page_owner->pid;
> +	new_page_owner->ts_nsec = old_page_owner->ts_nsec;
>  
>  	/*
>  	 * We don't clear the bit on the oldpage as it's going to be freed
> @@ -349,9 +356,10 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  		return -ENOMEM;
>  
>  	ret = snprintf(kbuf, count,
> -			"Page allocated via order %u, mask %#x(%pGg)\n",
> +			"Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns\n",
>  			page_owner->order, page_owner->gfp_mask,
> -			&page_owner->gfp_mask);
> +			&page_owner->gfp_mask, page_owner->pid,
> +			page_owner->ts_nsec);
>  
>  	if (ret >= count)
>  		goto err;
> @@ -427,8 +435,9 @@ void __dump_page_owner(struct page *page)
>  	else
>  		pr_alert("page_owner tracks the page as freed\n");
>  
> -	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg)\n",
> -		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask);
> +	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, ts %llu\n",
> +		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
> +		 page_owner->pid, page_owner->ts_nsec);
>  
>  	handle = READ_ONCE(page_owner->handle);
>  	if (!handle) {
>

Patch

diff --git a/Documentation/vm/page_owner.rst b/Documentation/vm/page_owner.rst
index 02deac76673f..cf7c0c361621 100644
--- a/Documentation/vm/page_owner.rst
+++ b/Documentation/vm/page_owner.rst
@@ -41,17 +41,17 @@  size change due to this facility.
 - Without page owner::
 
    text    data     bss     dec     hex filename
-   40662   1493     644   42799    a72f mm/page_alloc.o
+  48392    2333     644   51369    c8a9 mm/page_alloc.o
 
 - With page owner::
 
    text    data     bss     dec     hex filename
-   40892   1493     644   43029    a815 mm/page_alloc.o
-   1427      24       8    1459     5b3 mm/page_ext.o
-   2722      50       0    2772     ad4 mm/page_owner.o
+  48800    2445     644   51889    cab1 mm/page_alloc.o
+   6574     108      29    6711    1a37 mm/page_owner.o
+   1025       8       8    1041     411 mm/page_ext.o
 
-Although, roughly, 4 KB code is added in total, page_alloc.o increase by
-230 bytes and only half of it is in hotpath. Building the kernel with
+Although, roughly, 8 KB code is added in total, page_alloc.o increase by
+520 bytes and less than half of it is in hotpath. Building the kernel with
 page owner and turning it on if needed would be great option to debug
 kernel memory problem.
 
diff --git a/mm/page_owner.c b/mm/page_owner.c
index b735a8eafcdb..af464bb7fbe7 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -10,6 +10,7 @@ 
 #include <linux/migrate.h>
 #include <linux/stackdepot.h>
 #include <linux/seq_file.h>
+#include <linux/sched/clock.h>
 
 #include "internal.h"
 
@@ -25,6 +26,8 @@  struct page_owner {
 	gfp_t gfp_mask;
 	depot_stack_handle_t handle;
 	depot_stack_handle_t free_handle;
+	u64 ts_nsec;
+	pid_t pid;
 };
 
 static bool page_owner_enabled = false;
@@ -172,6 +175,8 @@  static inline void __set_page_owner_handle(struct page *page,
 		page_owner->order = order;
 		page_owner->gfp_mask = gfp_mask;
 		page_owner->last_migrate_reason = -1;
+		page_owner->pid = current->pid;
+		page_owner->ts_nsec = local_clock();
 		__set_bit(PAGE_EXT_OWNER, &page_ext->flags);
 		__set_bit(PAGE_EXT_OWNER_ALLOCATED, &page_ext->flags);
 
@@ -236,6 +241,8 @@  void __copy_page_owner(struct page *oldpage, struct page *newpage)
 	new_page_owner->last_migrate_reason =
 		old_page_owner->last_migrate_reason;
 	new_page_owner->handle = old_page_owner->handle;
+	new_page_owner->pid = old_page_owner->pid;
+	new_page_owner->ts_nsec = old_page_owner->ts_nsec;
 
 	/*
 	 * We don't clear the bit on the oldpage as it's going to be freed
@@ -349,9 +356,10 @@  print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 		return -ENOMEM;
 
 	ret = snprintf(kbuf, count,
-			"Page allocated via order %u, mask %#x(%pGg)\n",
+			"Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns\n",
 			page_owner->order, page_owner->gfp_mask,
-			&page_owner->gfp_mask);
+			&page_owner->gfp_mask, page_owner->pid,
+			page_owner->ts_nsec);
 
 	if (ret >= count)
 		goto err;
@@ -427,8 +435,9 @@  void __dump_page_owner(struct page *page)
 	else
 		pr_alert("page_owner tracks the page as freed\n");
 
-	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg)\n",
-		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask);
+	pr_alert("page last allocated via order %u, migratetype %s, gfp_mask %#x(%pGg), pid %d, ts %llu\n",
+		 page_owner->order, migratetype_names[mt], gfp_mask, &gfp_mask,
+		 page_owner->pid, page_owner->ts_nsec);
 
 	handle = READ_ONCE(page_owner->handle);
 	if (!handle) {