[PATCH] rcu: do not include rtmutex_common.h unconditionally

[PATCH] rcu: do not include rtmutex_common.h unconditionally

[Sebastian Andrzej Siewior]

Since commit bcda31a26594 ("rcu: Suppress lockdep false-positive
->boost_mtx complaints") the rtmutex_common.h is included
unconditionally. This break CONFIG_FUTEX=n configs which do not have
CONFIG_RT_MUTEX enabled which leads to the lack of certain members in
task_struct which are accessed in rtmutex_common.h as reported by the kbuild
test robot:
|   In file included from include/uapi/linux/stddef.h:1:0,
|                    from include/linux/stddef.h:4,
|                    from include/uapi/linux/posix_types.h:4,
|                    from include/uapi/linux/types.h:13,
|                    from include/linux/types.h:5,
|                    from kernel/rcu/tree.c:30:
|   kernel/rcu/../locking/rtmutex_common.h: In function 'task_has_pi_waiters':
|>> kernel/rcu/../locking/rtmutex_common.h:62:26: error: 'struct task_struct' has no member named 'pi_waiters'; did you mean 'cpu_timers'?

among other things.
I move the include back to the RCU_BOOST ifdef and add there the
rt_mutex_futex_unlock define like we already have it for rt_mutex_owner
for the same reason.
While at it, I remove the second rtmutex_common.h include within the
RCU_BOOST block because one of those is enough.

Fixes: bcda31a26594 ("rcu: Suppress lockdep false-positive ->boost_mtx complaints")
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2017-10-18 09:32:09 [+0200], To Paul E. McKenney wrote:
>  I will
> look at this once I made some slides…

slides, who needs those anyway…

 kernel/rcu/tree_plugin.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 72d82eebf70d..d97bb9bd2f3b 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -31,9 +31,9 @@
 #include <linux/smpboot.h>
 #include <uapi/linux/sched/types.h>
 #include "../time/tick-internal.h"
-#include "../locking/rtmutex_common.h"
 
 #ifdef CONFIG_RCU_BOOST
+#include "../locking/rtmutex_common.h"
 
 /*
  * Control variables for per-CPU and per-rcu_node kthreads.  These
@@ -53,6 +53,7 @@ DEFINE_PER_CPU(char, rcu_cpu_has_work);
  * This probably needs to be excluded from -rt builds.
  */
 #define rt_mutex_owner(a) ({ WARN_ON_ONCE(1); NULL; })
+#define rt_mutex_futex_unlock(x) WARN_ON_ONCE(1)
 
 #endif /* #else #ifdef CONFIG_RCU_BOOST */
 
@@ -896,8 +897,6 @@ void exit_rcu(void)
 
 #ifdef CONFIG_RCU_BOOST
 
-#include "../locking/rtmutex_common.h"
-
 static void rcu_wake_cond(struct task_struct *t, int status)
 {
 	/*
-- 
2.15.0.rc0

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Paul E. McKenney]

On Wed, Oct 18, 2017 at 10:34:36AM +0200, Sebastian Andrzej Siewior wrote:
> Since commit bcda31a26594 ("rcu: Suppress lockdep false-positive
> ->boost_mtx complaints") the rtmutex_common.h is included
> unconditionally. This break CONFIG_FUTEX=n configs which do not have
> CONFIG_RT_MUTEX enabled which leads to the lack of certain members in
> task_struct which are accessed in rtmutex_common.h as reported by the kbuild
> test robot:
> |   In file included from include/uapi/linux/stddef.h:1:0,
> |                    from include/linux/stddef.h:4,
> |                    from include/uapi/linux/posix_types.h:4,
> |                    from include/uapi/linux/types.h:13,
> |                    from include/linux/types.h:5,
> |                    from kernel/rcu/tree.c:30:
> |   kernel/rcu/../locking/rtmutex_common.h: In function 'task_has_pi_waiters':
> |>> kernel/rcu/../locking/rtmutex_common.h:62:26: error: 'struct task_struct' has no member named 'pi_waiters'; did you mean 'cpu_timers'?
> 
> among other things.
> I move the include back to the RCU_BOOST ifdef and add there the
> rt_mutex_futex_unlock define like we already have it for rt_mutex_owner
> for the same reason.
> While at it, I remove the second rtmutex_common.h include within the
> RCU_BOOST block because one of those is enough.
> 
> Fixes: bcda31a26594 ("rcu: Suppress lockdep false-positive ->boost_mtx complaints")
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thank you very much, hand-applied as a preparatory patch for
"Suppress lockdep false-positive ->boost_mtx complaints", please see
below.

What I don't understand is why 0day test robot didn't complain about
my copy of the exact same patch.  Or maybe it did and I fat-fingered it?
Except that I have gotten "BUILD SUCCESS" reports for commits including
that one.

Ah well, hopefully all is well that ends well...

> ---
> On 2017-10-18 09:32:09 [+0200], To Paul E. McKenney wrote:
> >  I will
> > look at this once I made some slides…
> 
> slides, who needs those anyway…

Best of everything on the presentation, and hope that I didn't mess
you up too badly.

							Thanx, Paul

------------------------------------------------------------------------

commit a06f537e75ea0a9e81245ede1b97bb3a5762b81b
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date:   Wed Oct 18 08:33:44 2017 -0700

    rcu: do not include rtmutex_common.h unconditionally
    
    This commit adjusts include files and provides definitions in preparation
    for suppressing lockdep false-positive ->boost_mtx complaints.  Without
    this preparation, architectures not supporting rt_mutex will get build
    failures.
    
    Reported-by: kbuild test robot <fengguang.wu@intel.com>
    Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index fed95fa941e6..969eae45f05d 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -54,6 +54,7 @@ DEFINE_PER_CPU(char, rcu_cpu_has_work);
  * This probably needs to be excluded from -rt builds.
  */
 #define rt_mutex_owner(a) ({ WARN_ON_ONCE(1); NULL; })
+#define rt_mutex_futex_unlock(x) WARN_ON_ONCE(1)
 
 #endif /* #else #ifdef CONFIG_RCU_BOOST */
 
@@ -911,8 +912,6 @@ void exit_rcu(void)
 
 #ifdef CONFIG_RCU_BOOST
 
-#include "../locking/rtmutex_common.h"
-
 static void rcu_wake_cond(struct task_struct *t, int status)
 {
 	/*

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Sebastian Andrzej Siewior]

On 2017-10-18 08:39:46 [-0700], Paul E. McKenney wrote:
> Thank you very much, hand-applied as a preparatory patch for
> "Suppress lockdep false-positive ->boost_mtx complaints", please see
> below.
okay.

> What I don't understand is why 0day test robot didn't complain about
> my copy of the exact same patch.  Or maybe it did and I fat-fingered it?
> Except that I have gotten "BUILD SUCCESS" reports for commits including
> that one.

I don't know. It is a "defconfig" for m32r. Unless it skipped that one,
dunno.

> commit a06f537e75ea0a9e81245ede1b97bb3a5762b81b
> Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date:   Wed Oct 18 08:33:44 2017 -0700
> 
>     rcu: do not include rtmutex_common.h unconditionally
>     
>     This commit adjusts include files and provides definitions in preparation
>     for suppressing lockdep false-positive ->boost_mtx complaints.  Without
>     this preparation, architectures not supporting rt_mutex will get build
>     failures.
>     
>     Reported-by: kbuild test robot <fengguang.wu@intel.com>
>     Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index fed95fa941e6..969eae45f05d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -54,6 +54,7 @@ DEFINE_PER_CPU(char, rcu_cpu_has_work);
>   * This probably needs to be excluded from -rt builds.
>   */
>  #define rt_mutex_owner(a) ({ WARN_ON_ONCE(1); NULL; })
> +#define rt_mutex_futex_unlock(x) WARN_ON_ONCE(1)
>  
>  #endif /* #else #ifdef CONFIG_RCU_BOOST */
>  
> @@ -911,8 +912,6 @@ void exit_rcu(void)
>  
>  #ifdef CONFIG_RCU_BOOST
>  
> -#include "../locking/rtmutex_common.h"
> -
>  static void rcu_wake_cond(struct task_struct *t, int status)
>  {
>  	/*

So this probably works. This is
  https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu%2Fdev&id=a06f537e75ea0a9e81245ede1b97bb3a5762b81b&context=40&ignorews=0&dt=0

and the rtmutex_common is still in the ifdef which confused me at first.
But then you wrote "preparatory" and I saw the following patch
  https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu/next&id=33d7471ce21202ce954993552c2e0298d9e0f031

where you move that include rtmutex_common.h. You shouldn't do that
because "rt_mutex_futex_unlock()" has been added added here for the
!BOOST + TREE case. So I thing this should break your build if you
disable CONFIG_FUTEX (which in turn unselects CONFIg_RT_MUTEX).

Sebastian

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Paul E. McKenney]

On Wed, Oct 18, 2017 at 06:16:21PM +0200, Sebastian Andrzej Siewior wrote:
> On 2017-10-18 08:39:46 [-0700], Paul E. McKenney wrote:
> > Thank you very much, hand-applied as a preparatory patch for
> > "Suppress lockdep false-positive ->boost_mtx complaints", please see
> > below.
> okay.
> 
> > What I don't understand is why 0day test robot didn't complain about
> > my copy of the exact same patch.  Or maybe it did and I fat-fingered it?
> > Except that I have gotten "BUILD SUCCESS" reports for commits including
> > that one.
> 
> I don't know. It is a "defconfig" for m32r. Unless it skipped that one,
> dunno.

No idea here, either.

> > commit a06f537e75ea0a9e81245ede1b97bb3a5762b81b
> > Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Date:   Wed Oct 18 08:33:44 2017 -0700
> > 
> >     rcu: do not include rtmutex_common.h unconditionally
> >     
> >     This commit adjusts include files and provides definitions in preparation
> >     for suppressing lockdep false-positive ->boost_mtx complaints.  Without
> >     this preparation, architectures not supporting rt_mutex will get build
> >     failures.
> >     
> >     Reported-by: kbuild test robot <fengguang.wu@intel.com>
> >     Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index fed95fa941e6..969eae45f05d 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -54,6 +54,7 @@ DEFINE_PER_CPU(char, rcu_cpu_has_work);
> >   * This probably needs to be excluded from -rt builds.
> >   */
> >  #define rt_mutex_owner(a) ({ WARN_ON_ONCE(1); NULL; })
> > +#define rt_mutex_futex_unlock(x) WARN_ON_ONCE(1)
> >  
> >  #endif /* #else #ifdef CONFIG_RCU_BOOST */
> >  
> > @@ -911,8 +912,6 @@ void exit_rcu(void)
> >  
> >  #ifdef CONFIG_RCU_BOOST
> >  
> > -#include "../locking/rtmutex_common.h"
> > -
> >  static void rcu_wake_cond(struct task_struct *t, int status)
> >  {
> >  	/*
> 
> So this probably works. This is
>   https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu%2Fdev&id=a06f537e75ea0a9e81245ede1b97bb3a5762b81b&context=40&ignorews=0&dt=0
> 
> and the rtmutex_common is still in the ifdef which confused me at first.
> But then you wrote "preparatory" and I saw the following patch
>   https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu/next&id=33d7471ce21202ce954993552c2e0298d9e0f031
> 
> where you move that include rtmutex_common.h. You shouldn't do that
> because "rt_mutex_futex_unlock()" has been added added here for the
> !BOOST + TREE case. So I thing this should break your build if you
> disable CONFIG_FUTEX (which in turn unselects CONFIg_RT_MUTEX).

Builds for me on x86 and 0day test robot hasn't complained, but might
as well get it right.  The new commits are:

a06f537e75ea ("rcu: do not include rtmutex_common.h unconditionally")
4a0fb5d70bb2 ("rcu: Suppress lockdep false-positive ->boost_mtx complaints")

							Thanx, Paul

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Sebastian Andrzej Siewior]

On 2017-10-18 13:42:59 [-0700], Paul E. McKenney wrote:
> 
> Builds for me on x86 and 0day test robot hasn't complained, but might
> as well get it right.

So I checked you tree and there is this:

|$ git one bc2eecd7ecce40af43b6eb3d256b6076257df846
|bc2eecd7ecce ("futex: Allow for compiling out PI support")
|$ git describe bc2eecd7ecce40af43b6eb3d256b6076257df846 --contains
|v4.14-rc1~162^2~57

so this exploded on my side because I applied it on top of v3.13, you on
the other hand had it post v4.13-rc1 so it was all good.
Now, that it is possible to include that header file with and without
CONFIG_RT_MUTEX=y we could indeed move that include outside of that
ifdef. Sorry for that.
We could do that and move the two defines (rt_mutex_owner +
rt_mutex_futex_unlock) to the rtmutex_common.h like in bc2eecd7ecce.
This might look good, I don't know. If you want, I can prepare a patch
for that, we could leave it as it…

>                        The new commits are:
> 
> a06f537e75ea ("rcu: do not include rtmutex_common.h unconditionally")
> 4a0fb5d70bb2 ("rcu: Suppress lockdep false-positive ->boost_mtx complaints")

Okay, thanks.

> 							Thanx, Paul

Sebastian

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Paul E. McKenney]

On Thu, Oct 19, 2017 at 08:15:05PM +0200, Sebastian Andrzej Siewior wrote:
> On 2017-10-18 13:42:59 [-0700], Paul E. McKenney wrote:
> > 
> > Builds for me on x86 and 0day test robot hasn't complained, but might
> > as well get it right.
> 
> So I checked you tree and there is this:
> 
> |$ git one bc2eecd7ecce40af43b6eb3d256b6076257df846
> |bc2eecd7ecce ("futex: Allow for compiling out PI support")
> |$ git describe bc2eecd7ecce40af43b6eb3d256b6076257df846 --contains
> |v4.14-rc1~162^2~57
> 
> so this exploded on my side because I applied it on top of v3.13, you on
> the other hand had it post v4.13-rc1 so it was all good.
> Now, that it is possible to include that header file with and without
> CONFIG_RT_MUTEX=y we could indeed move that include outside of that
> ifdef. Sorry for that.

Sounds like something I would do!  ;-)

> We could do that and move the two defines (rt_mutex_owner +
> rt_mutex_futex_unlock) to the rtmutex_common.h like in bc2eecd7ecce.
> This might look good, I don't know. If you want, I can prepare a patch
> for that, we could leave it as it…

One advantage is that builds would go more easily, and things like
RCU that need to build either way wouldn't need their own splat-generating
definitions of the rt_mutex primitives.

On the other hand, this would lose the current build-time detection
of use of rt_mutux on architectures not providing it.

Given the choice, I would prefer the build-time detection.

Thoughts?

							Thanx, Paul

> >                        The new commits are:
> > 
> > a06f537e75ea ("rcu: do not include rtmutex_common.h unconditionally")
> > 4a0fb5d70bb2 ("rcu: Suppress lockdep false-positive ->boost_mtx complaints")
> 
> Okay, thanks.
> 
> > 							Thanx, Paul
> 
> Sebastian
>