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)
Hi,
page_owner is a great debug functionality tool that gets us to know
about all pages that have been allocated/freed and their stacktrace.
This comes very handy when e.g: debugging leaks, as with some scripting
we might be able to see those stacktraces that are allocating pages
but not freeing theme.
In my experience, that is one of the most useful cases, but it can get
really tedious to screen through all pages aand try to reconstruct the
stack <-> allocated/freed relationship. There is a lot of noise
to cancel off.
This patch aims to fix that by adding a new functionality into page_owner.
What this does is to create a new read-only file "page_owner_stacks",
which prints only the allocating stacktraces and their counting, being that
the times the stacktrace has allocated - the times it has freed.
So we have a clear overview of stacks <-> allocated/freed relationship
without the need to fiddle with pages and trying to match free stacktraces
with allocated stacktraces.
This is achieved by adding a new refcount_t field in the stack_record struct,
incrementing that refcount_t everytime the same stacktrace allocates,
and decrementing it when it frees a page. Details can be seen in the
respective patches.
We also create another file called "page_owner_threshold", which let us
specify a threshold, so when when reading from "page_owner_stacks",
we will only see those stacktraces which counting goes beyond the
threshold we specified.
One thing I am not completely happy about is to polute lib/stackdepot.c file
with the stack_* functions.
We could sort that out if the stack_record struct definitions were in a header
file instead of stackdepot.c.
But I am not sure about that trade-off, so suggestions are accepted.
A PoC can be found below:
# cat /sys/kernel/debug/page_owner_threshold
0
# cat /sys/kernel/debug/page_owner_stacks > stacks_full.txt
# head -32 stacks_full.txt
prep_new_page+0x10d/0x180
get_page_from_freelist+0x1bd6/0x1e10
__alloc_pages+0x194/0x360
alloc_page_interleave+0x13/0x90
new_slab+0x31d/0x530
___slab_alloc+0x5d7/0x720
__slab_alloc.isra.85+0x4a/0x90
kmem_cache_alloc+0x455/0x4a0
acpi_ps_alloc_op+0x57/0x8f
acpi_ps_create_scope_op+0x12/0x23
acpi_ps_execute_method+0x102/0x2c1
acpi_ns_evaluate+0x343/0x4da
acpi_evaluate_object+0x1cb/0x392
acpi_run_osc+0x135/0x260
acpi_init+0x165/0x4ed
do_one_initcall+0x3e/0x200
stack count: 2
free_pcp_prepare+0x287/0x5c0
free_unref_page+0x1c/0xd0
__mmdrop+0x50/0x160
finish_task_switch+0x249/0x2b0
__schedule+0x2c3/0x960
schedule+0x44/0xb0
futex_wait_queue+0x70/0xd0
futex_wait+0x160/0x250
do_futex+0x11c/0x1b0
__x64_sys_futex+0x5e/0x1d0
do_syscall_64+0x37/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 1
# echo 10000 > /sys/kernel/debug/page_owner_threshold
# cat /sys/kernel/debug/page_owner_stacks > stacks_10000.txt
# cat stacks_10000.txt
prep_new_page+0x10d/0x180
get_page_from_freelist+0x1bd6/0x1e10
__alloc_pages+0x194/0x360
folio_alloc+0x17/0x40
page_cache_ra_unbounded+0x96/0x170
filemap_get_pages+0x23d/0x5e0
filemap_read+0xbf/0x3a0
__kernel_read+0x136/0x2f0
kernel_read_file+0x197/0x2d0
kernel_read_file_from_fd+0x54/0x90
__do_sys_finit_module+0x89/0x120
do_syscall_64+0x37/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 36195
prep_new_page+0x10d/0x180
get_page_from_freelist+0x1bd6/0x1e10
__alloc_pages+0x194/0x360
folio_alloc+0x17/0x40
page_cache_ra_unbounded+0x96/0x170
filemap_get_pages+0x23d/0x5e0
filemap_read+0xbf/0x3a0
new_sync_read+0x106/0x180
vfs_read+0x16f/0x190
ksys_read+0xa5/0xe0
do_syscall_64+0x37/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 44484
prep_new_page+0x10d/0x180
get_page_from_freelist+0x1bd6/0x1e10
__alloc_pages+0x194/0x360
folio_alloc+0x17/0x40
page_cache_ra_unbounded+0x96/0x170
filemap_get_pages+0xdd/0x5e0
filemap_read+0xbf/0x3a0
new_sync_read+0x106/0x180
vfs_read+0x16f/0x190
ksys_read+0xa5/0xe0
do_syscall_64+0x37/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 17874
Oscar Salvador (3):
lib/stackdepot: Add a refcount field in stack_record
mm, page_owner: Add page_owner_stacks file to print out only stacks
and their counte
mm,page_owner: Filter out stacks by a threshold counter
include/linux/stackdepot.h | 17 +++-
lib/stackdepot.c | 160 ++++++++++++++++++++++++++++++++++---
mm/kasan/common.c | 3 +-
mm/page_owner.c | 46 ++++++++++-
4 files changed, 209 insertions(+), 17 deletions(-)
--
2.35.3
We want to filter out page_owner output and print only those
stacks that have been repeated beyond a certain threshold.
This gives us the chance to get rid of a lot of noise.
In order to do that, we need to keep track of how many repeated stacks
(for allocation) do we have, so we add a new refcount_t field
in the stack_record struct.
Note that this might increase the size of the struct for some
architectures.
E.g: x86_64 is not affected due to alignment, but x86 32bits might.
The alternative would be to have some kind of struct like this:
struct track_stacks {
struct stack_record *stack;
struct track_stacks *next;
refcount_t stack_count;
But ithat would imply to perform more allocations and glue everything
together, which would make the code more complex, so I think that
going with a new field in the struct stack_record is good enough.
Note that on __set_page_owner_handle(), page_owner->handle is set,
and on __reset_page_owner(), page_owner->free_handle is set.
We are interested in page_owner->handle, so when __set_page_owner()
gets called, we derive the stack_record struct from page_owner->handle,
and we increment its refcount_t field; and when __reset_page_owner()
gets called, we derive its stack_record from page_owner->handle()
and we decrement its refcount_t field.
Signed-off-by: Oscar Salvador <[email protected]>
---
include/linux/stackdepot.h | 8 ++++-
lib/stackdepot.c | 72 ++++++++++++++++++++++++++++++++------
mm/kasan/common.c | 3 +-
mm/page_owner.c | 13 ++++---
4 files changed, 79 insertions(+), 17 deletions(-)
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index e58306783d8e..b94d33312839 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -93,7 +93,9 @@ static inline int stack_depot_early_init(void) { return 0; }
*/
depot_stack_handle_t __stack_depot_save(unsigned long *entries,
unsigned int nr_entries,
- gfp_t gfp_flags, bool can_alloc);
+ gfp_t gfp_flags, bool can_alloc,
+ bool counter);
+void stack_depot_dec_count(depot_stack_handle_t handle);
/**
* stack_depot_save - Save a stack trace to stack depot
@@ -109,6 +111,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
*/
depot_stack_handle_t stack_depot_save(unsigned long *entries,
unsigned int nr_entries, gfp_t gfp_flags);
+depot_stack_handle_t stack_depot_save_action(unsigned long *entries,
+ unsigned int nr_entries,
+ gfp_t gfp_flags,
+ bool counter);
/**
* stack_depot_fetch - Fetch a stack trace from stack depot
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 036da8e295d1..e99f4ef218ef 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -59,6 +59,7 @@ struct stack_record {
u32 hash; /* Hash in the hash table */
u32 size; /* Number of stored frames */
union handle_parts handle;
+ refcount_t count; /* Number of the same repeated stacks */
unsigned long entries[]; /* Variable-sized array of frames */
};
@@ -304,6 +305,7 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
stack->handle.offset = pool_offset >> DEPOT_STACK_ALIGN;
stack->handle.valid = 1;
stack->handle.extra = 0;
+ refcount_set(&stack->count, 1);
memcpy(stack->entries, entries, flex_array_size(stack, entries, size));
pool_offset += required_size;
@@ -349,9 +351,15 @@ static inline struct stack_record *find_stack(struct stack_record *bucket,
return NULL;
}
+static void stack_depot_inc_count(struct stack_record *stack)
+{
+ refcount_inc(&stack->count);
+}
+
depot_stack_handle_t __stack_depot_save(unsigned long *entries,
unsigned int nr_entries,
- gfp_t alloc_flags, bool can_alloc)
+ gfp_t alloc_flags, bool can_alloc,
+ bool counter)
{
struct stack_record *found = NULL, **bucket;
union handle_parts retval = { .handle = 0 };
@@ -436,8 +444,11 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
/* Stack depot didn't use this memory, free it. */
free_pages((unsigned long)prealloc, DEPOT_POOL_ORDER);
}
- if (found)
+ if (found) {
retval.handle = found->handle.handle;
+ if (counter)
+ stack_depot_inc_count(found);
+ }
fast_exit:
return retval.handle;
}
@@ -447,12 +458,20 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries,
unsigned int nr_entries,
gfp_t alloc_flags)
{
- return __stack_depot_save(entries, nr_entries, alloc_flags, true);
+ return __stack_depot_save(entries, nr_entries, alloc_flags, true, false);
}
EXPORT_SYMBOL_GPL(stack_depot_save);
-unsigned int stack_depot_fetch(depot_stack_handle_t handle,
- unsigned long **entries)
+depot_stack_handle_t stack_depot_save_action(unsigned long *entries,
+ unsigned int nr_entries,
+ gfp_t alloc_flags,
+ bool counter)
+{
+ return __stack_depot_save(entries, nr_entries, alloc_flags, true, counter);
+}
+EXPORT_SYMBOL_GPL(stack_depot_save_action);
+
+static struct stack_record *stack_depot_getstack(depot_stack_handle_t handle)
{
union handle_parts parts = { .handle = handle };
/*
@@ -464,25 +483,56 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
size_t offset = parts.offset << DEPOT_STACK_ALIGN;
struct stack_record *stack;
- *entries = NULL;
- if (!handle)
- return 0;
+ if(!handle)
+ return NULL;
if (parts.pool_index > pool_index_cached) {
WARN(1, "pool index %d out of bounds (%d) for stack id %08x\n",
- parts.pool_index, pool_index_cached, handle);
- return 0;
+ parts.pool_index, pool_index_cached, handle);
+ return NULL;
}
pool = stack_pools[parts.pool_index];
if (!pool)
- return 0;
+ return NULL;
+
stack = pool + offset;
+ return stack;
+}
+
+unsigned int stack_depot_fetch(depot_stack_handle_t handle,
+ unsigned long **entries)
+{
+ struct stack_record *stack;
+
+ *entries = NULL;
+ if (!handle)
+ return 0;
+
+ stack = stack_depot_getstack(handle);
+ if (!stack)
+ return 0;
*entries = stack->entries;
return stack->size;
}
EXPORT_SYMBOL_GPL(stack_depot_fetch);
+void stack_depot_dec_count(depot_stack_handle_t handle)
+{
+ struct stack_record *stack = NULL;
+
+ stack = stack_depot_getstack(handle);
+ if (stack) {
+ /*
+ * page_owner creates some stacks via create_dummy_stack().
+ * We are not interested in those, so make sure we only decrement
+ * "valid" stacks.
+ */
+ if (refcount_read(&stack->count) > 1)
+ refcount_dec(&stack->count);
+ }
+}
+
void stack_depot_print(depot_stack_handle_t stack)
{
unsigned long *entries;
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index b376a5d055e5..ea0061ea8ae9 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -43,7 +43,8 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags, bool can_alloc)
unsigned int nr_entries;
nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
- return __stack_depot_save(entries, nr_entries, flags, can_alloc);
+ return __stack_depot_save(entries, nr_entries, flags, can_alloc,
+ false);
}
void kasan_set_track(struct kasan_track *track, gfp_t flags)
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 220cdeddc295..b6637524e442 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -107,7 +107,7 @@ static inline struct page_owner *get_page_owner(struct page_ext *page_ext)
return (void *)page_ext + page_owner_ops.offset;
}
-static noinline depot_stack_handle_t save_stack(gfp_t flags)
+static noinline depot_stack_handle_t save_stack(gfp_t flags, bool counter)
{
unsigned long entries[PAGE_OWNER_STACK_DEPTH];
depot_stack_handle_t handle;
@@ -126,7 +126,7 @@ static noinline depot_stack_handle_t save_stack(gfp_t flags)
current->in_page_owner = 1;
nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 2);
- handle = stack_depot_save(entries, nr_entries, flags);
+ handle = stack_depot_save_action(entries, nr_entries, flags, counter);
if (!handle)
handle = failure_handle;
@@ -139,6 +139,7 @@ void __reset_page_owner(struct page *page, unsigned short order)
int i;
struct page_ext *page_ext;
depot_stack_handle_t handle;
+ depot_stack_handle_t alloc_handle;
struct page_owner *page_owner;
u64 free_ts_nsec = local_clock();
@@ -146,7 +147,10 @@ void __reset_page_owner(struct page *page, unsigned short order)
if (unlikely(!page_ext))
return;
- handle = save_stack(GFP_NOWAIT | __GFP_NOWARN);
+ page_owner = get_page_owner(page_ext);
+ alloc_handle = page_owner->handle;
+
+ handle = save_stack(GFP_NOWAIT | __GFP_NOWARN, false);
for (i = 0; i < (1 << order); i++) {
__clear_bit(PAGE_EXT_OWNER_ALLOCATED, &page_ext->flags);
page_owner = get_page_owner(page_ext);
@@ -155,6 +159,7 @@ void __reset_page_owner(struct page *page, unsigned short order)
page_ext = page_ext_next(page_ext);
}
page_ext_put(page_ext);
+ stack_depot_dec_count(alloc_handle);
}
static inline void __set_page_owner_handle(struct page_ext *page_ext,
@@ -189,7 +194,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
struct page_ext *page_ext;
depot_stack_handle_t handle;
- handle = save_stack(gfp_mask);
+ handle = save_stack(gfp_mask, true);
page_ext = page_ext_get(page);
if (unlikely(!page_ext))
--
2.35.3
We might be only interested in knowing about stacks <-> count
relationship, so instead of having to fiddle with page_owner
output and screen through pfns, let us add a new file called
'page_owner_stacks' that does just that.
By cating such file, we will get all the stacktraces followed by
its counter, so we can have a more global view.
Signed-off-by: Oscar Salvador <[email protected]
---
include/linux/stackdepot.h | 6 ++++
lib/stackdepot.c | 73 ++++++++++++++++++++++++++++++++++++++
mm/page_owner.c | 28 +++++++++++++++
3 files changed, 107 insertions(+)
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index b94d33312839..e1d05d9adcd1 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -116,6 +116,12 @@ depot_stack_handle_t stack_depot_save_action(unsigned long *entries,
gfp_t gfp_flags,
bool counter);
+#ifdef CONFIG_PAGE_OWNER
+void *stack_start(struct seq_file *m, loff_t *ppos);
+void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
+int stack_print(struct seq_file *m, void *v);
+#endif
+
/**
* stack_depot_fetch - Fetch a stack trace from stack depot
*
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index e99f4ef218ef..d0a4e6ac0bc9 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -28,6 +28,8 @@
#include <linux/types.h>
#include <linux/memblock.h>
#include <linux/kasan-enabled.h>
+#include <linux/seq_file.h>
+#include <linux/debugfs.h>
#define DEPOT_HANDLE_BITS (sizeof(depot_stack_handle_t) * 8)
@@ -499,6 +501,77 @@ static struct stack_record *stack_depot_getstack(depot_stack_handle_t handle)
return stack;
}
+#ifdef CONFIG_PAGE_OWNER
+void *stack_start(struct seq_file *m, loff_t *ppos)
+{
+ unsigned long *table = m->private;
+ struct stack_record **stacks, *stack;
+
+ /* First time */
+ if (*ppos == 0)
+ *table = 0;
+
+ if (*ppos == -1UL)
+ return NULL;
+
+ stacks = &stack_table[*table];
+ stack = (struct stack_record *)stacks;
+
+ return stack;
+}
+
+void *stack_next(struct seq_file *m, void *v, loff_t *ppos)
+{
+ unsigned long *table = m->private;
+ unsigned long nr_table = *table;
+ struct stack_record *next = NULL, *stack = v, **stacks;
+ unsigned long stack_table_entries = stack_hash_mask + 1;
+
+ if (!stack) {
+new_table:
+ /* New table */
+ nr_table++;
+ if (nr_table >= stack_table_entries)
+ goto out;
+ stacks = &stack_table[nr_table];
+ stack = (struct stack_record *)stacks;
+ next = stack;
+ } else {
+ next = stack->next;
+ }
+
+ if (!next)
+ goto new_table;
+
+out:
+ *table = nr_table;
+ *ppos = (nr_table >= stack_table_entries) ? -1UL : *ppos + 1;
+ return next;
+}
+
+int stack_print(struct seq_file *m, void *v)
+{
+ char *buf;
+ int ret = 0;
+ struct stack_record *stack =v;
+
+ if (!stack->size || stack->size < 0 ||
+ stack->size > PAGE_SIZE || stack->handle.valid != 1 ||
+ refcount_read(&stack->count) < 1)
+ return 0;
+
+ buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ ret += stack_trace_snprint(buf, PAGE_SIZE, stack->entries, stack->size, 0);
+ scnprintf(buf + ret, PAGE_SIZE - ret, "stack count: %d\n\n",
+ refcount_read(&stack->count));
+ seq_printf(m, buf);
+ seq_puts(m, "\n\n");
+ kfree(buf);
+
+ return 0;
+}
+#endif
+
unsigned int stack_depot_fetch(depot_stack_handle_t handle,
unsigned long **entries)
{
diff --git a/mm/page_owner.c b/mm/page_owner.c
index b6637524e442..b191ad1d41f9 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -718,6 +718,31 @@ static const struct file_operations proc_page_owner_operations = {
.llseek = lseek_page_owner,
};
+static void stack_stop(struct seq_file *m, void *v)
+{
+ return;
+}
+
+static const struct seq_operations page_owner_stack_op = {
+ .start = stack_start,
+ .next = stack_next,
+ .stop = stack_stop,
+ .show = stack_print
+};
+
+static int page_owner_stack_open(struct inode *inode, struct file *file)
+{
+ return seq_open_private(file, &page_owner_stack_op,
+ sizeof(unsigned long));
+}
+
+const struct file_operations page_owner_stack_operations = {
+ .open = page_owner_stack_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release,
+};
+
static int __init pageowner_init(void)
{
if (!static_branch_unlikely(&page_owner_inited)) {
@@ -728,6 +753,9 @@ static int __init pageowner_init(void)
debugfs_create_file("page_owner", 0400, NULL, NULL,
&proc_page_owner_operations);
+ debugfs_create_file("page_owner_stacks", S_IRUSR, NULL, NULL,
+ &page_owner_stack_operations);
+
return 0;
}
late_initcall(pageowner_init)
--
2.35.3
We want to be able to filter out the output on a threshold basis,
in this way we can get rid of a lot of noise and focus only on those
stacks which have an allegedly high counter.
We can control the threshold value by a new file called
'page_owner_threshold', which is 0 by default.
Signed-off-by: Oscar Salvador <[email protected]>
---
include/linux/stackdepot.h | 3 +++
lib/stackdepot.c | 17 ++++++++++++++++-
mm/page_owner.c | 5 +++++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index e1d05d9adcd1..c6b54199ea26 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -120,6 +120,9 @@ depot_stack_handle_t stack_depot_save_action(unsigned long *entries,
void *stack_start(struct seq_file *m, loff_t *ppos);
void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
int stack_print(struct seq_file *m, void *v);
+
+int page_owner_threshold_get(void *data, u64 *val);
+int page_owner_threshold_set(void *data, u64 val);
#endif
/**
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index d0a4e6ac0bc9..2f1a41f0ae4f 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -502,6 +502,9 @@ static struct stack_record *stack_depot_getstack(depot_stack_handle_t handle)
}
#ifdef CONFIG_PAGE_OWNER
+
+static unsigned long page_owner_stack_threshold = 0;
+
void *stack_start(struct seq_file *m, loff_t *ppos)
{
unsigned long *table = m->private;
@@ -557,7 +560,7 @@ int stack_print(struct seq_file *m, void *v)
if (!stack->size || stack->size < 0 ||
stack->size > PAGE_SIZE || stack->handle.valid != 1 ||
- refcount_read(&stack->count) < 1)
+ refcount_read(&stack->count) < page_owner_stack_threshold)
return 0;
buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
@@ -570,6 +573,18 @@ int stack_print(struct seq_file *m, void *v)
return 0;
}
+
+int page_owner_threshold_get(void *data, u64 *val)
+{
+ *val = page_owner_stack_threshold;
+ return 0;
+}
+
+int page_owner_threshold_set(void *data, u64 val)
+{
+ page_owner_stack_threshold = val;
+ return 0;
+}
#endif
unsigned int stack_depot_fetch(depot_stack_handle_t handle,
diff --git a/mm/page_owner.c b/mm/page_owner.c
index b191ad1d41f9..daec789b0b50 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -743,6 +743,9 @@ const struct file_operations page_owner_stack_operations = {
.release = seq_release,
};
+DEFINE_SIMPLE_ATTRIBUTE(proc_page_owner_threshold, &page_owner_threshold_get,
+ &page_owner_threshold_set, "%lu");
+
static int __init pageowner_init(void)
{
if (!static_branch_unlikely(&page_owner_inited)) {
@@ -755,6 +758,8 @@ static int __init pageowner_init(void)
debugfs_create_file("page_owner_stacks", S_IRUSR, NULL, NULL,
&page_owner_stack_operations);
+ debugfs_create_file("page_owner_threshold", 0600, NULL, NULL,
+ &proc_page_owner_threshold);
return 0;
}
--
2.35.3
On Fri, Apr 21, 2023 at 12:14 PM Oscar Salvador <[email protected]> wrote:
>
> 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)
>
> Hi,
>
> page_owner is a great debug functionality tool that gets us to know
> about all pages that have been allocated/freed and their stacktrace.
> This comes very handy when e.g: debugging leaks, as with some scripting
> we might be able to see those stacktraces that are allocating pages
> but not freeing theme.
>
> In my experience, that is one of the most useful cases, but it can get
> really tedious to screen through all pages aand try to reconstruct the
> stack <-> allocated/freed relationship. There is a lot of noise
> to cancel off.
>
> This patch aims to fix that by adding a new functionality into page_owner.
> What this does is to create a new read-only file "page_owner_stacks",
> which prints only the allocating stacktraces and their counting, being that
> the times the stacktrace has allocated - the times it has freed.
>
> So we have a clear overview of stacks <-> allocated/freed relationship
> without the need to fiddle with pages and trying to match free stacktraces
> with allocated stacktraces.
>
> This is achieved by adding a new refcount_t field in the stack_record struct,
> incrementing that refcount_t everytime the same stacktrace allocates,
> and decrementing it when it frees a page. Details can be seen in the
> respective patches.
I think the implementation of these counters is too specific to
page_owner and is hard to use for any other purpose.
If we decide to have them, there should be no page_owner-specific
logic in the way we initialize/increment/decrement these counters.
The thresholds in "mm,page_owner: Filter out stacks by a threshold
counter" should also belong elsewhere.
Given that no other stackdepot user needs these counters, maybe it
should be cleaner to store an opaque struct along with the stack,
passing its size to stack_depot_save(), and letting users access it
directly using the stackdepot handler.
I am also wondering if a separate hashtable mapping handlers to
counters would solve the problem for you?
Hi Oscar,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc7]
[cannot apply to akpm-mm/mm-everything next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
patch link: https://lore.kernel.org/r/20230421101415.5734-3-osalvador%40suse.de
patch subject: [PATCH v4 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counte
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230421/[email protected]/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6c6dfc43015e1939f433f4371d33418ab4d03411
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
git checkout 6c6dfc43015e1939f433f4371d33418ab4d03411
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash M=lib/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from include/linux/page_ext.h:7,
from include/linux/mm.h:22,
from include/linux/mman.h:5,
from lib/test_user_copy.c:13:
>> include/linux/stackdepot.h:120:26: warning: 'struct seq_file' declared inside parameter list will not be visible outside of this definition or declaration
120 | void *stack_start(struct seq_file *m, loff_t *ppos);
| ^~~~~~~~
include/linux/stackdepot.h:121:25: warning: 'struct seq_file' declared inside parameter list will not be visible outside of this definition or declaration
121 | void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
| ^~~~~~~~
include/linux/stackdepot.h:122:24: warning: 'struct seq_file' declared inside parameter list will not be visible outside of this definition or declaration
122 | int stack_print(struct seq_file *m, void *v);
| ^~~~~~~~
vim +120 include/linux/stackdepot.h
99
100 /**
101 * stack_depot_save - Save a stack trace to stack depot
102 *
103 * @entries: Pointer to the stack trace
104 * @nr_entries: Number of frames in the stack
105 * @alloc_flags: Allocation GFP flags
106 *
107 * Context: Contexts where allocations via alloc_pages() are allowed.
108 * See __stack_depot_save() for more details.
109 *
110 * Return: Handle of the stack trace stored in depot, 0 on failure
111 */
112 depot_stack_handle_t stack_depot_save(unsigned long *entries,
113 unsigned int nr_entries, gfp_t gfp_flags);
114 depot_stack_handle_t stack_depot_save_action(unsigned long *entries,
115 unsigned int nr_entries,
116 gfp_t gfp_flags,
117 bool counter);
118
119 #ifdef CONFIG_PAGE_OWNER
> 120 void *stack_start(struct seq_file *m, loff_t *ppos);
121 void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
122 int stack_print(struct seq_file *m, void *v);
123 #endif
124
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Oscar,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc7]
[cannot apply to akpm-mm/mm-everything next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
base: linus/master
patch link: https://lore.kernel.org/r/20230421101415.5734-4-osalvador%40suse.de
patch subject: [PATCH v4 3/3] mm,page_owner: Filter out stacks by a threshold counter
config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20230422/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/716d3f03add56cf9ed9ae5e49d73cf7e0cbfcb19
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
git checkout 716d3f03add56cf9ed9ae5e49d73cf7e0cbfcb19
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from include/linux/debugfs.h:15,
from mm/page_owner.c:2:
mm/page_owner.c: In function 'proc_page_owner_threshold_open':
>> mm/page_owner.c:747:52: warning: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'long long unsigned int' [-Wformat=]
747 | &page_owner_threshold_set, "%lu");
| ^~~~~
include/linux/fs.h:3066:36: note: in definition of macro 'DEFINE_SIMPLE_ATTRIBUTE_XSIGNED'
3066 | __simple_attr_check_format(__fmt, 0ull); \
| ^~~~~
mm/page_owner.c:746:1: note: in expansion of macro 'DEFINE_SIMPLE_ATTRIBUTE'
746 | DEFINE_SIMPLE_ATTRIBUTE(proc_page_owner_threshold, &page_owner_threshold_get,
| ^~~~~~~~~~~~~~~~~~~~~~~
mm/page_owner.c:747:55: note: format string is defined here
747 | &page_owner_threshold_set, "%lu");
| ~~^
| |
| long unsigned int
| %llu
vim +747 mm/page_owner.c
745
746 DEFINE_SIMPLE_ATTRIBUTE(proc_page_owner_threshold, &page_owner_threshold_get,
> 747 &page_owner_threshold_set, "%lu");
748
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Oscar,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc7]
[cannot apply to akpm-mm/mm-everything next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
base: linus/master
patch link: https://lore.kernel.org/r/20230421101415.5734-4-osalvador%40suse.de
patch subject: [PATCH v4 3/3] mm,page_owner: Filter out stacks by a threshold counter
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20230422/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/716d3f03add56cf9ed9ae5e49d73cf7e0cbfcb19
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
git checkout 716d3f03add56cf9ed9ae5e49d73cf7e0cbfcb19
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> mm/page_owner.c:746:1: warning: format specifies type 'unsigned long' but the argument has type 'unsigned long long' [-Wformat]
DEFINE_SIMPLE_ATTRIBUTE(proc_page_owner_threshold, &page_owner_threshold_get,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fs.h:3079:2: note: expanded from macro 'DEFINE_SIMPLE_ATTRIBUTE'
DEFINE_SIMPLE_ATTRIBUTE_XSIGNED(__fops, __get, __set, __fmt, false)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/fs.h:3066:36: note: expanded from macro 'DEFINE_SIMPLE_ATTRIBUTE_XSIGNED'
__simple_attr_check_format(__fmt, 0ull); \
~~~~~ ^~~~
1 warning generated.
vim +746 mm/page_owner.c
745
> 746 DEFINE_SIMPLE_ATTRIBUTE(proc_page_owner_threshold, &page_owner_threshold_get,
747 &page_owner_threshold_set, "%lu");
748
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Oscar,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.3-rc7]
[cannot apply to akpm-mm/mm-everything next-20230421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
base: linus/master
patch link: https://lore.kernel.org/r/20230421101415.5734-3-osalvador%40suse.de
patch subject: [PATCH v4 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counte
config: powerpc-randconfig-r026-20230421 (https://download.01.org/0day-ci/archive/20230422/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 437b7602e4a998220871de78afcb020b9c14a661)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/6c6dfc43015e1939f433f4371d33418ab4d03411
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
git checkout 6c6dfc43015e1939f433f4371d33418ab4d03411
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash arch/powerpc/kernel/ kernel/dma/ lib/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
In file included from lib/show_mem.c:8:
In file included from include/linux/mm.h:22:
In file included from include/linux/page_ext.h:7:
>> include/linux/stackdepot.h:120:26: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
void *stack_start(struct seq_file *m, loff_t *ppos);
^
include/linux/stackdepot.h:121:25: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
^
include/linux/stackdepot.h:122:24: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
int stack_print(struct seq_file *m, void *v);
^
3 warnings generated.
--
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:8:
In file included from include/linux/mm.h:22:
In file included from include/linux/page_ext.h:7:
>> include/linux/stackdepot.h:120:26: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
void *stack_start(struct seq_file *m, loff_t *ppos);
^
include/linux/stackdepot.h:121:25: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
^
include/linux/stackdepot.h:122:24: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
int stack_print(struct seq_file *m, void *v);
^
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:214:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:577:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:216:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:578:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:218:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:579:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:220:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:580:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:222:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:581:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/scatterlist.c:9:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:224:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:582:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
9 warnings generated.
--
In file included from lib/stackdepot.c:20:
In file included from include/linux/mm.h:22:
In file included from include/linux/page_ext.h:7:
>> include/linux/stackdepot.h:120:26: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
void *stack_start(struct seq_file *m, loff_t *ppos);
^
include/linux/stackdepot.h:121:25: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
^
include/linux/stackdepot.h:122:24: warning: declaration of 'struct seq_file' will not be visible outside of this function [-Wvisibility]
int stack_print(struct seq_file *m, void *v);
^
In file included from lib/stackdepot.c:29:
In file included from include/linux/memblock.h:13:
In file included from arch/powerpc/include/asm/dma.h:22:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:217:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:577:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/stackdepot.c:29:
In file included from include/linux/memblock.h:13:
In file included from arch/powerpc/include/asm/dma.h:22:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:219:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:578:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/stackdepot.c:29:
In file included from include/linux/memblock.h:13:
In file included from arch/powerpc/include/asm/dma.h:22:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:221:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:579:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/stackdepot.c:29:
In file included from include/linux/memblock.h:13:
In file included from arch/powerpc/include/asm/dma.h:22:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:223:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:580:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/stackdepot.c:29:
In file included from include/linux/memblock.h:13:
In file included from arch/powerpc/include/asm/dma.h:22:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:225:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:581:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from lib/stackdepot.c:29:
In file included from include/linux/memblock.h:13:
In file included from arch/powerpc/include/asm/dma.h:22:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:227:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:582:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
>> lib/stackdepot.c:505:7: error: conflicting types for 'stack_start'
void *stack_start(struct seq_file *m, loff_t *ppos)
^
include/linux/stackdepot.h:120:7: note: previous declaration is here
void *stack_start(struct seq_file *m, loff_t *ppos);
^
>> lib/stackdepot.c:523:7: error: conflicting types for 'stack_next'
void *stack_next(struct seq_file *m, void *v, loff_t *ppos)
^
include/linux/stackdepot.h:121:7: note: previous declaration is here
void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
^
>> lib/stackdepot.c:552:5: error: conflicting types for 'stack_print'
int stack_print(struct seq_file *m, void *v)
^
include/linux/stackdepot.h:122:5: note: previous declaration is here
int stack_print(struct seq_file *m, void *v);
^
9 warnings and 3 errors generated.
--
In file included from arch/powerpc/kernel/align.c:17:
In file included from include/linux/mm.h:22:
In file included from include/linux/page_ext.h:7:
>> include/linux/stackdepot.h:120:26: error: declaration of 'struct seq_file' will not be visible outside of this function [-Werror,-Wvisibility]
void *stack_start(struct seq_file *m, loff_t *ppos);
^
include/linux/stackdepot.h:121:25: error: declaration of 'struct seq_file' will not be visible outside of this function [-Werror,-Wvisibility]
void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
^
include/linux/stackdepot.h:122:24: error: declaration of 'struct seq_file' will not be visible outside of this function [-Werror,-Wvisibility]
int stack_print(struct seq_file *m, void *v);
^
In file included from arch/powerpc/kernel/align.c:22:
In file included from arch/powerpc/include/asm/emulated_ops.h:10:
In file included from include/linux/perf_event.h:52:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:43:1: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:220:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:577:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/align.c:22:
In file included from arch/powerpc/include/asm/emulated_ops.h:10:
In file included from include/linux/perf_event.h:52:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:45:1: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:222:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:578:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/align.c:22:
In file included from arch/powerpc/include/asm/emulated_ops.h:10:
In file included from include/linux/perf_event.h:52:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:47:1: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:224:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:579:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/align.c:22:
In file included from arch/powerpc/include/asm/emulated_ops.h:10:
In file included from include/linux/perf_event.h:52:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:49:1: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:226:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:580:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
vim +/stack_start +505 lib/stackdepot.c
503
504 #ifdef CONFIG_PAGE_OWNER
> 505 void *stack_start(struct seq_file *m, loff_t *ppos)
506 {
507 unsigned long *table = m->private;
508 struct stack_record **stacks, *stack;
509
510 /* First time */
511 if (*ppos == 0)
512 *table = 0;
513
514 if (*ppos == -1UL)
515 return NULL;
516
517 stacks = &stack_table[*table];
518 stack = (struct stack_record *)stacks;
519
520 return stack;
521 }
522
> 523 void *stack_next(struct seq_file *m, void *v, loff_t *ppos)
524 {
525 unsigned long *table = m->private;
526 unsigned long nr_table = *table;
527 struct stack_record *next = NULL, *stack = v, **stacks;
528 unsigned long stack_table_entries = stack_hash_mask + 1;
529
530 if (!stack) {
531 new_table:
532 /* New table */
533 nr_table++;
534 if (nr_table >= stack_table_entries)
535 goto out;
536 stacks = &stack_table[nr_table];
537 stack = (struct stack_record *)stacks;
538 next = stack;
539 } else {
540 next = stack->next;
541 }
542
543 if (!next)
544 goto new_table;
545
546 out:
547 *table = nr_table;
548 *ppos = (nr_table >= stack_table_entries) ? -1UL : *ppos + 1;
549 return next;
550 }
551
> 552 int stack_print(struct seq_file *m, void *v)
553 {
554 char *buf;
555 int ret = 0;
556 struct stack_record *stack =v;
557
558 if (!stack->size || stack->size < 0 ||
559 stack->size > PAGE_SIZE || stack->handle.valid != 1 ||
560 refcount_read(&stack->count) < 1)
561 return 0;
562
563 buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
564 ret += stack_trace_snprint(buf, PAGE_SIZE, stack->entries, stack->size, 0);
565 scnprintf(buf + ret, PAGE_SIZE - ret, "stack count: %d\n\n",
566 refcount_read(&stack->count));
567 seq_printf(m, buf);
568 seq_puts(m, "\n\n");
569 kfree(buf);
570
571 return 0;
572 }
573 #endif
574
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Oscar,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc7]
[cannot apply to akpm-mm/mm-everything next-20230421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oscar-Salvador/lib-stackdepot-Add-a-refcount-field-in-stack_record/20230421-181709
base: linus/master
patch link: https://lore.kernel.org/r/20230421101415.5734-3-osalvador%40suse.de
patch subject: [PATCH v4 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counte
config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20230422/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
smatch warnings:
lib/stackdepot.c:558 stack_print() warn: unsigned 'stack->size' is never less than zero.
vim +558 lib/stackdepot.c
551
552 int stack_print(struct seq_file *m, void *v)
553 {
554 char *buf;
555 int ret = 0;
556 struct stack_record *stack =v;
557
> 558 if (!stack->size || stack->size < 0 ||
559 stack->size > PAGE_SIZE || stack->handle.valid != 1 ||
560 refcount_read(&stack->count) < 1)
561 return 0;
562
563 buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
564 ret += stack_trace_snprint(buf, PAGE_SIZE, stack->entries, stack->size, 0);
565 scnprintf(buf + ret, PAGE_SIZE - ret, "stack count: %d\n\n",
566 refcount_read(&stack->count));
567 seq_printf(m, buf);
568 seq_puts(m, "\n\n");
569 kfree(buf);
570
571 return 0;
572 }
573 #endif
574
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 2023-04-21 13:19, Alexander Potapenko wrote:
> I think the implementation of these counters is too specific to
> page_owner and is hard to use for any other purpose.
> If we decide to have them, there should be no page_owner-specific
> logic in the way we initialize/increment/decrement these counters.
Another solution would be to always increment the refcount in
__stack_depot_save,
in this case the "page-owner" specific changes are gone, and
it is more of a generic thing.
e.g: Andrey Konovalov mentioned that in a future KASAN remodelation,
he would be using a stack refcount as well.
> The thresholds in "mm,page_owner: Filter out stacks by a threshold
> counter" should also belong elsewhere.
That can certainly be cleaned up I guess to not polute non-page_owner
code.
> Given that no other stackdepot user needs these counters, maybe it
> should be cleaner to store an opaque struct along with the stack,
> passing its size to stack_depot_save(), and letting users access it
> directly using the stackdepot handler.
>
> I am also wondering if a separate hashtable mapping handlers to
> counters would solve the problem for you?
Let us see first if with the changes from above the code gets to a more
generic and clean stage, if not we can explore further options.
Thanks for your feedback Alexander!
--
Oscar Salvador
SUSE Labs
On Mon, 24 Apr 2023 05:54:59 +0200 Oscar Salvador <[email protected]> wrote:
> > Given that no other stackdepot user needs these counters, maybe it
> > should be cleaner to store an opaque struct along with the stack,
> > passing its size to stack_depot_save(), and letting users access it
> > directly using the stackdepot handler.
> >
> > I am also wondering if a separate hashtable mapping handlers to
> > counters would solve the problem for you?
>
> Let us see first if with the changes from above the code gets to a more
> generic and clean stage, if not we can explore further options.
Alexander, does this approach sound reasonable to you?
The overall feature seems useful, although I'm not seeing any positive
reviewer feedback.
On 6/9/23 23:55, Andrew Morton wrote:
> On Mon, 24 Apr 2023 05:54:59 +0200 Oscar Salvador <[email protected]> wrote:
>
>> > Given that no other stackdepot user needs these counters, maybe it
>> > should be cleaner to store an opaque struct along with the stack,
>> > passing its size to stack_depot_save(), and letting users access it
>> > directly using the stackdepot handler.
>> >
>> > I am also wondering if a separate hashtable mapping handlers to
>> > counters would solve the problem for you?
>>
>> Let us see first if with the changes from above the code gets to a more
>> generic and clean stage, if not we can explore further options.
>
> Alexander, does this approach sound reasonable to you?
Note this is a v4 thread; there was (and the version in mm-unstable is) v5,
where the latest was Alexander requesting further changes:
https://lore.kernel.org/all/CAG_fn%[email protected]/
> The overall feature seems useful, although I'm not seeing any positive
> reviewer feedback.