sched/preempt: fix cond_resched_lock() and cond_resched_softirq()

PATCH RESEND In review Konstantin Khlebnikov 2 participants 3 messages May 14, 2015 – May 27, 2015

Subsystems (6)

x86 PowerPC Scheduler Generic include/asm header files +2 more subsystems

lore

Share & tools

lore

Konstantin Khlebnikov

author

2015-05-14 16:23 UTC #0 lore raw

PATCH RESEND 1/1 sched/preempt: fix cond_resched_lock() and cond_resched_softirq()

These functions check should_resched() before unlocking spinlock/bh-enable:
preempt_count always non-zero => should_resched() always returns false. cond_resched_lock() works iff spin_needbreak is set.

This patch adds argument "preempt_offset" to should_resched() add rearranges preempt_count offset constants for that:

PREEMPT_OFFSET - offset after preempt_disable() (0 if CONFIG_PREEMPT_COUNT=n) PREEMPT_LOCK_OFFSET - offset after spin_lock() (alias for PREEMPT_OFFSET) SOFTIRQ_DISABLE_OFFSET - offset after local_bh_distable() SOFTIRQ_LOCK_OFFSET - offset after spin_lock_bh()

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 arch/powerpc/kvm/book3s_hv.c   |    2 +-
 arch/x86/include/asm/preempt.h |    4 ++--
 drivers/xen/preempt.c          |    2 +-
 include/asm-generic/preempt.h  |    4 ++--
 include/linux/preempt.h        |    5 +++--
 include/linux/preempt_mask.h   |   21 ++++++++++++++-------
 include/linux/sched.h          |    6 ------
 kernel/sched/core.c            |    6 +++---
 8 files changed, 26 insertions(+), 24 deletions(-)

arch/powerpc/kvm/book3s_hv.c +1 -1

