[PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[Huang Ying]

Add __ftrace_enabled_save/restore, used to disable ftrace for a
while. Now, this is used by kexec jump, which need a version without
lock, for general situation, a locked version should be used.

Signed-off-by: Huang Ying <ying.huang@intel.com>

---
 include/linux/ftrace.h |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -98,6 +98,27 @@ static inline void tracer_disable(void)
 #endif
 }
 
+/* Ftrace disable/restore without lock. Some synchronization mechanism
+ * must be used to prevent ftrace_enabled to be changed between
+ * disable/restore. */
+static inline int __ftrace_enabled_save(void)
+{
+#ifdef CONFIG_FTRACE
+	int saved_ftrace_enabled = ftrace_enabled;
+	ftrace_enabled = 0;
+	return saved_ftrace_enabled;
+#else
+	return 0;
+#endif
+}
+
+static inline void __ftrace_enabled_restore(int enabled)
+{
+#ifdef CONFIG_FTRACE
+	ftrace_enabled = enabled;
+#endif
+}
+
 #ifdef CONFIG_FRAME_POINTER
 /* TODO: need to fix this for ARM */
 # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))

Re: [PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[Vivek Goyal]

On Tue, Aug 12, 2008 at 11:14:36AM +0800, Huang Ying wrote:
> Add __ftrace_enabled_save/restore, used to disable ftrace for a
> while. Now, this is used by kexec jump, which need a version without
> lock, for general situation, a locked version should be used.
> 
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> 
> ---
>  include/linux/ftrace.h |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -98,6 +98,27 @@ static inline void tracer_disable(void)
>  #endif
>  }
>  
> +/* Ftrace disable/restore without lock. Some synchronization mechanism
> + * must be used to prevent ftrace_enabled to be changed between
> + * disable/restore. */
> +static inline int __ftrace_enabled_save(void)
> +{
> +#ifdef CONFIG_FTRACE
> +	int saved_ftrace_enabled = ftrace_enabled;
> +	ftrace_enabled = 0;
> +	return saved_ftrace_enabled;
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static inline void __ftrace_enabled_restore(int enabled)
> +{
> +#ifdef CONFIG_FTRACE
> +	ftrace_enabled = enabled;
> +#endif
> +}
> +
>  #ifdef CONFIG_FRAME_POINTER
>  /* TODO: need to fix this for ARM */
>  # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))

I guess steven would like to see a patch which introduces both locked
and lockless versions and with a very good comment explaining in what
kind of unusual situation one can use the lockless version.

Thanks
Vivek

Re: [PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[Huang Ying]

On Tue, 2008-08-12 at 09:06 -0400, Vivek Goyal wrote:
> On Tue, Aug 12, 2008 at 11:14:36AM +0800, Huang Ying wrote:
> > Add __ftrace_enabled_save/restore, used to disable ftrace for a
> > while. Now, this is used by kexec jump, which need a version without
> > lock, for general situation, a locked version should be used.
> > 
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > 
> > ---
> >  include/linux/ftrace.h |   21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -98,6 +98,27 @@ static inline void tracer_disable(void)
> >  #endif
> >  }
> >  
> > +/* Ftrace disable/restore without lock. Some synchronization mechanism
> > + * must be used to prevent ftrace_enabled to be changed between
> > + * disable/restore. */
> > +static inline int __ftrace_enabled_save(void)
> > +{
> > +#ifdef CONFIG_FTRACE
> > +	int saved_ftrace_enabled = ftrace_enabled;
> > +	ftrace_enabled = 0;
> > +	return saved_ftrace_enabled;
> > +#else
> > +	return 0;
> > +#endif
> > +}
> > +
> > +static inline void __ftrace_enabled_restore(int enabled)
> > +{
> > +#ifdef CONFIG_FTRACE
> > +	ftrace_enabled = enabled;
> > +#endif
> > +}
> > +
> >  #ifdef CONFIG_FRAME_POINTER
> >  /* TODO: need to fix this for ARM */
> >  # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))
> 
> I guess steven would like to see a patch which introduces both locked
> and lockless versions and with a very good comment explaining in what
> kind of unusual situation one can use the lockless version.

Have sent a locked version to Steven. And, there are some comments for
non-locked version, __ftrace_enabled_save() in above patch. What do you
think about it?

Best Regards,
Huang Ying

Re: [PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[Ingo Molnar]

* Huang Ying <ying.huang@intel.com> wrote:

> +/* Ftrace disable/restore without lock. Some synchronization mechanism
> + * must be used to prevent ftrace_enabled to be changed between
> + * disable/restore. */

use the proper comment style please:

/*
 *
 */

> +static inline int __ftrace_enabled_save(void)
> +{
> +#ifdef CONFIG_FTRACE
> +	int saved_ftrace_enabled = ftrace_enabled;
> +	ftrace_enabled = 0;
> +	return saved_ftrace_enabled;
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static inline void __ftrace_enabled_restore(int enabled)
> +{
> +#ifdef CONFIG_FTRACE
> +	ftrace_enabled = enabled;
> +#endif
> +}

hm, what is this used for?

also, instead of such an ugly inline, why not create a proper 
kernel/trace/* function for this. That would also give it access to all 
the proper locking mechanisms - instead of relying on some extral 
mechanism.

	Ingo

Re: [PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[Huang Ying]

On Fri, 2008-08-15 at 14:49 +0200, Ingo Molnar wrote:
> * Huang Ying <ying.huang@intel.com> wrote:
> 
> > +/* Ftrace disable/restore without lock. Some synchronization mechanism
> > + * must be used to prevent ftrace_enabled to be changed between
> > + * disable/restore. */
> 
> use the proper comment style please:
> 
> /*
>  *
>  */

OK. I will change it.

> > +static inline int __ftrace_enabled_save(void)
> > +{
> > +#ifdef CONFIG_FTRACE
> > +	int saved_ftrace_enabled = ftrace_enabled;
> > +	ftrace_enabled = 0;
> > +	return saved_ftrace_enabled;
> > +#else
> > +	return 0;
> > +#endif
> > +}
> > +
> > +static inline void __ftrace_enabled_restore(int enabled)
> > +{
> > +#ifdef CONFIG_FTRACE
> > +	ftrace_enabled = enabled;
> > +#endif
> > +}
> 
> hm, what is this used for?
> 
> also, instead of such an ugly inline, why not create a proper 
> kernel/trace/* function for this. That would also give it access to all 
> the proper locking mechanisms - instead of relying on some extral 
> mechanism.

This function is used for kexec jump in machine_kexec(). Where all
non-boot CPUs and IRQ are disabled, system is going to kexec, and it is
not allowed to schedule to other process in this circumstance, so a
non-lock version is needed. A locked version has been implemented by
Steven Rostedt, I think it can be used for other circumstance.

Best Regards,
Huang Ying

Re: [PATCH -v3 6/7] kexec jump: __ftrace_enabled_save/restore

[Ingo Molnar]

* Huang Ying <ying.huang@intel.com> wrote:

> On Fri, 2008-08-15 at 14:49 +0200, Ingo Molnar wrote:
> > * Huang Ying <ying.huang@intel.com> wrote:
> > 
> > > +/* Ftrace disable/restore without lock. Some synchronization mechanism
> > > + * must be used to prevent ftrace_enabled to be changed between
> > > + * disable/restore. */
> > 
> > use the proper comment style please:
> > 
> > /*
> >  *
> >  */
> 
> OK. I will change it.

it's upstream already, please send a followup clean up patch.

> > > +static inline int __ftrace_enabled_save(void)
> > > +{
> > > +#ifdef CONFIG_FTRACE
> > > +	int saved_ftrace_enabled = ftrace_enabled;
> > > +	ftrace_enabled = 0;
> > > +	return saved_ftrace_enabled;
> > > +#else
> > > +	return 0;
> > > +#endif
> > > +}
> > > +
> > > +static inline void __ftrace_enabled_restore(int enabled)
> > > +{
> > > +#ifdef CONFIG_FTRACE
> > > +	ftrace_enabled = enabled;
> > > +#endif
> > > +}
> > 
> > hm, what is this used for?
> > 
> > also, instead of such an ugly inline, why not create a proper 
> > kernel/trace/* function for this. That would also give it access to all 
> > the proper locking mechanisms - instead of relying on some extral 
> > mechanism.
> 
> This function is used for kexec jump in machine_kexec(). Where all 
> non-boot CPUs and IRQ are disabled, system is going to kexec, and it 
> is not allowed to schedule to other process in this circumstance, so a 
> non-lock version is needed. A locked version has been implemented by 
> Steven Rostedt, I think it can be used for other circumstance.

ok, fair enough.

	Ingo