Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Fri, 19 Apr 2019 00:44:17 +0200 (CEST)
Thomas Gleixner wrote:
> On Thu, 18 Apr 2019, Steven Rostedt wrote:
> > On Thu, 18 Apr 2019 10:41:20 +0200
> > Thomas Gleixner wrote:
> >
> > > @@ -412,23 +404,20 @@ stack_trace_sysctl(struct ctl_table *tab
> > > void __user *buffer, size_t *lenp,
> > > loff_t *ppos)
> > > {
> > > - int ret;
> > > + int ret, was_enabled;
> >
> > One small nit. Could this be:
> >
> > int was_enabled;
> > int ret;
> >
> > I prefer only joining variables that are related on the same line.
> > Makes it look cleaner IMO.
>
> If you wish so. To me it's waste of screen space :)
At least you didn't say it helps the compiler ;-)
>
> > >
> > > mutex_lock(_sysctl_mutex);
> > > + was_enabled = !!stack_tracer_enabled;
> > >
> >
> > Bah, not sure why I didn't do it this way to begin with. I think I
> > copied something else that couldn't do it this way for some reason and
> > didn't put any brain power behind the copy. :-/ But that was back in
> > 2008 so I blame it on being "young and stupid" ;-)
>
> The young part is gone for sure :)
I purposely set you up for that response.
>
> > Other then the above nit and removing the unneeded +1 in max_entries:
>
> s/+1/-1/
That was an ode to G+
-- Steve
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, 18 Apr 2019, Steven Rostedt wrote:
> On Thu, 18 Apr 2019 10:41:20 +0200
> Thomas Gleixner wrote:
>
> > @@ -412,23 +404,20 @@ stack_trace_sysctl(struct ctl_table *tab
> >void __user *buffer, size_t *lenp,
> >loff_t *ppos)
> > {
> > - int ret;
> > + int ret, was_enabled;
>
> One small nit. Could this be:
>
> int was_enabled;
> int ret;
>
> I prefer only joining variables that are related on the same line.
> Makes it look cleaner IMO.
If you wish so. To me it's waste of screen space :)
> >
> > mutex_lock(_sysctl_mutex);
> > + was_enabled = !!stack_tracer_enabled;
> >
>
> Bah, not sure why I didn't do it this way to begin with. I think I
> copied something else that couldn't do it this way for some reason and
> didn't put any brain power behind the copy. :-/ But that was back in
> 2008 so I blame it on being "young and stupid" ;-)
The young part is gone for sure :)
> Other then the above nit and removing the unneeded +1 in max_entries:
s/+1/-1/
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, 18 Apr 2019 10:41:20 +0200
Thomas Gleixner wrote:
> @@ -412,23 +404,20 @@ stack_trace_sysctl(struct ctl_table *tab
> void __user *buffer, size_t *lenp,
> loff_t *ppos)
> {
> - int ret;
> + int ret, was_enabled;
One small nit. Could this be:
int was_enabled;
int ret;
I prefer only joining variables that are related on the same line.
Makes it look cleaner IMO.
>
> mutex_lock(_sysctl_mutex);
> + was_enabled = !!stack_tracer_enabled;
>
Bah, not sure why I didn't do it this way to begin with. I think I
copied something else that couldn't do it this way for some reason and
didn't put any brain power behind the copy. :-/ But that was back in
2008 so I blame it on being "young and stupid" ;-)
Other then the above nit and removing the unneeded +1 in max_entries:
Reviewed-by: Steven Rostedt (VMware)
-- Steve
> ret = proc_dointvec(table, write, buffer, lenp, ppos);
>
> - if (ret || !write ||
> - (last_stack_tracer_enabled == !!stack_tracer_enabled))
> + if (ret || !write || (was_enabled == !!stack_tracer_enabled))
> goto out;
>
> - last_stack_tracer_enabled = !!stack_tracer_enabled;
> -
> if (stack_tracer_enabled)
> register_ftrace_function(_ops);
> else
> unregister_ftrace_function(_ops);
> -
> out:
> mutex_unlock(_sysctl_mutex);
> return ret;
> @@ -444,7 +433,6 @@ static __init int enable_stacktrace(char
> strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
>
> stack_tracer_enabled = 1;
> - last_stack_tracer_enabled = 1;
> return 1;
> }
> __setup("stacktrace", enable_stacktrace);
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, 18 Apr 2019 17:24:43 -0400 Steven Rostedt wrote: > I believe it was for historical leftovers (there was a time it was > required), and left there for "paranoid" sake. But let me apply the > patch and see if it is really needed. I removed the +1 on the max_entries and set SET_TRACE_ENTRIES to 5 (a bit extreme). Then I ran the stack tracing with KASAN enabled and it never complained. As stated, it was there for historical reasons and I felt 500 was way more than enough and left the buffer there just out of laziness and paranoia. Feel free to remove that if you want. -- Steve ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, 18 Apr 2019 23:14:45 +0200 (CEST) Thomas Gleixner wrote: > On Thu, 18 Apr 2019, Josh Poimboeuf wrote: > > > On Thu, Apr 18, 2019 at 10:41:20AM +0200, Thomas Gleixner wrote: > > > - Remove the extra array member of stack_dump_trace[]. It's not required > > > as > > > the stack tracer stores at max array size - 1 entries so there is still > > > an empty slot. > > > > What is the empty slot used for? > > I was trying to find an answer but failed. Maybe it's just historical > leftovers or Steven knows where the magic is in this maze. > I believe it was for historical leftovers (there was a time it was required), and left there for "paranoid" sake. But let me apply the patch and see if it is really needed. -- Steve ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, 18 Apr 2019, Josh Poimboeuf wrote: > On Thu, Apr 18, 2019 at 10:41:20AM +0200, Thomas Gleixner wrote: > > - Remove the extra array member of stack_dump_trace[]. It's not required as > > the stack tracer stores at max array size - 1 entries so there is still > > an empty slot. > > What is the empty slot used for? I was trying to find an answer but failed. Maybe it's just historical leftovers or Steven knows where the magic is in this maze. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, Apr 18, 2019 at 10:41:20AM +0200, Thomas Gleixner wrote: > - Remove the extra array member of stack_dump_trace[]. It's not required as > the stack tracer stores at max array size - 1 entries so there is still > an empty slot. What is the empty slot used for? -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch V2 01/29] tracing: Cleanup stack trace code
- Remove the extra array member of stack_dump_trace[]. It's not required as
the stack tracer stores at max array size - 1 entries so there is still
an empty slot.
- Make variables which are only used in trace_stack.c static.
- Simplify the enable/disable logic.
- Rename stack_trace_print() as it's using the stack_trace_ namespace. Free
the name up for stack trace related functions.
Signed-off-by: Thomas Gleixner
Cc: Steven Rostedt
---
V2: Add more cleanups and use print_max_stack() as requested by Steven.
---
include/linux/ftrace.h | 18 --
kernel/trace/trace_stack.c | 36
2 files changed, 16 insertions(+), 38 deletions(-)
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -241,21 +241,11 @@ static inline void ftrace_free_mem(struc
#ifdef CONFIG_STACK_TRACER
-#define STACK_TRACE_ENTRIES 500
-
-struct stack_trace;
-
-extern unsigned stack_trace_index[];
-extern struct stack_trace stack_trace_max;
-extern unsigned long stack_trace_max_size;
-extern arch_spinlock_t stack_trace_max_lock;
-
extern int stack_tracer_enabled;
-void stack_trace_print(void);
-int
-stack_trace_sysctl(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp,
- loff_t *ppos);
+
+int stack_trace_sysctl(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos);
/* DO NOT MODIFY THIS VARIABLE DIRECTLY! */
DECLARE_PER_CPU(int, disable_stack_tracer);
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -18,8 +18,10 @@
#include "trace.h"
-static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES + 1];
-unsigned stack_trace_index[STACK_TRACE_ENTRIES];
+#define STACK_TRACE_ENTRIES 500
+
+static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES];
+static unsigned stack_trace_index[STACK_TRACE_ENTRIES];
/*
* Reserve one entry for the passed in ip. This will allow
@@ -31,17 +33,16 @@ struct stack_trace stack_trace_max = {
.entries= _dump_trace[0],
};
-unsigned long stack_trace_max_size;
-arch_spinlock_t stack_trace_max_lock =
+static unsigned long stack_trace_max_size;
+static arch_spinlock_t stack_trace_max_lock =
(arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
DEFINE_PER_CPU(int, disable_stack_tracer);
static DEFINE_MUTEX(stack_sysctl_mutex);
int stack_tracer_enabled;
-static int last_stack_tracer_enabled;
-void stack_trace_print(void)
+static void print_max_stack(void)
{
long i;
int size;
@@ -61,16 +62,7 @@ void stack_trace_print(void)
}
}
-/*
- * When arch-specific code overrides this function, the following
- * data should be filled up, assuming stack_trace_max_lock is held to
- * prevent concurrent updates.
- * stack_trace_index[]
- * stack_trace_max
- * stack_trace_max_size
- */
-void __weak
-check_stack(unsigned long ip, unsigned long *stack)
+static void check_stack(unsigned long ip, unsigned long *stack)
{
unsigned long this_size, flags; unsigned long *p, *top, *start;
static int tracer_frame;
@@ -179,7 +171,7 @@ check_stack(unsigned long ip, unsigned l
stack_trace_max.nr_entries = x;
if (task_stack_end_corrupted(current)) {
- stack_trace_print();
+ print_max_stack();
BUG();
}
@@ -412,23 +404,20 @@ stack_trace_sysctl(struct ctl_table *tab
void __user *buffer, size_t *lenp,
loff_t *ppos)
{
- int ret;
+ int ret, was_enabled;
mutex_lock(_sysctl_mutex);
+ was_enabled = !!stack_tracer_enabled;
ret = proc_dointvec(table, write, buffer, lenp, ppos);
- if (ret || !write ||
- (last_stack_tracer_enabled == !!stack_tracer_enabled))
+ if (ret || !write || (was_enabled == !!stack_tracer_enabled))
goto out;
- last_stack_tracer_enabled = !!stack_tracer_enabled;
-
if (stack_tracer_enabled)
register_ftrace_function(_ops);
else
unregister_ftrace_function(_ops);
-
out:
mutex_unlock(_sysctl_mutex);
return ret;
@@ -444,7 +433,6 @@ static __init int enable_stacktrace(char
strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
stack_tracer_enabled = 1;
- last_stack_tracer_enabled = 1;
return 1;
}
__setup("stacktrace", enable_stacktrace);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