index 48d3c5d2ecc9..6e73b74c0c60 100644
@@ -2177,7 +2177,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		vc->runner = vcpu;
 		if (n_ceded == vc->n_runnable) {
 			kvmppc_vcore_blocked(vc);
-		} else if (should_resched()) {
+		} else if (should_resched(PREEMPT_LOCK_OFFSET)) {
 			vc->vcore_state = VCORE_PREEMPT;
 			/* Let something else run */
 			cond_resched_lock(&vc->lock);

arch/x86/include/asm/preempt.h +2 -2

index 8f3271842533..67b6cd00a44f 100644
@@ -90,9 +90,9 @@ static __always_inline bool __preempt_count_dec_and_test(void)
 /*
  * Returns true when we need to resched and can (barring IRQ state).
  */
-static __always_inline bool should_resched(void)
+static __always_inline bool should_resched(int preempt_offset)
 {
-	return unlikely(!raw_cpu_read_4(__preempt_count));
+	return unlikely(raw_cpu_read_4(__preempt_count) == preempt_offset);
 }
 
 #ifdef CONFIG_PREEMPT

drivers/xen/preempt.c +1 -1

index a1800c150839..46188cf41d08 100644
@@ -31,7 +31,7 @@ EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
 asmlinkage __visible void xen_maybe_preempt_hcall(void)
 {
 	if (unlikely(__this_cpu_read(xen_in_preemptible_hcall)
-		     && should_resched())) {
+		     && should_resched(0))) {
 		/*
 		 * Clear flag as we may be rescheduled on a different
 		 * cpu.

include/asm-generic/preempt.h +2 -2

index eb6f9e6c3075..e91fb799a6da 100644
@@ -71,9 +71,9 @@ static __always_inline bool __preempt_count_dec_and_test(void)
 /*
  * Returns true when we need to resched and can (barring IRQ state).
  */
-static __always_inline bool should_resched(void)
+static __always_inline bool should_resched(int offset)
 {
-	return unlikely(!preempt_count() && tif_need_resched());
+	return unlikely(preempt_count() == offset && tif_need_resched());
 }
 
 #ifdef CONFIG_PREEMPT

include/linux/preempt.h +3 -2

index de83b4eb1642..8cd6725c5758 100644
@@ -20,7 +20,8 @@
 #if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
 extern void preempt_count_add(int val);
 extern void preempt_count_sub(int val);
-#define preempt_count_dec_and_test() ({ preempt_count_sub(1); should_resched(); })
+#define preempt_count_dec_and_test() \
+	({ preempt_count_sub(1); should_resched(0); })
 #else
 #define preempt_count_add(val)	__preempt_count_add(val)
 #define preempt_count_sub(val)	__preempt_count_sub(val)

@@ -59,7 +60,7 @@ do { \
 
 #define preempt_check_resched() \
 do { \
-	if (should_resched()) \
+	if (should_resched(0)) \
 		__preempt_schedule(); \
 } while (0)

include/linux/preempt_mask.h +14 -7

index dbeec4d4a3be..a832c243da0b 100644
@@ -39,7 +39,15 @@
 #define HARDIRQ_MASK	(__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
 #define NMI_MASK	(__IRQ_MASK(NMI_BITS)     << NMI_SHIFT)
 
+/*
+ * The preempt_count offset after preempt_disable();
+ */
+#ifdef CONFIG_PREEMPT_COUNT
 #define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
+#else
+#define PREEMPT_OFFSET	0
+#endif
+
 #define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
 #define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
 #define NMI_OFFSET	(1UL << NMI_SHIFT)

@@ -71,11 +79,10 @@
  */
 #define in_nmi()	(preempt_count() & NMI_MASK)
 
-#if defined(CONFIG_PREEMPT_COUNT)
-# define PREEMPT_CHECK_OFFSET 1
-#else
-# define PREEMPT_CHECK_OFFSET 0
-#endif
+/*
+ * The preempt_count offset after spin_lock()
+ */
+#define PREEMPT_LOCK_OFFSET	PREEMPT_OFFSET
 
 /*
  * The preempt_count offset needed for things like:

@@ -90,7 +97,7 @@
  *
  * Work as expected.
  */
-#define SOFTIRQ_LOCK_OFFSET (SOFTIRQ_DISABLE_OFFSET + PREEMPT_CHECK_OFFSET)
+#define SOFTIRQ_LOCK_OFFSET (SOFTIRQ_DISABLE_OFFSET + PREEMPT_LOCK_OFFSET)
 
 /*
  * Are we running in atomic context?  WARNING: this macro cannot

@@ -106,7 +113,7 @@
  * (used by the scheduler, *after* releasing the kernel lock)
  */
 #define in_atomic_preempt_off() \
-		((preempt_count() & ~PREEMPT_ACTIVE) != PREEMPT_CHECK_OFFSET)
+		((preempt_count() & ~PREEMPT_ACTIVE) != PREEMPT_OFFSET)
 
 #ifdef CONFIG_PREEMPT_COUNT
 # define preemptible()	(preempt_count() == 0 && !irqs_disabled())

include/linux/sched.h +0 -6

index 26a2e6122734..61f4f2d5c882 100644
@@ -2834,12 +2834,6 @@ extern int _cond_resched(void);
 
 extern int __cond_resched_lock(spinlock_t *lock);
 
-#ifdef CONFIG_PREEMPT_COUNT
-#define PREEMPT_LOCK_OFFSET	PREEMPT_OFFSET
-#else
-#define PREEMPT_LOCK_OFFSET	0
-#endif
-
 #define cond_resched_lock(lock) ({				\
 	___might_sleep(__FILE__, __LINE__, PREEMPT_LOCK_OFFSET);\
 	__cond_resched_lock(lock);				\

kernel/sched/core.c +3 -3

index fe22f7510bce..087bf36ecd46 100644
@@ -4223,7 +4223,7 @@ SYSCALL_DEFINE0(sched_yield)
 
 int __sched _cond_resched(void)
 {
-	if (should_resched()) {
+	if (should_resched(0)) {
 		preempt_schedule_common();
 		return 1;
 	}

@@ -4241,7 +4241,7 @@ EXPORT_SYMBOL(_cond_resched);
  */
 int __cond_resched_lock(spinlock_t *lock)
 {
-	int resched = should_resched();
+	int resched = should_resched(PREEMPT_LOCK_OFFSET);
 	int ret = 0;
 
 	lockdep_assert_held(lock);

@@ -4263,7 +4263,7 @@ int __sched __cond_resched_softirq(void)
 {
 	BUG_ON(!in_softirq());
 
-	if (should_resched()) {
+	if (should_resched(SOFTIRQ_DISABLE_OFFSET)) {
 		local_bh_enable();
 		preempt_schedule_common();
 		local_bh_disable();

Peter Zijlstra

maintainer

2015-05-27 09:09 UTC #1 lore raw

Quoted 7 lines · Konstantin Khlebnikov

These functions check should_resched() before unlocking spinlock/bh-enable:
preempt_count always non-zero => should_resched() always returns false. cond_resched_lock() works iff spin_needbreak is set.
This patch adds argument "preempt_offset" to should_resched() add rearranges preempt_count offset constants for that:

Quoted 6 lines

PREEMPT_OFFSET - offset after preempt_disable() (0 if CONFIG_PREEMPT_COUNT=n) PREEMPT_LOCK_OFFSET - offset after spin_lock() (alias for PREEMPT_OFFSET) SOFTIRQ_DISABLE_OFFSET - offset after local_bh_distable() SOFTIRQ_LOCK_OFFSET - offset after spin_lock_bh()
Signed-off-by Konstantin Khlebnikov <[email protected]>

Sorry, but it doesn't apply anymore because of that whole pagefault_disable() muck we merged.

arch/powerpc/kvm/book3s_hv.c static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) · line 2180

Quoted 10 lines

arch/powerpc/kvm/book3s_hv.c +1 -1

index 48d3c5d2ecc9..6e73b74c0c60 100644
@@ -2177,7 +2177,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		vc->runner = vcpu;
 		if (n_ceded == vc->n_runnable) {
 			kvmppc_vcore_blocked(vc);
-		} else if (should_resched()) {
+		} else if (should_resched(PREEMPT_LOCK_OFFSET)) {

I'm thinking this wants to be: need_resched() ?

Quoted 10 lines

arch/powerpc/kvm/book3s_hv.c +0 -0

 			vc->vcore_state = VCORE_PREEMPT;
 			/* Let something else run */
 			cond_resched_lock(&vc->lock);

include/asm-generic/preempt.h +0 -0

index eb6f9e6c3075..e91fb799a6da 100644
@@ -71,9 +71,9 @@ static __always_inline bool __preempt_count_dec_and_test(void)
 /*

include/asm-generic/preempt.h static __always_inline bool __preempt_count_dec_and_test(void) · line 79

Quoted 10 lines

include/asm-generic/preempt.h +2 -2

  * Returns true when we need to resched and can (barring IRQ state).
  */
-static __always_inline bool should_resched(void)
+static __always_inline bool should_resched(int offset)
 {
-	return unlikely(!preempt_count() && tif_need_resched());
+	return unlikely(preempt_count() == offset && tif_need_resched());
 }
 
 #ifdef CONFIG_PREEMPT

So the reason I held off on this patch for a wee bit is because I don't like the should_resched() change you did; although I fully understand why you did it.

That said, I could not come up with anything better either and I suppose that once we fix that ppc-kvm user, there really isn't a user left outside of core code and thus we can deal with a slightly dangerous function.

I did not really look, but it would be good if we could also get rid of the Xen usage.

Konstantin Khlebnikov

author

2015-05-27 09:19 UTC #2 lore raw

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Konstantin Khlebnikov
These functions check should_resched() before unlocking spinlock/bh-enable:
preempt_count always non-zero => should_resched() always returns false. cond_resched_lock() works iff spin_needbreak is set.
This patch adds argument "preempt_offset" to should_resched() add rearranges preempt_count offset constants for that:
PREEMPT_OFFSET - offset after preempt_disable() (0 if CONFIG_PREEMPT_COUNT=n) PREEMPT_LOCK_OFFSET - offset after spin_lock() (alias for PREEMPT_OFFSET) SOFTIRQ_DISABLE_OFFSET - offset after local_bh_distable() SOFTIRQ_LOCK_OFFSET - offset after spin_lock_bh()

Quoted 4 lines

Quoted 1 line
Signed-off-by Konstantin Khlebnikov <[email protected]>
Sorry, but it doesn't apply anymore because of that whole pagefault_disable() muck we merged.

No problem. I'll dig into this stuff again later.
This bug is harmless and it's here for ages.

Quoted 47 lines

arch/powerpc/kvm/book3s_hv.c static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) · line 2180
Quoted 11 lines
arch/powerpc/kvm/book3s_hv.c +1 -1
index 48d3c5d2ecc9..6e73b74c0c60 100644
@@ -2177,7 +2177,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
  		vc->runner = vcpu;
  		if (n_ceded == vc->n_runnable) {
  			kvmppc_vcore_blocked(vc);
-		} else if (should_resched()) {
+		} else if (should_resched(PREEMPT_LOCK_OFFSET)) {
I'm thinking this wants to be: need_resched() ?
Quoted 10 lines
arch/powerpc/kvm/book3s_hv.c +0 -0
  			vc->vcore_state = VCORE_PREEMPT;
  			/* Let something else run */
  			cond_resched_lock(&vc->lock);
include/asm-generic/preempt.h +0 -0
index eb6f9e6c3075..e91fb799a6da 100644
@@ -71,9 +71,9 @@ static __always_inline bool __preempt_count_dec_and_test(void)
  /*
include/asm-generic/preempt.h static __always_inline bool __preempt_count_dec_and_test(void) · line 79
Quoted 10 lines
include/asm-generic/preempt.h +2 -2
   * Returns true when we need to resched and can (barring IRQ state).
   */
-static __always_inline bool should_resched(void)
+static __always_inline bool should_resched(int offset)
  {
-	return unlikely(!preempt_count() && tif_need_resched());
+	return unlikely(preempt_count() == offset && tif_need_resched());
  }
 
  #ifdef CONFIG_PREEMPT
So the reason I held off on this patch for a wee bit is because I don't like the should_resched() change you did; although I fully understand why you did it.
That said, I could not come up with anything better either and I suppose that once we fix that ppc-kvm user, there really isn't a user left outside of core code and thus we can deal with a slightly dangerous function.
I did not really look, but it would be good if we could also get rid of the Xen usage.

Signature · 1 line

Konstantin