| Message ID | 20220223012311.134314-1-xiaowei66@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ftrace: move sysctl_ftrace_enabled to ftrace.c | expand |
On Wed, Feb 23, 2022 at 09:23:11AM +0800, Wei Xiao wrote: > This moves ftrace_enabled to trace/ftrace.c. Hey Wei, thanks for you patch! This does not explain how this is being to help with maitenance as otherwise this makes kernel/sysctl.c hard to maintain and we also tend to get many conflicts. It also does not explain how all the filesystem sysctls are not gone and that this is just the next step, moving slowly the rest of the sysctls. Explaining this in the commit log will help patch review and subsystem maintainers understand the conext / logic behind the move. I'd be more than happy to take this if ftrace folks Ack. To avoid conflicts I can route this through sysctl-next which is put forward in particular to avoid conflicts across trees for this effort. Let me know. > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index f9feb197b2da..4a5b4d6996a4 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -7846,7 +7846,8 @@ static bool is_permanent_ops_registered(void) > return false; > } > > -int > +#ifdef CONFIG_SYSCTL > +static int Is the ifdef really needed? It was not there before, ie, does ftrace not depend on sysctls? I don't see a direct relationship but I do wonder if its implicit. > ftrace_enable_sysctl(struct ctl_table *table, int write, > void *buffer, size_t *lenp, loff_t *ppos) > { > @@ -7889,3 +7890,22 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > mutex_unlock(&ftrace_lock); > return ret; > } > + > +static struct ctl_table ftrace_sysctls[] = { > + { > + .procname = "ftrace_enabled", > + .data = &ftrace_enabled, > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = ftrace_enable_sysctl, > + }, > + {} > +}; > + > +static int __init ftrace_sysctl_init(void) > +{ > + register_sysctl_init("kernel", ftrace_sysctls); > + return 0; > +} > +late_initcall(ftrace_sysctl_init); > +#endif There's other __init calls already on ftrace, would this be better placed in one of them, and then have this be a no-op iff we determine ftrace can be built without sysctls and then have a no-op for when sysctls are disabled. Luis
On Tue, 22 Feb 2022 17:52:59 -0800 Luis Chamberlain <mcgrof@kernel.org> wrote: > On Wed, Feb 23, 2022 at 09:23:11AM +0800, Wei Xiao wrote: > > This moves ftrace_enabled to trace/ftrace.c. > > Hey Wei, thanks for you patch! > > This does not explain how this is being to help with maitenance as > otherwise this makes kernel/sysctl.c hard to maintain and we also tend > to get many conflicts. It also does not explain how all the filesystem > sysctls are not gone and that this is just the next step, moving slowly > the rest of the sysctls. Explaining this in the commit log will help > patch review and subsystem maintainers understand the conext / logic > behind the move. > > I'd be more than happy to take this if ftrace folks Ack. To avoid conflicts > I can route this through sysctl-next which is put forward in particular > to avoid conflicts across trees for this effort. Let me know. I'm fine with this change going through another tree. Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org> for your convenience. > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index f9feb197b2da..4a5b4d6996a4 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -7846,7 +7846,8 @@ static bool is_permanent_ops_registered(void) > > return false; > > } > > > > -int > > +#ifdef CONFIG_SYSCTL > > +static int > > Is the ifdef really needed? It was not there before, ie, does > ftrace not depend on sysctls? I don't see a direct relationship > but I do wonder if its implicit. I think because the function is now static, and that it now includes the structure itself, that the #ifdef is added. When I first saw it, I was a bit uncomfortable with adding more #ifdefs, but for this use case, I believe it's OK. > > > ftrace_enable_sysctl(struct ctl_table *table, int write, > > void *buffer, size_t *lenp, loff_t *ppos) > > { > > @@ -7889,3 +7890,22 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, > > mutex_unlock(&ftrace_lock); > > return ret; > > } > > + > > +static struct ctl_table ftrace_sysctls[] = { > > + { > > + .procname = "ftrace_enabled", > > + .data = &ftrace_enabled, > > + .maxlen = sizeof(int), > > + .mode = 0644, > > + .proc_handler = ftrace_enable_sysctl, > > + }, > > + {} > > +}; > > + > > +static int __init ftrace_sysctl_init(void) > > +{ > > + register_sysctl_init("kernel", ftrace_sysctls); > > + return 0; > > +} > > +late_initcall(ftrace_sysctl_init); > > +#endif > > There's other __init calls already on ftrace, would this be better > placed in one of them, and then have this be a no-op iff we determine > ftrace can be built without sysctls and then have a no-op for when > sysctls are disabled. I rather have the initcall here, as the other initcalls are special needs, and not generic functions. -- Steve
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 9999e29187de..659b2840563a 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -94,9 +94,6 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val #ifdef CONFIG_FUNCTION_TRACER extern int ftrace_enabled; -extern int -ftrace_enable_sysctl(struct ctl_table *table, int write, - void *buffer, size_t *lenp, loff_t *ppos); #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 5ae443b2882e..55279ec66b28 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1906,15 +1906,6 @@ static struct ctl_table kern_table[] = { .mode = 0644, .proc_handler = proc_dointvec, }, -#ifdef CONFIG_FUNCTION_TRACER - { - .procname = "ftrace_enabled", - .data = &ftrace_enabled, - .maxlen = sizeof(int), - .mode = 0644, - .proc_handler = ftrace_enable_sysctl, - }, -#endif #ifdef CONFIG_STACK_TRACER { .procname = "stack_tracer_enabled", diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index f9feb197b2da..4a5b4d6996a4 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -7846,7 +7846,8 @@ static bool is_permanent_ops_registered(void) return false; } -int +#ifdef CONFIG_SYSCTL +static int ftrace_enable_sysctl(struct ctl_table *table, int write, void *buffer, size_t *lenp, loff_t *ppos) { @@ -7889,3 +7890,22 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, mutex_unlock(&ftrace_lock); return ret; } + +static struct ctl_table ftrace_sysctls[] = { + { + .procname = "ftrace_enabled", + .data = &ftrace_enabled, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = ftrace_enable_sysctl, + }, + {} +}; + +static int __init ftrace_sysctl_init(void) +{ + register_sysctl_init("kernel", ftrace_sysctls); + return 0; +} +late_initcall(ftrace_sysctl_init); +#endif
This moves ftrace_enabled to trace/ftrace.c. Signed-off-by: Wei Xiao <xiaowei66@huawei.com> --- include/linux/ftrace.h | 3 --- kernel/sysctl.c | 9 --------- kernel/trace/ftrace.c | 22 +++++++++++++++++++++- 3 files changed, 21 insertions(+), 13 deletions(-)