[05/15] stackdepot: use fixed-sized slots for stack records

Commit Message

[e-mail] Aug. 29, 2023, 5:11 p.m. UTC

From: Andrey Konovalov <[e-mail]>

Instead of storing stack records in stack depot pools one right after
another, use 32-frame-sized slots.

This is preparatory patch for implementing the eviction of stack records
from the stack depot.

Signed-off-by: Andrey Konovalov <[e-mail]>
---
 lib/stackdepot.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Alexander Potapenko Aug. 30, 2023, 8:21 a.m. UTC | #1

On Tue, Aug 29, 2023 at 7:11 PM <[e-mail]> wrote:
>
> From: Andrey Konovalov <[e-mail]>
>
> Instead of storing stack records in stack depot pools one right after
> another, use 32-frame-sized slots.

I am slightly concerned about the KMSAN use case here, which defines
KMSAN_STACK_DEPTH to 64.
I don't have a comprehensive stack depth breakdown, but a quick poking
around syzkaller.appspot.com shows several cases where the stacks are
actually longer than 32 frames.
Can you add a config parameter for the stack depth instead of
mandating 32 frames everywhere?

As a side note, kmsan_internal_chain_origin()
(https://elixir.bootlin.com/linux/latest/source/mm/kmsan/core.c#L214)
creates small 3-frame records in the stack depot to link two stacks
together, which will add unnecessary stackdepot pressure.
But this can be fixed by storing both the new stack trace and the link
to the old stack trace in the same record.

Andrey Konovalov Sept. 13, 2023, 5:07 p.m. UTC | #2

On Wed, Aug 30, 2023 at 10:22 AM Alexander Potapenko <[e-mail]> wrote:
>
> On Tue, Aug 29, 2023 at 7:11 PM <[e-mail]> wrote:
> >
> > From: Andrey Konovalov <[e-mail]>
> >
> > Instead of storing stack records in stack depot pools one right after
> > another, use 32-frame-sized slots.
>
> I am slightly concerned about the KMSAN use case here, which defines
> KMSAN_STACK_DEPTH to 64.

Hm, indeed. KASAN also defines the depth to 64 actually.

I think it's reasonable to change the default value to 64 to cover all
the existing users. And whoever wants to save up on memory can change
the Kconfig parameter (I'll add one as you suggested).

> I don't have a comprehensive stack depth breakdown, but a quick poking
> around syzkaller.appspot.com shows several cases where the stacks are
> actually longer than 32 frames.

Whichever value we choose, some of stack traces will not fit
unfortunately. But yeah, 64 seems to be a more reasonable value.

> Can you add a config parameter for the stack depth instead of
> mandating 32 frames everywhere?

Sure, will do in v2.

> As a side note, kmsan_internal_chain_origin()
> (https://elixir.bootlin.com/linux/latest/source/mm/kmsan/core.c#L214)
> creates small 3-frame records in the stack depot to link two stacks
> together, which will add unnecessary stackdepot pressure.
> But this can be fixed by storing both the new stack trace and the link
> to the old stack trace in the same record.

Do you mean this can be fixed in KMSAN? Or do you mean some kind of an
extension to the stack depot interface?

Alexander Potapenko Sept. 15, 2023, 10:36 a.m. UTC | #3

> > As a side note, kmsan_internal_chain_origin()
> > (https://elixir.bootlin.com/linux/latest/source/mm/kmsan/core.c#L214)
> > creates small 3-frame records in the stack depot to link two stacks
> > together, which will add unnecessary stackdepot pressure.
> > But this can be fixed by storing both the new stack trace and the link
> > to the old stack trace in the same record.
>
> Do you mean this can be fixed in KMSAN? Or do you mean some kind of an
> extension to the stack depot interface?

Yes, I'll just fix this on the KMSAN side.

Patch

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 2128108f2acb..93191ee70fc3 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -42,6 +42,7 @@ 
 #define DEPOT_MAX_POOLS \
 	(((1LL << (DEPOT_POOL_INDEX_BITS)) < DEPOT_POOLS_CAP) ? \
 	 (1LL << (DEPOT_POOL_INDEX_BITS)) : DEPOT_POOLS_CAP)
+#define DEPOT_STACK_MAX_FRAMES 32
 
 /* Compact structure that stores a reference to a stack. */
 union handle_parts {
@@ -58,9 +59,12 @@  struct stack_record {
 	u32 hash;			/* Hash in the hash table */
 	u32 size;			/* Number of stored frames */
 	union handle_parts handle;
-	unsigned long entries[];	/* Variable-sized array of frames */
+	unsigned long entries[DEPOT_STACK_MAX_FRAMES];	/* Frames */
 };
 
+#define DEPOT_STACK_RECORD_SIZE \
+		ALIGN(sizeof(struct stack_record), 1 << DEPOT_STACK_ALIGN)
+
 static bool stack_depot_disabled;
 static bool __stack_depot_early_init_requested __initdata = IS_ENABLED(CONFIG_STACKDEPOT_ALWAYS_INIT);
 static bool __stack_depot_early_init_passed __initdata;
@@ -258,9 +262,7 @@  static struct stack_record *
 depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
 {
 	struct stack_record *stack;
-	size_t required_size = struct_size(stack, entries, size);
-
-	required_size = ALIGN(required_size, 1 << DEPOT_STACK_ALIGN);
+	size_t required_size = DEPOT_STACK_RECORD_SIZE;
 
 	/* Check if there is not enough space in the current pool. */
 	if (unlikely(pool_offset + required_size > DEPOT_POOL_SIZE)) {
@@ -295,6 +297,10 @@  depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
 	if (stack_pools[pool_index] == NULL)
 		return NULL;
 
+	/* Limit number of saved frames to DEPOT_STACK_MAX_FRAMES. */
+	if (size > DEPOT_STACK_MAX_FRAMES)
+		size = DEPOT_STACK_MAX_FRAMES;
+
 	/* Save the stack trace. */
 	stack = stack_pools[pool_index] + pool_offset;
 	stack->hash = hash;