Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1267970 > unrolled thread

[PATCH] sched/rt: hide push_irq_work_func declaration

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/rt: hide push_irq_work_func declaration Arnd Bergmann <arnd@arndb.de> - 2015-11-12 15:30 +0100
    Re: [PATCH] sched/rt: hide push_irq_work_func declaration Steven Rostedt <rostedt@goodmis.org> - 2015-11-12 15:50 +0100
      Re: [PATCH] sched/rt: hide push_irq_work_func declaration Arnd Bergmann <arnd@arndb.de> - 2015-11-12 16:20 +0100
        Re: [PATCH] sched/rt: hide push_irq_work_func declaration Steven Rostedt <rostedt@goodmis.org> - 2015-11-12 16:50 +0100
    Re: [PATCH] sched/rt: hide push_irq_work_func declaration Peter Zijlstra <peterz@infradead.org> - 2015-11-12 16:00 +0100
      Re: [PATCH] sched/rt: hide push_irq_work_func declaration Steven Rostedt <rostedt@goodmis.org> - 2015-11-12 16:20 +0100
    [PATCH v2] sched/rt: hide push_irq_work_func declaration Arnd Bergmann <arnd@arndb.de> - 2015-11-12 17:30 +0100
      Re: [PATCH v2] sched/rt: hide push_irq_work_func declaration Steven Rostedt <rostedt@goodmis.org> - 2015-11-12 17:30 +0100

#1267970 — [PATCH] sched/rt: hide push_irq_work_func declaration

The push_irq_work_func() function is conditionally defined only
when both CONFIG_SMP and HAVE_RT_PUSH_IPI are defined, but the
forward declaration remains visibile without HAVE_RT_PUSH_IPI,
causing a gcc warning in ARM64 allnoconfig:

kernel/sched/rt.c:68:13: warning: 'push_irq_work_func' declared 'static' but never defined [-Wunused-function]

This changes the code to use the same condition for both the
declaration and the function definition, which gets rid of the
warning.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: b6366f048e0c ("sched/rt: Use IPI to trigger RT task push migration instead of pulling")
---
Found on arm64 allnoconfig

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index e3cc16312046..ce7b36d6f477 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -64,7 +64,7 @@ static void start_rt_bandwidth(struct rt_bandwidth *rt_b)
 	raw_spin_unlock(&rt_b->rt_runtime_lock);
 }
 
-#ifdef CONFIG_SMP
+#if IS_ENABLED(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
 static void push_irq_work_func(struct irq_work *work);
 #endif
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1267997

On Thu, 12 Nov 2015 15:22:22 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> The push_irq_work_func() function is conditionally defined only
> when both CONFIG_SMP and HAVE_RT_PUSH_IPI are defined, but the
> forward declaration remains visibile without HAVE_RT_PUSH_IPI,
> causing a gcc warning in ARM64 allnoconfig:
> 
> kernel/sched/rt.c:68:13: warning: 'push_irq_work_func' declared 'static' but never defined [-Wunused-function]
> 
> This changes the code to use the same condition for both the
> declaration and the function definition, which gets rid of the
> warning.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: b6366f048e0c ("sched/rt: Use IPI to trigger RT task push migration instead of pulling")
> ---
> Found on arm64 allnoconfig
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index e3cc16312046..ce7b36d6f477 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -64,7 +64,7 @@ static void start_rt_bandwidth(struct rt_bandwidth *rt_b)
>  	raw_spin_unlock(&rt_b->rt_runtime_lock);
>  }
>  
> -#ifdef CONFIG_SMP
> +#if IS_ENABLED(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)

Why IS_ENABLED() and not defined()?

#if defined(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)

I thought IS_ENABLED() is used for C code, like:

	if (IS_ENABLED(CONFIG_SMP)) {
		[...]
	}

-- Steve

>  static void push_irq_work_func(struct irq_work *work);
>  #endif
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268025

On Thursday 12 November 2015 09:47:39 Steven Rostedt wrote:
> > -#ifdef CONFIG_SMP
> > +#if IS_ENABLED(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
> 
> Why IS_ENABLED() and not defined()?
>
> #if defined(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
> 
> I thought IS_ENABLED() is used for C code, like:
> 
>         if (IS_ENABLED(CONFIG_SMP)) {
>                 [...]
>         }

"#if IS_ENABLED(CONFIG_foo)" has another property, which is
to evaluate to true when Kconfig has set the symbol to "=m".
Obviously that cannot happen for CONFIG_SMP, but some
maintainers prefer using IS_ENABLED() consistently for all
config symbols.

I don't care much either way, and it's easily changed if
we still want the patch and you prefer a plain #if defined().

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268048

