* [PATCH] ftrace: move sysctl_ftrace_enabled to ftrace.c
@ 2022-02-23 1:23 Wei Xiao
2022-02-23 1:52 ` Luis Chamberlain
0 siblings, 1 reply; 3+ messages in thread
From: Wei Xiao @ 2022-02-23 1:23 UTC (permalink / raw)
To: rostedt, mingo, mcgrof, keescook, yzaikin
Cc: linux-kernel, linux-fsdevel, young.liuyang, zengweilin,
nixiaoming, xiaowei66
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(-)
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
--
2.19.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] ftrace: move sysctl_ftrace_enabled to ftrace.c
2022-02-23 1:23 [PATCH] ftrace: move sysctl_ftrace_enabled to ftrace.c Wei Xiao
@ 2022-02-23 1:52 ` Luis Chamberlain
2022-02-23 2:13 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Luis Chamberlain @ 2022-02-23 1:52 UTC (permalink / raw)
To: Wei Xiao
Cc: rostedt, mingo, keescook, yzaikin, linux-kernel, linux-fsdevel,
young.liuyang, zengweilin, nixiaoming
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] ftrace: move sysctl_ftrace_enabled to ftrace.c
2022-02-23 1:52 ` Luis Chamberlain
@ 2022-02-23 2:13 ` Steven Rostedt
0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2022-02-23 2:13 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Wei Xiao, mingo, keescook, yzaikin, linux-kernel, linux-fsdevel,
young.liuyang, zengweilin, nixiaoming
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-23 2:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-23 1:23 [PATCH] ftrace: move sysctl_ftrace_enabled to ftrace.c Wei Xiao
2022-02-23 1:52 ` Luis Chamberlain
2022-02-23 2:13 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).