Skip to contentSkip to conversation

sched: Add schedule_(raw_)spin_unlock and schedule_(raw_)spin_unlock_irq

PATCH In review Kirill Tkhai 3 participants 4 messages Jun 18, 2013 – Jun 24, 2013

Subsystems (2)
Scheduler Locking primitives
lore
Share & tools
lore

Kirill Tkhai

author
#0 lore raw

PATCH 1/2 sched: Add schedule_(raw_)spin_unlock and schedule_(raw_)spin_unlock_irq

Helpers for replacement repeating patterns:

1)spin_unlock(lock);
schedule();
2)spin_unlock_irq(lock);
schedule();

(The same for raw_spinlock_t)

This allows to prevent excess preempt_schedule(), which can happen on preemptible kernel.

Signed-off-by Kirill Tkhai <[email protected]>
CC Steven Rostedt <[email protected]>
CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
---
 include/linux/sched.h            |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/spinlock_api_smp.h |   21 +++++++++++++++++++++
 include/linux/spinlock_api_up.h  |    8 ++++++++
 kernel/spinlock.c                |   13 +++++++++++++
 4 files changed, 79 insertions(+), 0 deletions(-)
include/linux/sched.h +37 -0
index 8b10339..2020071 100644
@@ -310,6 +310,43 @@ extern signed long schedule_timeout_uninterruptible(signed long timeout);
 asmlinkage void schedule(void);
 extern void schedule_preempt_disabled(void);
 
+/*
+ * schedule_raw_spin_unlock() -- should be used instead of pattern:
+ *
+ *     raw_spin_unlock(lock);
+ *     schedule();
+ *
+ * It's the same, but prevents preempt_schedule() call during the unlocking.
+ */
+static inline void schedule_raw_spin_unlock(raw_spinlock_t *lock)
+{
+	_raw_spin_unlock_no_resched(lock);
+	schedule();
+}
+
+/*
+ * schedule_raw_spin_unlock_irq() -- should be used instead of pattern:
+ *
+ *     raw_spin_unlock_irq(lock);
+ *     schedule();
+ *
+ * It's the same, but prevents preempt_schedule() call during the unlocking.
+ */
+static inline void schedule_raw_spin_unlock_irq(raw_spinlock_t *lock)
+{
+	_raw_spin_unlock_irq_no_resched(lock);
+	schedule();
+}
+
+static inline void schedule_spin_unlock(spinlock_t *lock)
+{
+	schedule_raw_spin_unlock(&lock->rlock);
+}
+static inline void schedule_spin_unlock_irq(spinlock_t *lock)
+{
+	schedule_raw_spin_unlock_irq(&lock->rlock);
+}
+
 struct nsproxy;
 struct user_namespace;
include/linux/spinlock_api_smp.h +21 -0
index 51df117..42dbdfe 100644
@@ -42,6 +42,10 @@ void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock)	__releases(lock);
 void __lockfunc
 _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
 								__releases(lock);
+void __lockfunc
+_raw_spin_unlock_no_resched(raw_spinlock_t *lock)		__releases(lock);
+void __lockfunc
+_raw_spin_unlock_irq_no_resched(raw_spinlock_t *lock)		__releases(lock);
 
 #ifdef CONFIG_INLINE_SPIN_LOCK
 #define _raw_spin_lock(lock) __raw_spin_lock(lock)

@@ -69,6 +73,7 @@ _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
 
 #ifndef CONFIG_UNINLINE_SPIN_UNLOCK
 #define _raw_spin_unlock(lock) __raw_spin_unlock(lock)
+#define _raw_spin_unlock_no_resched(lock) __raw_spin_unlock_no_resched(lock)
 #endif
 
 #ifdef CONFIG_INLINE_SPIN_UNLOCK_BH

@@ -77,6 +82,7 @@ _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
 
 #ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ
 #define _raw_spin_unlock_irq(lock) __raw_spin_unlock_irq(lock)
+#define _raw_spin_unlock_irq_no_resched(lock) __raw_spin_unlock_irq_no_resched(lock)
 #endif
 
 #ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE

@@ -153,6 +159,13 @@ static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 	preempt_enable();
 }
 
+static inline void __raw_spin_unlock_no_resched(raw_spinlock_t *lock)
+{
+	spin_release(&lock->dep_map, 1, _RET_IP_);
+	do_raw_spin_unlock(lock);
+	preempt_enable_no_resched();
+}
+
 static inline void __raw_spin_unlock_irqrestore(raw_spinlock_t *lock,
 					    unsigned long flags)
 {

@@ -170,6 +183,14 @@ static inline void __raw_spin_unlock_irq(raw_spinlock_t *lock)
 	preempt_enable();
 }
 