On Thu, 12 Nov 2015 16:14:06 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Thursday 12 November 2015 09:47:39 Steven Rostedt wrote:
> > > -#ifdef CONFIG_SMP
> > > +#if IS_ENABLED(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)  
> > 
> > Why IS_ENABLED() and not defined()?
> >
> > #if defined(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
> > 
> > I thought IS_ENABLED() is used for C code, like:
> > 
> >         if (IS_ENABLED(CONFIG_SMP)) {
> >                 [...]
> >         }  
> 
> "#if IS_ENABLED(CONFIG_foo)" has another property, which is
> to evaluate to true when Kconfig has set the symbol to "=m".
> Obviously that cannot happen for CONFIG_SMP, but some
> maintainers prefer using IS_ENABLED() consistently for all
> config symbols.
> 
> I don't care much either way, and it's easily changed if
> we still want the patch and you prefer a plain #if defined().
> 

For this instance, I do prefer the "defined()" version. I can
understand the IS_ENABLED() for module configs. But I don't see the
need to make all #if's use it. Maybe because I don't deal with module
configs much.

Anyway, this patch should probably go in now as a current "fix", and
the redesign should be looked at as well, but that's unrelated to your
arch, and you shouldn't need to worry about it.

Thanks,

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268008

On Thu, Nov 12, 2015 at 03:22:22PM +0100, Arnd Bergmann wrote:
> -#ifdef CONFIG_SMP
> +#if IS_ENABLED(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
>  static void push_irq_work_func(struct irq_work *work);
>  #endif

I think we can get tid of the whole HAVE_RT_PUSH_IPI thing after:
8053871d0f7f ("smp: Fix smp_call_function_single_async() locking")

That should allow us to use smp_calling_function_single_async() instead
of irq_work_queue_on(), removing the dependency on irq_work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268022

On Thu, 12 Nov 2015 15:53:31 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Nov 12, 2015 at 03:22:22PM +0100, Arnd Bergmann wrote:
> > -#ifdef CONFIG_SMP
> > +#if IS_ENABLED(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
> >  static void push_irq_work_func(struct irq_work *work);
> >  #endif  
> 
> I think we can get tid of the whole HAVE_RT_PUSH_IPI thing after:
> 8053871d0f7f ("smp: Fix smp_call_function_single_async() locking")
> 
> That should allow us to use smp_calling_function_single_async() instead
> of irq_work_queue_on(), removing the dependency on irq_work.

Interesting. I'll have to take a look at that.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268077 — [PATCH v2] sched/rt: hide push_irq_work_func declaration

The push_irq_work_func() function is conditionally defined only
when both CONFIG_SMP and HAVE_RT_PUSH_IPI are defined, but the
forward declaration remains visibile without HAVE_RT_PUSH_IPI,
causing a gcc warning in ARM64 allnoconfig:

kernel/sched/rt.c:68:13: warning: 'push_irq_work_func' declared 'static' but never defined [-Wunused-function]

This changes the code to use the same condition for both the
declaration and the function definition, which gets rid of the
warning.

As Peter Zijlstra, we can possibly get rid of the whole HAVE_RT_PUSH_IPI
thing after:
8053871d0f7f ("smp: Fix smp_call_function_single_async() locking")

Until that is done, this patch can be used to avoid the warning.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: b6366f048e0c ("sched/rt: Use IPI to trigger RT task push migration instead of pulling")
---
Found on arm64 allnoconfig

v2: now using #if defined() instead IS_ENABLED()

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index e3cc16312046..ce7b36d6f477 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -64,7 +64,7 @@ static void start_rt_bandwidth(struct rt_bandwidth *rt_b)
        raw_spin_unlock(&rt_b->rt_runtime_lock);
 }
 
-#ifdef CONFIG_SMP
+#if defined(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
 static void push_irq_work_func(struct irq_work *work);
 #endif
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268084 — Re: [PATCH v2] sched/rt: hide push_irq_work_func declaration

On Thu, 12 Nov 2015 17:19:58 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> The push_irq_work_func() function is conditionally defined only
> when both CONFIG_SMP and HAVE_RT_PUSH_IPI are defined, but the
> forward declaration remains visibile without HAVE_RT_PUSH_IPI,
> causing a gcc warning in ARM64 allnoconfig:
> 
> kernel/sched/rt.c:68:13: warning: 'push_irq_work_func' declared 'static' but never defined [-Wunused-function]
> 
> This changes the code to use the same condition for both the
> declaration and the function definition, which gets rid of the
> warning.
> 
> As Peter Zijlstra, we can possibly get rid of the whole HAVE_RT_PUSH_IPI
> thing after:
> 8053871d0f7f ("smp: Fix smp_call_function_single_async() locking")
> 
> Until that is done, this patch can be used to avoid the warning.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: b6366f048e0c ("sched/rt: Use IPI to trigger RT task push migration instead of pulling")
> ---
> Found on arm64 allnoconfig
> 
> v2: now using #if defined() instead IS_ENABLED()
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index e3cc16312046..ce7b36d6f477 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -64,7 +64,7 @@ static void start_rt_bandwidth(struct rt_bandwidth *rt_b)
>         raw_spin_unlock(&rt_b->rt_runtime_lock);
>  }
>  
> -#ifdef CONFIG_SMP
> +#if defined(CONFIG_SMP) && defined(HAVE_RT_PUSH_IPI)
>  static void push_irq_work_func(struct irq_work *work);
>  #endif
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web