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(&stack_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(&stack_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(&stack_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(&trace_ops); > else > unregister_ftrace_function(&trace_ops); > - > out: > mutex_unlock(&stack_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