stackdepot: Rename pool_index to pool_index_plus_1
Subsystems (2)
PATCH 1/1 stackdepot: Rename pool_index to pool_index_plus_1
Commit 3ee34eabac2a ("lib/stackdepot: fix first entry having a 0-handle") changed the meaning of the pool_index field to mean "the pool index plus 1". This made the code accessing this field less self-documenting, as well as causing debuggers such as drgn to not be able to easily remain compatible with both old and new kernels, because they typically do that by testing for presence of the new field. Because stackdepot is a debugging tool, we should make sure that it is debugger friendly. Therefore, give the field a different name to improve readability as well as enabling debugger backwards compatibility.
---
Although this technically isn't a bug fix in the kernel, I would appreciate if this could be picked up for 6.9 to avoid temporarily introducing a silent regression in drgn (loud regressions are fine).
include/linux/stackdepot.h | 7 +++---- lib/stackdepot.c | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-)
include/linux/stackdepot.h +3 -4
@@ -44,10 +44,9 @@ typedef u32 depot_stack_handle_t;union handle_parts {depot_stack_handle_t handle;struct { -/* pool_index is offset by 1 */ -u32 pool_index :DEPOT_POOL_INDEX_BITS ; -u32 offset :DEPOT_OFFSET_BITS ; -u32 extra :STACK_DEPOT_EXTRA_BITS ; +u32 pool_index_plus_1 :DEPOT_POOL_INDEX_BITS ; +u32 offset :DEPOT_OFFSET_BITS ; +u32 extra :STACK_DEPOT_EXTRA_BITS ; }; };
lib/stackdepot.c +2 -2
@@ -330,7 +330,7 @@ static struct stack_record *depot_pop_free_pool(void **prealloc, size_t size) stack = current_pool + pool_offset;/* Pre-initialize handle once. */ - stack->handle.pool_index = pool_index +1 ; + stack->handle.pool_index_plus_1 = pool_index +1 ; stack->handle.offset = pool_offset >>DEPOT_STACK_ALIGN ; stack->handle.extra =0 ;INIT_LIST_HEAD (&stack->hash_list);
@@ -441,7 +441,7 @@ static struct stack_record *depot_fetch_stack(depot_stack_handle_t handle)const int pools_num_cached =READ_ONCE (pools_num);union handle_parts parts = { .handle = handle };void *pool; -u32 pool_index = parts.pool_index -1 ; +u32 pool_index = parts.pool_index_plus_1 -1 ;size_t offset = parts.offset <<DEPOT_STACK_ALIGN ;struct stack_record *stack;
Signature · 1 line
2.44.0.478.gd926399ef9-goog
Quoted 11 lines · Peter Collingbourne
Commit
3ee34eabac2a("lib/stackdepot: fix first entry having a 0-handle") changed the meaning of thepool_indexfield to mean "the pool index plus 1". This made the code accessing this field less self-documenting, as well as causing debuggers such as drgn to not be able to easily remain compatible with both old and new kernels, because they typically do that by testing for presence of the new field. Because stackdepot is a debugging tool, we should make sure that it is debugger friendly. Therefore, give the field a different name to improve readability as well as enabling debugger backwards compatibility.
Quoted 2 lines
Quoted 50 lines
---
Although this technically isn't a bug fix in the kernel, I would appreciate if this could be picked up for 6.9 to avoid temporarily introducing a silent regression in drgn (loud regressions are fine).include/linux/stackdepot.h | 7 +++---- lib/stackdepot.c | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-)include/linux/stackdepot.h +3
-4@@ -44,10 +44,9 @@ typedef u32 depot_stack_handle_t;union handle_parts {depot_stack_handle_t handle;struct { -/* pool_index is offset by 1 */ -u32 pool_index :DEPOT_POOL_INDEX_BITS ; -u32 offset :DEPOT_OFFSET_BITS ; -u32 extra :STACK_DEPOT_EXTRA_BITS ; +u32 pool_index_plus_1 :DEPOT_POOL_INDEX_BITS ; +u32 offset :DEPOT_OFFSET_BITS ; +u32 extra :STACK_DEPOT_EXTRA_BITS ; }; };lib/stackdepot.c +2
-2@@ -330,7 +330,7 @@ static struct stack_record *depot_pop_free_pool(void **prealloc, size_t size) stack = current_pool + pool_offset;/* Pre-initialize handle once. */ - stack->handle.pool_index = pool_index +1 ; + stack->handle.pool_index_plus_1 = pool_index +1 ; stack->handle.offset = pool_offset >>DEPOT_STACK_ALIGN ; stack->handle.extra =0 ;INIT_LIST_HEAD (&stack->hash_list);@@ -441,7 +441,7 @@ static struct stack_record *depot_fetch_stack(depot_stack_handle_t handle)const int pools_num_cached =READ_ONCE (pools_num);union handle_parts parts = { .handle = handle };void *pool; -u32 pool_index = parts.pool_index -1 ; +u32 pool_index = parts.pool_index_plus_1 -1 ;size_t offset = parts.offset <<DEPOT_STACK_ALIGN ;struct stack_record *stack;
Alexander Potapenko
Reviewed-byQuoted 13 lines · Vlastimil Babka
Quoted 11 lines · Peter Collingbourne
Commit
3ee34eabac2a("lib/stackdepot: fix first entry having a 0-handle") changed the meaning of thepool_indexfield to mean "the pool index plus 1". This made the code accessing this field less self-documenting, as well as causing debuggers such as drgn to not be able to easily remain compatible with both old and new kernels, because they typically do that by testing for presence of the new field. Because stackdepot is a debugging tool, we should make sure that it is debugger friendly. Therefore, give the field a different name to improve readability as well as enabling debugger backwards compatibility.
Quoted 4 lines
Quoted 2 lines
Acked-by Vlastimil Babka <[email protected]>
Marco Elver
Acked-byQuoted 15 lines · Alexander Potapenko
Quoted 13 lines · Vlastimil Babka
Quoted 11 lines · Peter Collingbourne
Commit
3ee34eabac2a("lib/stackdepot: fix first entry having a 0-handle") changed the meaning of thepool_indexfield to mean "the pool index plus 1". This made the code accessing this field less self-documenting, as well as causing debuggers such as drgn to not be able to easily remain compatible with both old and new kernels, because they typically do that by testing for presence of the new field. Because stackdepot is a debugging tool, we should make sure that it is debugger friendly. Therefore, give the field a different name to improve readability as well as enabling debugger backwards compatibility.
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Acked-by Vlastimil Babka <[email protected]>Reviewed-by Alexander Potapenko <[email protected]>
Quoted 11 lines · Peter Collingbourne
Commit
3ee34eabac2a("lib/stackdepot: fix first entry having a 0-handle") changed the meaning of thepool_indexfield to mean "the pool index plus 1". This made the code accessing this field less self-documenting, as well as causing debuggers such as drgn to not be able to easily remain compatible with both old and new kernels, because they typically do that by testing for presence of the new field. Because stackdepot is a debugging tool, we should make sure that it is debugger friendly. Therefore, give the field a different name to improve readability as well as enabling debugger backwards compatibility.
Quoted 2 lines
Signature · 2 lines
Oscar Salvador SUSE Labs