Re: [Intel-gfx] [patch V2 14/29] dm bufio: Simplify stack trace retrieval
On Thu, Apr 18, 2019 at 1:54 PM Thomas Gleixner wrote: > > On Thu, 18 Apr 2019, Alexander Potapenko wrote: > > On Thu, Apr 18, 2019 at 11:06 AM Thomas Gleixner wrote: > > > - save_stack_trace(>stack_trace); > > > + b->stack_len = stack_trace_save(b->stack_entries, MAX_STACK, 2); > > As noted in one of similar patches before, can we have an inline > > comment to indicate what does this "2" stand for? > > Come on. We have gazillion of functions which take numerical constant > arguments. Should we add comments to all of them? Ok, sorry. I might not be familiar enough with the kernel style guide. > Thanks, > > tglx -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [patch V2 14/29] dm bufio: Simplify stack trace retrieval
On Thu, Apr 18, 2019 at 11:06 AM Thomas Gleixner wrote:
>
> Replace the indirection through struct stack_trace with an invocation of
> the storage array based interface.
>
> Signed-off-by: Thomas Gleixner
> Cc: dm-de...@redhat.com
> Cc: Mike Snitzer
> Cc: Alasdair Kergon
> ---
> drivers/md/dm-bufio.c | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -150,7 +150,7 @@ struct dm_buffer {
> void (*end_io)(struct dm_buffer *, blk_status_t);
> #ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
> #define MAX_STACK 10
> - struct stack_trace stack_trace;
> + unsigned int stack_len;
> unsigned long stack_entries[MAX_STACK];
> #endif
> };
> @@ -232,11 +232,7 @@ static DEFINE_MUTEX(dm_bufio_clients_loc
> #ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
> static void buffer_record_stack(struct dm_buffer *b)
> {
> - b->stack_trace.nr_entries = 0;
> - b->stack_trace.max_entries = MAX_STACK;
> - b->stack_trace.entries = b->stack_entries;
> - b->stack_trace.skip = 2;
> - save_stack_trace(>stack_trace);
> + b->stack_len = stack_trace_save(b->stack_entries, MAX_STACK, 2);
As noted in one of similar patches before, can we have an inline
comment to indicate what does this "2" stand for?
> }
> #endif
>
> @@ -438,7 +434,7 @@ static struct dm_buffer *alloc_buffer(st
> adjust_total_allocated(b->data_mode, (long)c->block_size);
>
> #ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
> - memset(>stack_trace, 0, sizeof(b->stack_trace));
> + b->stack_len = 0;
> #endif
> return b;
> }
> @@ -1520,8 +1516,9 @@ static void drop_buffers(struct dm_bufio
> DMERR("leaked buffer %llx, hold count %u, list %d",
> (unsigned long long)b->block, b->hold_count, i);
> #ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
> - print_stack_trace(>stack_trace, 1);
> - b->hold_count = 0; /* mark unclaimed to avoid BUG_ON
> below */
> + stack_trace_print(b->stack_entries, b->stack_len, 1);
> + /* mark unclaimed to avoid BUG_ON below */
> + b->hold_count = 0;
> #endif
> }
>
>
>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [patch V2 14/29] dm bufio: Simplify stack trace retrieval
On Thu, 18 Apr 2019 14:11:44 +0200 Alexander Potapenko wrote: > On Thu, Apr 18, 2019 at 1:54 PM Thomas Gleixner wrote: > > > > On Thu, 18 Apr 2019, Alexander Potapenko wrote: > > > On Thu, Apr 18, 2019 at 11:06 AM Thomas Gleixner > > > wrote: > > > > - save_stack_trace(>stack_trace); > > > > + b->stack_len = stack_trace_save(b->stack_entries, MAX_STACK, > > > > 2); > > > As noted in one of similar patches before, can we have an inline > > > comment to indicate what does this "2" stand for? > > > > Come on. We have gazillion of functions which take numerical constant > > arguments. Should we add comments to all of them? > Ok, sorry. I might not be familiar enough with the kernel style guide. It is a legitimate complaint but not for this series. I only complain about hard coded constants when they are added. That "2" was not added by this series. This patch set is a clean up of the stack tracing code, not a clean up of removing hard coded constants, or commenting them. The hard coded "2" was there without a comment before this patch series and Thomas is correct to leave it as is for these changes. This patch series should not modify what was already there which is out of scope for the purpose of these changes. A separate clean up patch to the maintainer of the subsystem (dm bufio in this case) is fine. But it's not Thomas's responsibility. -- Steve ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [patch V2 14/29] dm bufio: Simplify stack trace retrieval
On Thu, 18 Apr 2019, Alexander Potapenko wrote: > On Thu, Apr 18, 2019 at 11:06 AM Thomas Gleixner wrote: > > - save_stack_trace(>stack_trace); > > + b->stack_len = stack_trace_save(b->stack_entries, MAX_STACK, 2); > As noted in one of similar patches before, can we have an inline > comment to indicate what does this "2" stand for? Come on. We have gazillion of functions which take numerical constant arguments. Should we add comments to all of them? Thanks, tglx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [patch V2 14/29] dm bufio: Simplify stack trace retrieval
Replace the indirection through struct stack_trace with an invocation of
the storage array based interface.
Signed-off-by: Thomas Gleixner
Cc: dm-de...@redhat.com
Cc: Mike Snitzer
Cc: Alasdair Kergon
---
drivers/md/dm-bufio.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -150,7 +150,7 @@ struct dm_buffer {
void (*end_io)(struct dm_buffer *, blk_status_t);
#ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
#define MAX_STACK 10
- struct stack_trace stack_trace;
+ unsigned int stack_len;
unsigned long stack_entries[MAX_STACK];
#endif
};
@@ -232,11 +232,7 @@ static DEFINE_MUTEX(dm_bufio_clients_loc
#ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
static void buffer_record_stack(struct dm_buffer *b)
{
- b->stack_trace.nr_entries = 0;
- b->stack_trace.max_entries = MAX_STACK;
- b->stack_trace.entries = b->stack_entries;
- b->stack_trace.skip = 2;
- save_stack_trace(>stack_trace);
+ b->stack_len = stack_trace_save(b->stack_entries, MAX_STACK, 2);
}
#endif
@@ -438,7 +434,7 @@ static struct dm_buffer *alloc_buffer(st
adjust_total_allocated(b->data_mode, (long)c->block_size);
#ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
- memset(>stack_trace, 0, sizeof(b->stack_trace));
+ b->stack_len = 0;
#endif
return b;
}
@@ -1520,8 +1516,9 @@ static void drop_buffers(struct dm_bufio
DMERR("leaked buffer %llx, hold count %u, list %d",
(unsigned long long)b->block, b->hold_count, i);
#ifdef CONFIG_DM_DEBUG_BLOCK_STACK_TRACING
- print_stack_trace(>stack_trace, 1);
- b->hold_count = 0; /* mark unclaimed to avoid BUG_ON
below */
+ stack_trace_print(b->stack_entries, b->stack_len, 1);
+ /* mark unclaimed to avoid BUG_ON below */
+ b->hold_count = 0;
#endif
}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
