* [PATCH] stackdepot: Rename pool_index to pool_index_plus_1
@ 2024-04-02 0:14 Peter Collingbourne
2024-04-02 6:54 ` Vlastimil Babka
2024-04-02 8:01 ` Oscar Salvador
0 siblings, 2 replies; 5+ messages in thread
From: Peter Collingbourne @ 2024-04-02 0:14 UTC (permalink / raw
To: Andrey Konovalov, Oscar Salvador
Cc: Peter Collingbourne, Andrew Morton, linux-kernel, linux-mm,
Michal Hocko, Vlastimil Babka, Marco Elver, Alexander Potapenko,
Omar Sandoval
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.
Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/Ib3e70c36c1d230dd0a118dc22649b33e768b9f88
---
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(-)
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index 3c6caa5abc7c..e9ec32fb97d4 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -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;
};
};
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index af6cc19a2003..68c97387aa54 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -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;
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] stackdepot: Rename pool_index to pool_index_plus_1
2024-04-02 0:14 [PATCH] stackdepot: Rename pool_index to pool_index_plus_1 Peter Collingbourne
@ 2024-04-02 6:54 ` Vlastimil Babka
2024-04-02 7:04 ` Alexander Potapenko
2024-04-02 8:01 ` Oscar Salvador
1 sibling, 1 reply; 5+ messages in thread
From: Vlastimil Babka @ 2024-04-02 6:54 UTC (permalink / raw
To: Peter Collingbourne, Andrey Konovalov, Oscar Salvador
Cc: Andrew Morton, linux-kernel, linux-mm, Michal Hocko, Marco Elver,
Alexander Potapenko, Omar Sandoval
On 4/2/24 2:14 AM, Peter Collingbourne wrote:
> 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.
>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/Ib3e70c36c1d230dd0a118dc22649b33e768b9f88
Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> 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(-)
>
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 3c6caa5abc7c..e9ec32fb97d4 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -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;
> };
> };
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index af6cc19a2003..68c97387aa54 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] stackdepot: Rename pool_index to pool_index_plus_1
2024-04-02 6:54 ` Vlastimil Babka
@ 2024-04-02 7:04 ` Alexander Potapenko
2024-04-02 7:08 ` Marco Elver
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Potapenko @ 2024-04-02 7:04 UTC (permalink / raw
To: Vlastimil Babka
Cc: Peter Collingbourne, Andrey Konovalov, Oscar Salvador,
Andrew Morton, linux-kernel, linux-mm, Michal Hocko, Marco Elver,
Omar Sandoval
On Tue, Apr 2, 2024 at 8:54 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 4/2/24 2:14 AM, Peter Collingbourne wrote:
> > 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.
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Link: https://linux-review.googlesource.com/id/Ib3e70c36c1d230dd0a118dc22649b33e768b9f88
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Alexander Potapenko <glider@google.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] stackdepot: Rename pool_index to pool_index_plus_1
2024-04-02 7:04 ` Alexander Potapenko
@ 2024-04-02 7:08 ` Marco Elver
0 siblings, 0 replies; 5+ messages in thread
From: Marco Elver @ 2024-04-02 7:08 UTC (permalink / raw
To: Alexander Potapenko
Cc: Vlastimil Babka, Peter Collingbourne, Andrey Konovalov,
Oscar Salvador, Andrew Morton, linux-kernel, linux-mm,
Michal Hocko, Omar Sandoval
On Tue, 2 Apr 2024 at 09:04, Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Apr 2, 2024 at 8:54 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >
> > On 4/2/24 2:14 AM, Peter Collingbourne wrote:
> > > 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.
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > Link: https://linux-review.googlesource.com/id/Ib3e70c36c1d230dd0a118dc22649b33e768b9f88
> >
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Reviewed-by: Alexander Potapenko <glider@google.com>
Acked-by: Marco Elver <elver@google.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] stackdepot: Rename pool_index to pool_index_plus_1
2024-04-02 0:14 [PATCH] stackdepot: Rename pool_index to pool_index_plus_1 Peter Collingbourne
2024-04-02 6:54 ` Vlastimil Babka
@ 2024-04-02 8:01 ` Oscar Salvador
1 sibling, 0 replies; 5+ messages in thread
From: Oscar Salvador @ 2024-04-02 8:01 UTC (permalink / raw
To: Peter Collingbourne
Cc: Andrey Konovalov, Andrew Morton, linux-kernel, linux-mm,
Michal Hocko, Vlastimil Babka, Marco Elver, Alexander Potapenko,
Omar Sandoval
On Mon, Apr 01, 2024 at 05:14:58PM -0700, Peter Collingbourne wrote:
> 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.
>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/Ib3e70c36c1d230dd0a118dc22649b33e768b9f88
Acked-by: Oscar Salvador <osalvador@suse.de>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-02 7:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-02 0:14 [PATCH] stackdepot: Rename pool_index to pool_index_plus_1 Peter Collingbourne
2024-04-02 6:54 ` Vlastimil Babka
2024-04-02 7:04 ` Alexander Potapenko
2024-04-02 7:08 ` Marco Elver
2024-04-02 8:01 ` Oscar Salvador
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).