Changes v8 -> v9
- Fix handle-0 for the very first stack_record entry
- Collect Acked-by and Reviewed-by from Marco and Vlastimil
- Adressed feedback from Marco and Vlastimil
- stack_print() no longer allocates a memory buffer, prints directly
using seq_printf: by Vlastimil
- Added two static struct stack for dummy_handle and faiure_handle
- add_stack_record_to_list() now filters out the gfp_mask the same way
stackdepot does, for consistency
- Rename set_threshold to count_threshold
Changes v7 -> v8
- Rebased on top of -next
- page_owner maintains its own stack_records list now
- Kill auxiliary stackdepot function to traverse buckets
- page_owner_stacks is now a directory with 'show_stacks'
and 'set_threshold'
- Update Documentation/mm/page_owner.rst
- Adressed feedback from Marco
Changes v6 -> v7:
- Rebased on top of Andrey Konovalov's libstackdepot patchset
- Reformulated the changelogs
Changes v5 -> v6:
- Rebase on top of v6.7-rc1
- Move stack_record struct to the header
- Addressed feedback from Vlastimil
(some code tweaks and changelogs suggestions)
Changes v4 -> v5:
- Addressed feedback from Alexander Potapenko
Changes v3 -> v4:
- Rebase (long time has passed)
- Use boolean instead of enum for action by Alexander Potapenko
- (I left some feedback untouched because it's been long and
would like to discuss it here now instead of re-vamping
and old thread)
Changes v2 -> v3:
- Replace interface in favor of seq operations
(suggested by Vlastimil)
- Use debugfs interface to store/read valued (suggested by Ammar)
page_owner is a great debug functionality tool that lets us know
about all pages that have been allocated/freed and their specific
stacktrace.
This comes very handy when debugging memory leaks, since with
some scripting we can see the outstanding allocations, which might point
to a memory leak.
In my experience, that is one of the most useful cases, but it can get
really tedious to screen through all pages and try to reconstruct the
stack <-> allocated/freed relationship, becoming most of the time a
daunting and slow process when we have tons of allocation/free operations.
This patchset aims to ease that by adding a new functionality into
page_owner.
This functionality creates a new directory called 'page_owner_stacks'
under 'sys/kernel//debug' with a read-only file called 'show_stacks',
which prints out all the stacks followed by their outstanding number
of allocations (being that the times the stacktrace has allocated
but not freed yet).
This gives us a clear and a quick overview of stacks <-> allocated/free.
We take advantage of the new refcount_f field that stack_record struct
gained, and increment/decrement the stack refcount on every
__set_page_owner() (alloc operation) and __reset_page_owner (free operation)
call.
Unfortunately, we cannot use the new stackdepot api
STACK_DEPOT_FLAG_GET because it does not fulfill page_owner needs,
meaning we would have to special case things, at which point
makes more sense for page_owner to do its own {dec,inc}rementing
of the stacks.
E.g: Using STACK_DEPOT_FLAG_PUT, once the refcount reaches 0,
such stack gets evicted, so page_owner would lose information.
This patch also creates a new file called 'set_threshold' within
'page_owner_stacks' directory, and by writing a value to it, the stacks
which refcount is below such value will be filtered out.
A PoC can be found below:
# cat /sys/kernel/debug/page_owner_stacks/show_stacks > page_owner_full_stacks.txt
# head -40 page_owner_full_stacks.txt
prep_new_page+0xa9/0x120
get_page_from_freelist+0x801/0x2210
__alloc_pages+0x18b/0x350
alloc_pages_mpol+0x91/0x1f0
folio_alloc+0x14/0x50
filemap_alloc_folio+0xb2/0x100
page_cache_ra_unbounded+0x96/0x180
filemap_get_pages+0xfd/0x590
filemap_read+0xcc/0x330
blkdev_read_iter+0xb8/0x150
vfs_read+0x285/0x320
ksys_read+0xa5/0xe0
do_syscall_64+0x80/0x160
entry_SYSCALL_64_after_hwframe+0x6e/0x76
stack_count: 521
prep_new_page+0xa9/0x120
get_page_from_freelist+0x801/0x2210
__alloc_pages+0x18b/0x350
alloc_pages_mpol+0x91/0x1f0
folio_alloc+0x14/0x50
filemap_alloc_folio+0xb2/0x100
__filemap_get_folio+0x14a/0x490
ext4_write_begin+0xbd/0x4b0 [ext4]
generic_perform_write+0xc1/0x1e0
ext4_buffered_write_iter+0x68/0xe0 [ext4]
ext4_file_write_iter+0x70/0x740 [ext4]
vfs_write+0x33d/0x420
ksys_write+0xa5/0xe0
do_syscall_64+0x80/0x160
entry_SYSCALL_64_after_hwframe+0x6e/0x76
stack_count: 4609
..
..
# echo 5000 > /sys/kernel/debug/page_owner_stacks/set_threshold
# cat /sys/kernel/debug/page_owner_stacks/show_stacks > page_owner_full_stacks_5000.txt
# head -40 page_owner_full_stacks_5000.txt
prep_new_page+0xa9/0x120
get_page_from_freelist+0x801/0x2210
__alloc_pages+0x18b/0x350
alloc_pages_mpol+0x91/0x1f0
folio_alloc+0x14/0x50
filemap_alloc_folio+0xb2/0x100
__filemap_get_folio+0x14a/0x490
ext4_write_begin+0xbd/0x4b0 [ext4]
generic_perform_write+0xc1/0x1e0
ext4_buffered_write_iter+0x68/0xe0 [ext4]
ext4_file_write_iter+0x70/0x740 [ext4]
vfs_write+0x33d/0x420
ksys_pwrite64+0x75/0x90
do_syscall_64+0x80/0x160
entry_SYSCALL_64_after_hwframe+0x6e/0x76
stack_count: 6781
prep_new_page+0xa9/0x120
get_page_from_freelist+0x801/0x2210
__alloc_pages+0x18b/0x350
pcpu_populate_chunk+0xec/0x350
pcpu_balance_workfn+0x2d1/0x4a0
process_scheduled_works+0x84/0x380
worker_thread+0x12a/0x2a0
kthread+0xe3/0x110
ret_from_fork+0x30/0x50
ret_from_fork_asm+0x1b/0x30
stack_count: 8641
Oscar Salvador (7):
lib/stackdepot: Fix first entry having a 0-handle
lib/stackdepot: Move stack_record struct definition into the header
mm,page_owner: Maintain own list of stack_records structs
mm,page_owner: Implement the tracking of the stacks count
mm,page_owner: Display all stacks and their count
mm,page_owner: Filter out stacks by a threshold
mm,page_owner: Update Documentation regarding page_owner_stacks
Documentation/mm/page_owner.rst | 45 +++++++
include/linux/stackdepot.h | 58 +++++++++
lib/stackdepot.c | 65 +++--------
mm/page_owner.c | 200 +++++++++++++++++++++++++++++++-
4 files changed, 318 insertions(+), 50 deletions(-)
--
2.43.0
We want to be able to filter out the stacks based on a threshold we can
can tune.
By writing to 'count_threshold' file, we can adjust the threshold value.
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/page_owner.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 5258a417f4d1..9b975f59b773 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -846,9 +846,11 @@ static void *stack_next(struct seq_file *m, void *v, loff_t *ppos)
return stack;
}
+static unsigned long page_owner_stack_threshold;
+
static int stack_print(struct seq_file *m, void *v)
{
- int i;
+ int i, stack_count;
struct stack *stack = v;
unsigned long *entries;
unsigned long nr_entries;
@@ -856,14 +858,15 @@ static int stack_print(struct seq_file *m, void *v)
nr_entries = stack_record->size;
entries = stack_record->entries;
+ stack_count = refcount_read(&stack_record->count);
- if (!nr_entries || nr_entries < 0 ||
- refcount_read(&stack_record->count) < 2)
+ if (!nr_entries || nr_entries < 0 || stack_count < 2 ||
+ stack_count < page_owner_stack_threshold)
return 0;
for (i = 0; i < nr_entries; i++)
seq_printf(m, " %pS\n", (void *)entries[i]);
- seq_printf(m, "stack_count: %d\n\n", refcount_read(&stack_record->count));
+ seq_printf(m, "stack_count: %d\n\n", stack_count);
return 0;
}
@@ -891,6 +894,22 @@ static const struct file_operations page_owner_stack_operations = {
.release = seq_release,
};
+static int page_owner_threshold_get(void *data, u64 *val)
+{
+ *val = READ_ONCE(page_owner_stack_threshold);
+ return 0;
+}
+
+static int page_owner_threshold_set(void *data, u64 val)
+{
+ WRITE_ONCE(page_owner_stack_threshold, val);
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(proc_page_owner_threshold, &page_owner_threshold_get,
+ &page_owner_threshold_set, "%llu");
+
+
static int __init pageowner_init(void)
{
struct dentry *dir;
@@ -905,6 +924,8 @@ static int __init pageowner_init(void)
dir = debugfs_create_dir("page_owner_stacks", NULL);
debugfs_create_file("show_stacks", 0400, dir, NULL,
&page_owner_stack_operations);
+ debugfs_create_file("count_threshold", 0600, dir, NULL,
+ &proc_page_owner_threshold);
return 0;
}
--
2.43.0
On 2/14/24 18:01, Oscar Salvador wrote:
> We want to be able to filter out the stacks based on a threshold we can
> can tune.
> By writing to 'count_threshold' file, we can adjust the threshold value.
>
> Signed-off-by: Oscar Salvador <[email protected]>
Reviewed-by: Vlastimil Babka <[email protected]>
..
> ---
> mm/page_owner.c | 29 +++++++++++++++++++++++++----
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 5258a417f4d1..9b975f59b773 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -846,9 +846,11 @@ static void *stack_next(struct seq_file *m, void *v, loff_t *ppos)
> return stack;
> }
>
> +static unsigned long page_owner_stack_threshold;
> +
> static int stack_print(struct seq_file *m, void *v)
> {
> - int i;
> + int i, stack_count;
> struct stack *stack = v;
> unsigned long *entries;
> unsigned long nr_entries;
> @@ -856,14 +858,15 @@ static int stack_print(struct seq_file *m, void *v)
>
> nr_entries = stack_record->size;
> entries = stack_record->entries;
> + stack_count = refcount_read(&stack_record->count);
Again "- 1" here.
> - if (!nr_entries || nr_entries < 0 ||
> - refcount_read(&stack_record->count) < 2)
> + if (!nr_entries || nr_entries < 0 || stack_count < 2 ||
> + stack_count < page_owner_stack_threshold)
Which will also correct the comparison.
> return 0;
>
> for (i = 0; i < nr_entries; i++)
> seq_printf(m, " %pS\n", (void *)entries[i]);
> - seq_printf(m, "stack_count: %d\n\n", refcount_read(&stack_record->count));
> + seq_printf(m, "stack_count: %d\n\n", stack_count);
And no - 1 needed here then.
On Thu, Feb 15, 2024 at 12:12:56PM +0100, Vlastimil Babka wrote:
> On 2/14/24 18:01, Oscar Salvador wrote:
> > We want to be able to filter out the stacks based on a threshold we can
> > can tune.
> > By writing to 'count_threshold' file, we can adjust the threshold value.
> >
> > Signed-off-by: Oscar Salvador <[email protected]>
>
> Reviewed-by: Vlastimil Babka <[email protected]>
Thanks
> Again "- 1" here.
Yes, I might just instead add this assigment in the previous patch,
so no further modifications wrt. stack_count will be needed in this
patch.
Might be cleaner?
--
Oscar Salvador
SUSE Labs