From: Andrey Konovalov <andreyknvl@google.com>
Add a reference counter for how many times a stack records has been added
to stack depot.
Do no yet decrement the refcount, this is implemented in one of the
following patches.
This is preparatory patch for implementing the eviction of stack records
from the stack depot.
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
lib/stackdepot.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 5ad454367379..a84c0debbb9e 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -22,6 +22,7 @@
#include <linux/mutex.h>
#include <linux/percpu.h>
#include <linux/printk.h>
+#include <linux/refcount.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/stacktrace.h>
@@ -60,6 +61,7 @@ struct stack_record {
u32 hash; /* Hash in hash table */
u32 size; /* Number of stored frames */
union handle_parts handle;
+ refcount_t count;
unsigned long entries[DEPOT_STACK_MAX_FRAMES]; /* Frames */
};
@@ -348,6 +350,7 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
stack->hash = hash;
stack->size = size;
/* stack->handle is already filled in by depot_init_pool. */
+ refcount_set(&stack->count, 1);
memcpy(stack->entries, entries, flex_array_size(stack, entries, size));
/*
@@ -452,6 +455,7 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
/* Fast path: look the stack trace up without full locking. */
found = find_stack(*bucket, entries, nr_entries, hash);
if (found) {
+ refcount_inc(&found->count);
read_unlock_irqrestore(&pool_rwlock, flags);
goto exit;
}
--
2.25.1On Tue, 2023-08-29 at 19:11 +0200, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Add a reference counter for how many times a stack records has been
> added
> to stack depot.
>
> Do no yet decrement the refcount, this is implemented in one of the
> following patches.
>
> This is preparatory patch for implementing the eviction of stack
> records
> from the stack depot.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
> lib/stackdepot.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 5ad454367379..a84c0debbb9e 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -22,6 +22,7 @@
> #include <linux/mutex.h>
> #include <linux/percpu.h>
> #include <linux/printk.h>
> +#include <linux/refcount.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/stacktrace.h>
> @@ -60,6 +61,7 @@ struct stack_record {
> u32 hash; /* Hash in hash table */
> u32 size; /* Number of stored frames */
> union handle_parts handle;
> + refcount_t count;
> unsigned long entries[DEPOT_STACK_MAX_FRAMES]; /* Frames */
> };
>
> @@ -348,6 +350,7 @@ depot_alloc_stack(unsigned long *entries, int
> size, u32 hash, void **prealloc)
> stack->hash = hash;
> stack->size = size;
> /* stack->handle is already filled in by depot_init_pool. */
> + refcount_set(&stack->count, 1);
> memcpy(stack->entries, entries, flex_array_size(stack, entries,
> size));
>
> /*
> @@ -452,6 +455,7 @@ depot_stack_handle_t __stack_depot_save(unsigned
> long *entries,
> /* Fast path: look the stack trace up without full locking. */
> found = find_stack(*bucket, entries, nr_entries, hash);
> if (found) {
> + refcount_inc(&found->count);
> read_unlock_irqrestore(&pool_rwlock, flags);
> goto exit;
> }
Hi Andrey,
There are two find_stack() function calls in __stack_depot_save().
Maybe we need to add refcount_inc() for both two find_stack()?
Thanks,
Kuan-Ying Lee
On Fri, Sep 1, 2023 at 3:06 PM 'Kuan-Ying Lee (李冠穎)' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> > @@ -452,6 +455,7 @@ depot_stack_handle_t __stack_depot_save(unsigned
> > long *entries,
> > /* Fast path: look the stack trace up without full locking. */
> > found = find_stack(*bucket, entries, nr_entries, hash);
> > if (found) {
> > + refcount_inc(&found->count);
> > read_unlock_irqrestore(&pool_rwlock, flags);
> > goto exit;
> > }
>
> Hi Andrey,
>
> There are two find_stack() function calls in __stack_depot_save().
>
> Maybe we need to add refcount_inc() for both two find_stack()?
Indeed, good catch! Will fix in v2.
Thanks!
On Tue, Aug 29, 2023 at 07:11PM +0200, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Add a reference counter for how many times a stack records has been added
> to stack depot.
>
> Do no yet decrement the refcount, this is implemented in one of the
> following patches.
>
> This is preparatory patch for implementing the eviction of stack records
> from the stack depot.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
> lib/stackdepot.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 5ad454367379..a84c0debbb9e 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -22,6 +22,7 @@
> #include <linux/mutex.h>
> #include <linux/percpu.h>
> #include <linux/printk.h>
> +#include <linux/refcount.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/stacktrace.h>
> @@ -60,6 +61,7 @@ struct stack_record {
> u32 hash; /* Hash in hash table */
> u32 size; /* Number of stored frames */
> union handle_parts handle;
> + refcount_t count;
> unsigned long entries[DEPOT_STACK_MAX_FRAMES]; /* Frames */
> };
>
> @@ -348,6 +350,7 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
> stack->hash = hash;
> stack->size = size;
> /* stack->handle is already filled in by depot_init_pool. */
> + refcount_set(&stack->count, 1);
> memcpy(stack->entries, entries, flex_array_size(stack, entries, size));
>
> /*
> @@ -452,6 +455,7 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
> /* Fast path: look the stack trace up without full locking. */
> found = find_stack(*bucket, entries, nr_entries, hash);
> if (found) {
> + refcount_inc(&found->count);
If someone doesn't use stack_depot_evict(), and the refcount eventually
overflows, it'll do a WARN (per refcount_warn_saturate()).
I think the interface needs to be different:
stack_depot_get(): increments refcount (could be inline if just
wrapper around refcount_inc())
stack_depot_put(): what stack_depot_evict() currently does
Then it's clear that if someone uses either stack_depot_get() or _put()
that these need to be balanced. Not using either will result in the old
behaviour of never evicting an entry.
> read_unlock_irqrestore(&pool_rwlock, flags);
> goto exit;
> }
> --
> 2.25.1
>
On Wed, Aug 30, 2023 at 11:33 AM Marco Elver <elver@google.com> wrote: > > If someone doesn't use stack_depot_evict(), and the refcount eventually > overflows, it'll do a WARN (per refcount_warn_saturate()). > > I think the interface needs to be different: > > stack_depot_get(): increments refcount (could be inline if just > wrapper around refcount_inc()) > > stack_depot_put(): what stack_depot_evict() currently does > > Then it's clear that if someone uses either stack_depot_get() or _put() > that these need to be balanced. Not using either will result in the old > behaviour of never evicting an entry. So you mean the exported interface needs to be different? And the users will need to call both stack_depot_save+stack_depot_get for saving? Hm, this seems odd. WDYT about adding a new flavor of stack_depot_save called stack_depot_save_get that would increment the refcount? And renaming stack_depot_evict to stack_depot_put. I'm not sure though if the overflow is actually an issue. Hitting that would require calling stack_depot_save INT_MAX times.
On Mon, 4 Sept 2023 at 20:46, Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Wed, Aug 30, 2023 at 11:33 AM Marco Elver <elver@google.com> wrote: > > > > If someone doesn't use stack_depot_evict(), and the refcount eventually > > overflows, it'll do a WARN (per refcount_warn_saturate()). > > > > I think the interface needs to be different: > > > > stack_depot_get(): increments refcount (could be inline if just > > wrapper around refcount_inc()) > > > > stack_depot_put(): what stack_depot_evict() currently does > > > > Then it's clear that if someone uses either stack_depot_get() or _put() > > that these need to be balanced. Not using either will result in the old > > behaviour of never evicting an entry. > > So you mean the exported interface needs to be different? And the > users will need to call both stack_depot_save+stack_depot_get for > saving? Hm, this seems odd. > > WDYT about adding a new flavor of stack_depot_save called > stack_depot_save_get that would increment the refcount? And renaming > stack_depot_evict to stack_depot_put. If there are no other uses of stack_depot_get(), which seems likely, just stack_depot_save_get() seems ok. > I'm not sure though if the overflow is actually an issue. Hitting that > would require calling stack_depot_save INT_MAX times. With a long-running kernel it's possible.
On Mon, Sep 4, 2023 at 8:56 PM Marco Elver <elver@google.com> wrote: > > > WDYT about adding a new flavor of stack_depot_save called > > stack_depot_save_get that would increment the refcount? And renaming > > stack_depot_evict to stack_depot_put. > > If there are no other uses of stack_depot_get(), which seems likely, > just stack_depot_save_get() seems ok. Ok, I will implement a similar approach in v2: add another flag to __stack_depot_save to avoid multiplying API functions. Thanks!
© 2016 - 2025 Red Hat, Inc.