sample fix for splat in futex_[un]lock_pi for !rt

migrate_disable() does nothing !SMP && !RT. This is bad for two reasons:
- The futex code relies on the fact migrate_disable() is part of spin_lock(). There is a workaround for the !in_atomic() case in migrate_disable() which work-arounds the different ordering (non-atomic lock and atomic unlock).

- we have a few instances where preempt_disable() is replaced with migrate_disable().

For both cases it is bad if migrate_disable() ends up as barrier() instead of preempt_disable(). Let migrate_disable() fallback to preempt_disable().

Cc [email protected]

Reported-by [email protected]

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 include/linux/preempt.h | 4 ++--
 kernel/sched/core.c     | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

include/linux/preempt.h +2 -2

index 043e431a7e8e..d46688d521e6 100644
@@ -241,8 +241,8 @@ static inline int __migrate_disabled(struct task_struct *p)
 }
 
 #else
-#define migrate_disable()		barrier()
-#define migrate_enable()		barrier()
+#define migrate_disable()		preempt_disable()
+#define migrate_enable()		preempt_enable()
 static inline int __migrate_disabled(struct task_struct *p)
 {
 	return 0;

kernel/sched/core.c +2 -0

index ac3fb8495bd5..626a62218518 100644
@@ -7326,6 +7326,7 @@ void migrate_disable(void)
 #endif
 
 	p->migrate_disable++;
+	preempt_disable();
 }
 EXPORT_SYMBOL(migrate_disable);
 

@@ -7349,6 +7350,7 @@ void migrate_enable(void)
 
 	WARN_ON_ONCE(p->migrate_disable <= 0);
 	p->migrate_disable--;
+	preempt_enable();
 }
 EXPORT_SYMBOL(migrate_enable);
 #endif

Signature · 1 line

2.18.0

Hi Sebastian,

I just verified that this fix does not work for my mix of config options (smp && preempt && !rt).

Regards,
Joe

On SMP + !RT migrate_disable() is still around. It is not part of spin_lock() anymore so it has almost no users. However the futex code has a workaround for the !in_atomic() part of migrate disable which fails because the matching migrade_disable() is no longer part of spin_lock().

On !SMP + !RT migrate_disable() is reduced to barrier(). This is not optimal because we few spots where a "preempt_disable()" statement was replaced with "migrate_disable()".

We also used the migration_disable counter to figure out if a sleeping lock is acquired so RCU does not complain about schedule() during rcu_read_lock() while a sleeping lock is held. This changed, we no longer use it, we have now a sleeping_lock counter for the RCU purpose.

This means we can now:
- for SMP + RT_BASE
full migration program, nothing changes here

- for !SMP + RT_BASE
the migration counting is no longer required. It used to ensure that the task is not migrated to another CPU and that this CPU remains online. !SMP ensures that already.
Move it to CONFIG_SCHED_DEBUG so the counting is done for debugging purpose only.

- for all other cases including !RT
fallback to preempt_disable(). The only remaining users of migrate_disable() are those which were converted from preempt_disable() and the futex workaround which is already in the preempt_disable() section due to the spin_lock that is held.

Cc [email protected]

Reported-by [email protected]

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
v1???v2: limit migrate_disable to RT only. Use preempt_disable() for !RT
if migrate_disable() is used.

 include/linux/preempt.h |    6 +++---
 include/linux/sched.h   |    4 ++--
 kernel/sched/core.c     |   23 +++++++++++------------
 kernel/sched/debug.c    |    2 +-
 4 files changed, 17 insertions(+), 18 deletions(-)

On SMP + !RT migrate_disable() is still around. It is not part of spin_lock() anymore so it has almost no users. However the futex code has a workaround for the !in_atomic() part of migrate disable which fails because the matching migrade_disable() is no longer part of spin_lock().

On !SMP + !RT migrate_disable() is reduced to barrier(). This is not optimal because we few spots where a "preempt_disable()" statement was replaced with "migrate_disable()".

We also used the migration_disable counter to figure out if a sleeping lock is acquired so RCU does not complain about schedule() during rcu_read_lock() while a sleeping lock is held. This changed, we no longer use it, we have now a sleeping_lock counter for the RCU purpose.

This means we can now:
- for SMP + RT_BASE
full migration program, nothing changes here

- for !SMP + RT_BASE
the migration counting is no longer required. It used to ensure that the task is not migrated to another CPU and that this CPU remains online. !SMP ensures that already.
Move it to CONFIG_SCHED_DEBUG so the counting is done for debugging purpose only.

- for all other cases including !RT
fallback to preempt_disable(). The only remaining users of migrate_disable() are those which were converted from preempt_disable() and the futex workaround which is already in the preempt_disable() section due to the spin_lock that is held.