+static inline void __raw_spin_unlock_irq_no_resched(raw_spinlock_t *lock)
+{
+	spin_release(&lock->dep_map, 1, _RET_IP_);
+	do_raw_spin_unlock(lock);
+	local_irq_enable();
+	preempt_enable_no_resched();
+}
+
 static inline void __raw_spin_unlock_bh(raw_spinlock_t *lock)
 {
 	spin_release(&lock->dep_map, 1, _RET_IP_);
include/linux/spinlock_api_up.h +8 -0
index af1f472..82280da 100644
@@ -39,6 +39,9 @@
 #define __UNLOCK(lock) \
   do { preempt_enable(); __release(lock); (void)(lock); } while (0)
 
+#define __UNLOCK_NO_RESCHED(lock) \
+  do { preempt_enable_no_resched(); __release(lock); (void)(lock); } while (0)
+
 #define __UNLOCK_BH(lock) \
   do { preempt_enable_no_resched(); local_bh_enable(); \
 	  __release(lock); (void)(lock); } while (0)

@@ -46,6 +49,9 @@
 #define __UNLOCK_IRQ(lock) \
   do { local_irq_enable(); __UNLOCK(lock); } while (0)
 
+#define __UNLOCK_IRQ_NO_RESCHED(lock) \
+  do { local_irq_enable(); __UNLOCK_NO_RESCHED(lock); } while (0)
+
 #define __UNLOCK_IRQRESTORE(lock, flags) \
   do { local_irq_restore(flags); __UNLOCK(lock); } while (0)
 

@@ -67,12 +73,14 @@
 #define _raw_write_trylock(lock)			({ __LOCK(lock); 1; })
 #define _raw_spin_trylock_bh(lock)		({ __LOCK_BH(lock); 1; })
 #define _raw_spin_unlock(lock)			__UNLOCK(lock)
+#define _raw_spin_unlock_no_resched(lock)	__UNLOCK_NO_RESCHED(lock)
 #define _raw_read_unlock(lock)			__UNLOCK(lock)
 #define _raw_write_unlock(lock)			__UNLOCK(lock)
 #define _raw_spin_unlock_bh(lock)		__UNLOCK_BH(lock)
 #define _raw_write_unlock_bh(lock)		__UNLOCK_BH(lock)
 #define _raw_read_unlock_bh(lock)		__UNLOCK_BH(lock)
 #define _raw_spin_unlock_irq(lock)		__UNLOCK_IRQ(lock)
+#define _raw_spin_unlock_irq_no_resched(lock)	__UNLOCK_IRQ_NO_RESCHED(lock)
 #define _raw_read_unlock_irq(lock)		__UNLOCK_IRQ(lock)
 #define _raw_write_unlock_irq(lock)		__UNLOCK_IRQ(lock)
 #define _raw_spin_unlock_irqrestore(lock, flags) \
kernel/spinlock.c +13 -0
index 5cdd806..218058f 100644
@@ -169,6 +169,13 @@ void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock)
 	__raw_spin_unlock(lock);
 }
 EXPORT_SYMBOL(_raw_spin_unlock);
+
+void __lockfunc _raw_spin_unlock_no_resched(raw_spinlock_t *lock)
+{
+	__raw_spin_unlock_no_resched(lock);
+}
+EXPORT_SYMBOL(_raw_spin_unlock_no_resched);
+
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE

@@ -185,6 +192,12 @@ void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock)
 	__raw_spin_unlock_irq(lock);
 }
 EXPORT_SYMBOL(_raw_spin_unlock_irq);
+
+void __lockfunc _raw_spin_unlock_irq_no_resched(raw_spinlock_t *lock)
+{
+	__raw_spin_unlock_irq_no_resched(lock);
+}
+EXPORT_SYMBOL(_raw_spin_unlock_irq_no_resched);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_UNLOCK_BH
maintainer
#1 lore raw
Quoted 7 lines · Kirill Tkhai

Helpers for replacement repeating patterns:

1)spin_unlock(lock);
schedule();
2)spin_unlock_irq(lock);
schedule();

I just noticed this; the existing schedule_preempt_disabled() is equivalent to:

preempt_enable()
schedule()
preempt_disable()

So I somewhat expected these new primitives to be:

spin_unlock()
schedule()
spin_lock()

Now I haven't actually looked at the usage patch to see what the converted sites look like (thanks for adding that one though!).

My OCD just triggered on the preemption and locked schedule calls having different semantics.

author
#2 lore raw

18.06.2013, 21:28, "Peter Zijlstra" <[email protected]>:

Quoted 22 lines · Kirill Tkhai
Quoted 7 lines

 Helpers for replacement repeating patterns:

 1)spin_unlock(lock);
   schedule();
 2)spin_unlock_irq(lock);
   schedule();

I just noticed this; the existing schedule_preempt_disabled() is equivalent to:

  preempt_enable()
  schedule()
  preempt_disable()

So I somewhat expected these new primitives to be:

  spin_unlock()
  schedule()
  spin_lock()

Quoted 5 lines

Now I haven't actually looked at the usage patch to see what the converted sites look like (thanks for adding that one though!).

My OCD just triggered on the preemption and locked schedule calls having different semantics.

They have different semantic and different ending.

Many places (as you can see from the second patch) need additional actions between schedule() and next spin_lock(). Several places don't do the second lock.

Kirill

#3 lore raw
In reply to Kirill Tkhai

include/linux/sched.h extern signed long schedule_timeout_uninterruptible(signed long timeout); · line 321

Quoted 9 lines · Kirill Tkhai
include/linux/sched.h +9 -0
+/*
+ * schedule_raw_spin_unlock() -- should be used instead of pattern:
+ *
+ *     raw_spin_unlock(lock);
+ *     schedule();
+ *
+ * It's the same, but prevents preempt_schedule() call during the unlocking.
+ */
+static inline void schedule_raw_spin_unlock(raw_spinlock_t *lock)

This should be raw_spin_unlock_and_schedule().

schedule_raw_spin_unlock() sounds like we schedule a raw_spin_unlock() for some point in the future.

include/linux/sched.h extern signed long schedule_timeout_uninterruptible(signed long timeout); · line 323

Quoted 2 lines
include/linux/sched.h +2 -0
+{
+	_raw_spin_unlock_no_resched(lock);

No, please do not expose such an interface. Instead of that implement it as:

raw_spin_unlock_and_schedule()

{
	spin_release(&lock->dep_map, 1, _RET_IP_);
	do_raw_spin_unlock(lock);
	preempt_enable_no_resched();
	schedule();  
}

And this goes into the spinlock header and not into sched.h.

Thanks,

tglx