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 | 1 +
lib/stackdepot.c | 40 ++++++++++++++++++++++++++++++++++++++
mm/page_owner.c | 25 ++++++++++++++++++++++++
3 files changed, 66 insertions(+)
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index 4e3a88f135ee..19d3f8295df8 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -25,6 +25,7 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
gfp_t gfp_flags, bool can_alloc,
enum stack_depot_action action);
void stack_depot_dec_count(depot_stack_handle_t handle);
+int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos);
/*
* Every user of stack depot has to call stack_depot_init() during its own init
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index a806ef58a385..a198b2dbe3fb 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -565,3 +565,43 @@ depot_stack_handle_t stack_depot_save_action(unsigned long *entries,
return __stack_depot_save(entries, nr_entries, alloc_flags, true, action);
}
EXPORT_SYMBOL_GPL(stack_depot_save_action);
+
+int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
+{
+ int i = *pos, ret = 0;
+ struct stack_record **stacks, *stack;
+ static struct stack_record *last = NULL;
+ unsigned long stack_table_entries = stack_hash_mask + 1;
+
+ /* Continue from the last stack if we have one */
+ if (last) {
+ stack = last->next;
+ } else {
+new_table:
+ stacks = &stack_table[i];
+ stack = (struct stack_record *)stacks;
+ }
+
+ for (; stack; stack = stack->next) {
+ if (!stack->size || stack->size < 0 ||
+ stack->size > size || stack->handle.valid != 1 ||
+ refcount_read(&stack->count) < 1)
+ continue;
+
+ ret += stack_trace_snprint(buf, size, stack->entries, stack->size, 0);
+ ret += scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
+ refcount_read(&stack->count));
+ last = stack;
+ return ret;
+ }
+
+ i++;
+ *pos = i;
+ last = NULL;
+
+ /* Keep looking all tables for valid stacks */
+ if (i < stack_table_entries)
+ goto new_table;
+
+ return 0;
+}
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 8730f377fa91..d88e6b4aefa0 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -664,6 +664,29 @@ static void init_early_allocated_pages(void)
init_zones_in_node(pgdat);
}
+static ssize_t read_page_owner_stacks(struct file *file, char __user *buf,
+ size_t count, loff_t *pos)
+{
+ char *kbuf;
+ int ret = 0;
+
+ count = min_t(size_t, count, PAGE_SIZE);
+ kbuf = kmalloc(count, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ ret += stack_depot_print_stacks_threshold(kbuf, count, pos);
+ if (copy_to_user(buf, kbuf, ret))
+ ret = -EFAULT;
+
+ kfree(kbuf);
+ return ret;
+}
+
+static const struct file_operations proc_page_owner_stacks = {
+ .read = read_page_owner_stacks,
+};
+
static const struct file_operations proc_page_owner_operations = {
.read = read_page_owner,
};
@@ -677,6 +700,8 @@ static int __init pageowner_init(void)
debugfs_create_file("page_owner", 0400, NULL, NULL,
&proc_page_owner_operations);
+ debugfs_create_file("page_owner_stacks", 0400, NULL, NULL,
+ &proc_page_owner_stacks);
return 0;
}
--
2.35.3
On Mon, Sep 05, 2022 at 05:10AM +0200, Oscar Salvador wrote:
[...]
> +int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
Can you add kernel-doc comment what this does (and also update
accordingly in 3/3 when you add 'threshold').
From what I see it prints *all* stacks that have a non-zero count.
Correct?
If so, should this be called stack_depot_print_all_count() (having
stack(s) in the name twice doesn't make it more obvious what it does)?
Then in the follow-up patch you add the 'threshold' arg.
> +{
> + int i = *pos, ret = 0;
> + struct stack_record **stacks, *stack;
> + static struct stack_record *last = NULL;
> + unsigned long stack_table_entries = stack_hash_mask + 1;
> +
> + /* Continue from the last stack if we have one */
> + if (last) {
> + stack = last->next;
This is dead code?
> + } else {
> +new_table:
> + stacks = &stack_table[i];
> + stack = (struct stack_record *)stacks;
> + }
> +
> + for (; stack; stack = stack->next) {
> + if (!stack->size || stack->size < 0 ||
> + stack->size > size || stack->handle.valid != 1 ||
> + refcount_read(&stack->count) < 1)
> + continue;
> +
> + ret += stack_trace_snprint(buf, size, stack->entries, stack->size, 0);
> + ret += scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
> + refcount_read(&stack->count));
> + last = stack;
> + return ret;
> + }
> +
> + i++;
> + *pos = i;
> + last = NULL;
> +
> + /* Keep looking all tables for valid stacks */
> + if (i < stack_table_entries)
> + goto new_table;
> +
> + return 0;
> +}
Either I'm missing something really obvious, but I was able to simplify
the above function to just this (untested!):
int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
{
const unsigned long stack_table_entries = stack_hash_mask + 1;
/* Iterate over all tables for valid stacks. */
for (; *pos < stack_table_entries; (*pos)++) {
for (struct stack_record *stack = stack_table[*pos]; stack; stack = stack->next) {
if (!stack->size || stack->size < 0 || stack->size > size ||
stack->handle.valid != 1 || refcount_read(&stack->count) < 1)
continue;
return stack_trace_snprint(buf, size, stack->entries, stack->size, 0) +
scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
refcount_read(&stack->count));
}
}
return 0;
}
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 8730f377fa91..d88e6b4aefa0 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -664,6 +664,29 @@ static void init_early_allocated_pages(void)
> init_zones_in_node(pgdat);
> }
>
> +static ssize_t read_page_owner_stacks(struct file *file, char __user *buf,
> + size_t count, loff_t *pos)
> +{
> + char *kbuf;
> + int ret = 0;
> +
> + count = min_t(size_t, count, PAGE_SIZE);
> + kbuf = kmalloc(count, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + ret += stack_depot_print_stacks_threshold(kbuf, count, pos);
If I understood right, this will print *all* stacks that have non-zero
count, and this isn't related to page_owner per-se. Correct?
This might not be a problem right now, but once there might be more
users that want to count stack usage, you'll end up with page_owner +
other stacks here.
On Mon, Sep 05, 2022 at 02:57PM +0200, Marco Elver wrote:
[...]
> > +{
> > + int i = *pos, ret = 0;
> > + struct stack_record **stacks, *stack;
> > + static struct stack_record *last = NULL;
> > + unsigned long stack_table_entries = stack_hash_mask + 1;
> > +
> > + /* Continue from the last stack if we have one */
> > + if (last) {
> > + stack = last->next;
>
> This is dead code?
Oof, I just noticed that 'last' is static. Please avoid that, because
it'll make this interface really tricky to use safely. I still don't
quite understand why it needs to do this, and a kernel-doc comment would
make this clearer.
Hi Oscar,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.0-rc4]
[cannot apply to akpm-mm/mm-everything next-20220901]
[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/page_owner-print-stacks-and-their-counter/20220905-111139
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7e18e42e4b280c85b76967a9106a13ca61c16179
config: microblaze-randconfig-m041-20220905 (https://download.01.org/0day-ci/archive/20220906/[email protected]/config)
compiler: microblaze-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
smatch warnings:
lib/stackdepot.c:586 stack_depot_print_stacks_threshold() warn: unsigned 'stack->size' is never less than zero.
vim +586 lib/stackdepot.c
568
569 int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
570 {
571 int i = *pos, ret = 0;
572 struct stack_record **stacks, *stack;
573 static struct stack_record *last = NULL;
574 unsigned long stack_table_entries = stack_hash_mask + 1;
575
576 /* Continue from the last stack if we have one */
577 if (last) {
578 stack = last->next;
579 } else {
580 new_table:
581 stacks = &stack_table[i];
582 stack = (struct stack_record *)stacks;
583 }
584
585 for (; stack; stack = stack->next) {
> 586 if (!stack->size || stack->size < 0 ||
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Mon, Sep 05, 2022 at 02:57:50PM +0200, Marco Elver wrote:
> On Mon, Sep 05, 2022 at 05:10AM +0200, Oscar Salvador wrote:
> [...]
> > +int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
>
> Can you add kernel-doc comment what this does (and also update
> accordingly in 3/3 when you add 'threshold').
Yes, I guess a kernel-doc comment is due.
> From what I see it prints *all* stacks that have a non-zero count.
> Correct?
That's right.
> If so, should this be called stack_depot_print_all_count() (having
> stack(s) in the name twice doesn't make it more obvious what it does)?
> Then in the follow-up patch you add the 'threshold' arg.
I guess so. The only reason I went with the actual name is that for me
"stack_depot" was kinda the name of the module/library, and
so I wanted to make crystal clear what were we printing.
But I'm ok with renaming it if it's already self-explanatory
> > +{
> > + int i = *pos, ret = 0;
> > + struct stack_record **stacks, *stack;
> > + static struct stack_record *last = NULL;
> > + unsigned long stack_table_entries = stack_hash_mask + 1;
> > +
> > + /* Continue from the last stack if we have one */
> > + if (last) {
> > + stack = last->next;
>
> This is dead code?
No, more below.
> Either I'm missing something really obvious, but I was able to simplify
> the above function to just this (untested!):
>
> int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
> {
> const unsigned long stack_table_entries = stack_hash_mask + 1;
>
> /* Iterate over all tables for valid stacks. */
> for (; *pos < stack_table_entries; (*pos)++) {
> for (struct stack_record *stack = stack_table[*pos]; stack; stack = stack->next) {
> if (!stack->size || stack->size < 0 || stack->size > size ||
> stack->handle.valid != 1 || refcount_read(&stack->count) < 1)
> continue;
>
> return stack_trace_snprint(buf, size, stack->entries, stack->size, 0) +
> scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
> refcount_read(&stack->count));
> }
> }
>
> return 0;
Yes, this will not work.
You have stack_table[] which is an array for struct stacks, and each struct
stack has a pointer to its next stack which walks from the beginning fo a specific
table till the end. e.g:
stack_table[0] = {stack1, stack2, stack3, ...} (each linked by ->next)
stack_table[1] = {stack1, stack2, stack3, ...} (each linked by ->next)
..
stack_table[stack_table_entries - 1] = {stack1, stack2, stack3, ...} (each linked by ->next)
*pos holds the index of stack_table[], while "last" holds the last stack within
the table we were processing.
So, when we find a valid stack to print, set "last" to that stack, and *pos to the index
of stack_table.
So, when we call stack_depot_print_stacks_threshold() again, we set "stack" to "last"->next,
and we are ready to keep looking with:
for (; stack; stack = stack->next) {
...
check if stack is valid
}
Should not we find any more valid stacks in that stack_table, we need to check in
the next table, so we do::
i++; (note that i was set to *pos at the beginning of the function)
*pos = i;
last = NULL;
goto new_table
and now are ready to do:
new_table:
stacks = &stack_table[i];
stack = (struct stack_record *)stacks;
Does this clarify it a little bit?
About using static vs non-static.
In the v1, I was using a parameter which contained last_stack:
https://patchwork.kernel.org/project/linux-mm/patch/[email protected]/
Not sure if that's better? Thoughts?
--
Oscar Salvador
SUSE Labs
On Tue, 6 Sept 2022 at 09:44, Oscar Salvador <[email protected]> wrote:
>
> On Mon, Sep 05, 2022 at 02:57:50PM +0200, Marco Elver wrote:
> > On Mon, Sep 05, 2022 at 05:10AM +0200, Oscar Salvador wrote:
> > [...]
> > > +int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
> >
> > Can you add kernel-doc comment what this does (and also update
> > accordingly in 3/3 when you add 'threshold').
>
> Yes, I guess a kernel-doc comment is due.
>
> > From what I see it prints *all* stacks that have a non-zero count.
> > Correct?
>
> That's right.
>
> > If so, should this be called stack_depot_print_all_count() (having
> > stack(s) in the name twice doesn't make it more obvious what it does)?
> > Then in the follow-up patch you add the 'threshold' arg.
>
> I guess so. The only reason I went with the actual name is that for me
> "stack_depot" was kinda the name of the module/library, and
> so I wanted to make crystal clear what were we printing.
>
> But I'm ok with renaming it if it's already self-explanatory
I think it's clear from the fact we're using the stack depot that any
printing will print stacks. To mirror the existing
'stack_depot_print()', I'd go with 'stack_depot_print_all_count()'.
> > > +{
> > > + int i = *pos, ret = 0;
> > > + struct stack_record **stacks, *stack;
> > > + static struct stack_record *last = NULL;
> > > + unsigned long stack_table_entries = stack_hash_mask + 1;
> > > +
> > > + /* Continue from the last stack if we have one */
> > > + if (last) {
> > > + stack = last->next;
> >
> > This is dead code?
>
> No, more below.
>
> > Either I'm missing something really obvious, but I was able to simplify
> > the above function to just this (untested!):
> >
> > int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos)
> > {
> > const unsigned long stack_table_entries = stack_hash_mask + 1;
> >
> > /* Iterate over all tables for valid stacks. */
> > for (; *pos < stack_table_entries; (*pos)++) {
> > for (struct stack_record *stack = stack_table[*pos]; stack; stack = stack->next) {
> > if (!stack->size || stack->size < 0 || stack->size > size ||
> > stack->handle.valid != 1 || refcount_read(&stack->count) < 1)
> > continue;
> >
> > return stack_trace_snprint(buf, size, stack->entries, stack->size, 0) +
> > scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
> > refcount_read(&stack->count));
> > }
> > }
> >
> > return 0;
>
> Yes, this will not work.
>
> You have stack_table[] which is an array for struct stacks, and each struct
> stack has a pointer to its next stack which walks from the beginning fo a specific
> table till the end. e.g:
>
> stack_table[0] = {stack1, stack2, stack3, ...} (each linked by ->next)
> stack_table[1] = {stack1, stack2, stack3, ...} (each linked by ->next)
> ..
> stack_table[stack_table_entries - 1] = {stack1, stack2, stack3, ...} (each linked by ->next)
>
> *pos holds the index of stack_table[], while "last" holds the last stack within
> the table we were processing.
>
> So, when we find a valid stack to print, set "last" to that stack, and *pos to the index
> of stack_table.
> So, when we call stack_depot_print_stacks_threshold() again, we set "stack" to "last"->next,
> and we are ready to keep looking with:
>
> for (; stack; stack = stack->next) {
> ...
> check if stack is valid
> }
>
> Should not we find any more valid stacks in that stack_table, we need to check in
> the next table, so we do::
>
> i++; (note that i was set to *pos at the beginning of the function)
> *pos = i;
> last = NULL;
> goto new_table
>
> and now are ready to do:
>
> new_table:
> stacks = &stack_table[i];
> stack = (struct stack_record *)stacks;
>
>
> Does this clarify it a little bit?
>
> About using static vs non-static.
> In the v1, I was using a parameter which contained last_stack:
>
> https://patchwork.kernel.org/project/linux-mm/patch/[email protected]/
>
> Not sure if that's better? Thoughts?
Moderately better, but still not great. Essentially you need 2
cursors, but with loff_t you only get 1.
I think the loff_t parameter can be used to encode both cursors. In
the kernel, loff_t is always 'long long', so it'll always be 64-bit.
Let's assume that collisions in the hash table are rare, so the number
of stacks per bucket are typically small. Then you can encode the
index into the bucket in bits 0-31 and the bucket index in bits 32-63.
STACK_HASH_ORDER_MAX is 20, so 32 bits is plenty to encode the index.
On Tue, Sep 06, 2022 at 10:35:00AM +0200, Marco Elver wrote:
> I think it's clear from the fact we're using the stack depot that any
> printing will print stacks. To mirror the existing
> 'stack_depot_print()', I'd go with 'stack_depot_print_all_count()'.
Fair enough, I will rename it then.
> Moderately better, but still not great. Essentially you need 2
> cursors, but with loff_t you only get 1.
>
> I think the loff_t parameter can be used to encode both cursors. In
> the kernel, loff_t is always 'long long', so it'll always be 64-bit.
>
> Let's assume that collisions in the hash table are rare, so the number
> of stacks per bucket are typically small. Then you can encode the
> index into the bucket in bits 0-31 and the bucket index in bits 32-63.
> STACK_HASH_ORDER_MAX is 20, so 32 bits is plenty to encode the index.
I see, I didn't think of it to be honest.
Then, the below (completely untested) should the trick:
<----
int stack_depot_print_all_count(char *buf, size_t size, loff_t *pos)
{
int ret = 0, stack_i, table_i;
struct stack_record **stacks, *stack;
unsigned long stack_table_entries = stack_hash_mask + 1;
stack_i = (*pos & 31);
table_i = (*pos >> 32);
new_table:
stacks = &stack_table[table_i];
stack = ((struct stack_record *)stacks) + stack_i;
for (; stack; stack = stack->next, stack_i++) {
if (!stack->size || stack->size < 0 ||
stack->size > size || stack->handle.valid != 1 ||
refcount_read(&stack->count) < 1)
continue;
ret += stack_trace_snprint(buf, size, stack->entries, stack->size, 0);
ret += scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
refcount_read(&stack->count));
*pos |= stack_i;
*pos |= ((long long)table_i << 32);
return ret;
}
table_i++;
/* Keep looking all tables for valid stacks */
if (table_i < stack_table_entries)
goto new_table;
return 0;
}
---->
I will give it a go.
Thanks Marco!
--
Oscar Salvador
SUSE Labs
On Wed, 7 Sept 2022 at 06:00, Oscar Salvador <[email protected]> wrote:
>
> On Tue, Sep 06, 2022 at 10:35:00AM +0200, Marco Elver wrote:
> > I think it's clear from the fact we're using the stack depot that any
> > printing will print stacks. To mirror the existing
> > 'stack_depot_print()', I'd go with 'stack_depot_print_all_count()'.
>
> Fair enough, I will rename it then.
>
> > Moderately better, but still not great. Essentially you need 2
> > cursors, but with loff_t you only get 1.
> >
> > I think the loff_t parameter can be used to encode both cursors. In
> > the kernel, loff_t is always 'long long', so it'll always be 64-bit.
> >
> > Let's assume that collisions in the hash table are rare, so the number
> > of stacks per bucket are typically small. Then you can encode the
> > index into the bucket in bits 0-31 and the bucket index in bits 32-63.
> > STACK_HASH_ORDER_MAX is 20, so 32 bits is plenty to encode the index.
>
> I see, I didn't think of it to be honest.
>
> Then, the below (completely untested) should the trick:
>
> <----
> int stack_depot_print_all_count(char *buf, size_t size, loff_t *pos)
> {
> int ret = 0, stack_i, table_i;
> struct stack_record **stacks, *stack;
> unsigned long stack_table_entries = stack_hash_mask + 1;
>
> stack_i = (*pos & 31);
> table_i = (*pos >> 32);
> new_table:
> stacks = &stack_table[table_i];
> stack = ((struct stack_record *)stacks) + stack_i;
Why are you casting a stack_record** to a stack_record*? stack_table
is already appropriately typed, and there should be no need to cast
things around.
'stacks' is supposed to be the bucket? In which case you need to
dereference it to get the first entry in the bucket: bucket =
stack_table[table_i];
stack_i cannot be used to index into the bucket, because the elements
in it are a linked list and not necessarily adjacent in memory. You
have to traverse the linked list stack_i elements to get to the start:
for (int i = 0; stack && i < stack_i; stack = stack->next, ++i);
then you can proceed with the below code.
> for (; stack; stack = stack->next, stack_i++) {
> if (!stack->size || stack->size < 0 ||
> stack->size > size || stack->handle.valid != 1 ||
> refcount_read(&stack->count) < 1)
> continue;
>
> ret += stack_trace_snprint(buf, size, stack->entries, stack->size, 0);
> ret += scnprintf(buf + ret, size - ret, "stack count: %d\n\n",
> refcount_read(&stack->count));
> *pos |= stack_i;
> *pos |= ((long long)table_i << 32);
> return ret;
> }
>
> table_i++;
> /* Keep looking all tables for valid stacks */
> if (table_i < stack_table_entries)
> goto new_table;
While you're at it, could you try to come up with a version that
avoids the goto?
> return 0;
> }
> ---->
>
> I will give it a go.
>
> Thanks Marco!
>
>
> --
> Oscar Salvador
> SUSE Labs
On Wed, Sep 07, 2022 at 09:14:35AM +0200, Marco Elver wrote:
> Why are you casting a stack_record** to a stack_record*? stack_table
> is already appropriately typed, and there should be no need to cast
> things around.
>
> 'stacks' is supposed to be the bucket? In which case you need to
> dereference it to get the first entry in the bucket: bucket =
> stack_table[table_i];
>
> stack_i cannot be used to index into the bucket, because the elements
> in it are a linked list and not necessarily adjacent in memory. You
> have to traverse the linked list stack_i elements to get to the start:
Yea, I figured that much after thinking about more, but I was overly
eager.
> for (int i = 0; stack && i < stack_i; stack = stack->next, ++i);
But this seems suboptimal.
With this code, we have to walk the list till we find the right index
every time we enter the function, while the actual code of v2
or even the patch from v1 [1], we do not really need to do that
because we already have the pointer to the stack.
So I much rather prefer that, than having to traverse the stacks
till the find the right one.
--
Oscar Salvador
SUSE Labs
On Thu, 8 Sept 2022 at 05:32, Oscar Salvador <[email protected]> wrote:
>
> On Wed, Sep 07, 2022 at 09:14:35AM +0200, Marco Elver wrote:
> > Why are you casting a stack_record** to a stack_record*? stack_table
> > is already appropriately typed, and there should be no need to cast
> > things around.
> >
> > 'stacks' is supposed to be the bucket? In which case you need to
> > dereference it to get the first entry in the bucket: bucket =
> > stack_table[table_i];
> >
> > stack_i cannot be used to index into the bucket, because the elements
> > in it are a linked list and not necessarily adjacent in memory. You
> > have to traverse the linked list stack_i elements to get to the start:
>
> Yea, I figured that much after thinking about more, but I was overly
> eager.
>
> > for (int i = 0; stack && i < stack_i; stack = stack->next, ++i);
>
> But this seems suboptimal.
> With this code, we have to walk the list till we find the right index
> every time we enter the function, while the actual code of v2
> or even the patch from v1 [1], we do not really need to do that
> because we already have the pointer to the stack.
>
> So I much rather prefer that, than having to traverse the stacks
> till the find the right one.
I would not prematurely optimize this. It's a hash map, and the only
problem is if there are tons of collisions. Also, this function isn't
performance critical, it's only used for printing, which itself is
slow.
I suggest you collect some stats how many entries each bucket has on
average. If the average is <10, I'd go with the cleaner interface.