Cc [email protected]

Reported-by [email protected]

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
v1…v2: limit migrate_disable to RT only. Use preempt_disable() for !RT
if migrate_disable() is used.

Quoted 31 lines · To Joe Korty

On SMP + !RT migrate_disable() is still around. It is not part of spin_lock() anymore so it has almost no users. However the futex code has a workaround for the !in_atomic() part of migrate disable which fails because the matching migrade_disable() is no longer part of spin_lock().

On !SMP + !RT migrate_disable() is reduced to barrier(). This is not optimal because we few spots where a "preempt_disable()" statement was replaced with "migrate_disable()".

We also used the migration_disable counter to figure out if a sleeping lock is acquired so RCU does not complain about schedule() during rcu_read_lock() while a sleeping lock is held. This changed, we no longer use it, we have now a sleeping_lock counter for the RCU purpose.

This means we can now:
- for SMP + RT_BASE
full migration program, nothing changes here

- for !SMP + RT_BASE
the migration counting is no longer required. It used to ensure that the task is not migrated to another CPU and that this CPU remains online. !SMP ensures that already.
Move it to CONFIG_SCHED_DEBUG so the counting is done for debugging purpose only.

- for all other cases including !RT
fallback to preempt_disable(). The only remaining users of migrate_disable() are those which were converted from preempt_disable() and the futex workaround which is already in the preempt_disable() section due to the spin_lock that is held.

Quoted 6 lines

Cc [email protected]

Reported-by [email protected]

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
v1…v2: limit migrate_disable to RT only. Use preempt_disable() for !RT
if migrate_disable() is used.

If there are no objections I would pick this up for next v4.16.

migrate_disable() does nothing !SMP && !RT. This is bad for two reasons:
- The futex code relies on the fact migrate_disable() is part of spin_lock(). There is a workaround for the !in_atomic() case in migrate_disable() which work-arounds the different ordering (non-atomic lock and atomic unlock).

- we have a few instances where preempt_disable() is replaced with migrate_disable().

For both cases it is bad if migrate_disable() ends up as barrier() instead of preempt_disable(). Let migrate_disable() fallback to preempt_disable().

Cc [email protected]

Reported-by [email protected]

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 include/linux/preempt.h | 4 ++--
 kernel/sched/core.c     | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

include/linux/preempt.h +2 -2

index 043e431a7e8e..d46688d521e6 100644
@@ -241,8 +241,8 @@ static inline int __migrate_disabled(struct task_struct *p)
 }
 
 #else
-#define migrate_disable()		barrier()
-#define migrate_enable()		barrier()
+#define migrate_disable()		preempt_disable()
+#define migrate_enable()		preempt_enable()
 static inline int __migrate_disabled(struct task_struct *p)
 {
 	return 0;

kernel/sched/core.c +2 -0

index ac3fb8495bd5..626a62218518 100644
@@ -7326,6 +7326,7 @@ void migrate_disable(void)
 #endif
 
 	p->migrate_disable++;
+	preempt_disable();
 }
 EXPORT_SYMBOL(migrate_disable);
 

@@ -7349,6 +7350,7 @@ void migrate_enable(void)
 
 	WARN_ON_ONCE(p->migrate_disable <= 0);
 	p->migrate_disable--;
+	preempt_enable();
 }
 EXPORT_SYMBOL(migrate_enable);
 #endif

[ Added Peter ]

Quoted 4 lines · On Thu, 5 Jul 2018 17:50:34 +0200 Sebastian Andrzej Siewior…

migrate_disable() does nothing !SMP && !RT. This is bad for two reasons:
- The futex code relies on the fact migrate_disable() is part of spin_lock(). There is a workaround for the !in_atomic() case in migrate_disable() which work-arounds the different ordering (non-atomic lock and atomic unlock).

But isn't it only part of spin_lock() in the RT case?

Quoted 3 lines

- we have a few instances where preempt_disable() is replaced with migrate_disable().

What? Really? I thought we only replace preempt_disable() with a local_lock(). Which gives annotation to why a preempt_disable() exists. And on non-RT, local_lock() is preempt_disable().

Quoted 3 lines

For both cases it is bad if migrate_disable() ends up as barrier() instead of preempt_disable(). Let migrate_disable() fallback to preempt_disable().

I still don't understand exactly what is "bad" about it.

IIRC, I remember Peter not wanting any open coded "migrate_disable" calls. It was to be for internal use cases only, and specifically, only for RT.

Personally, I think making migrate_disable() into preempt_disable() on NON_RT is incorrect too.

-- Steve