spin_unlock*_no_resched()

PATCH In review Kirill Tkhai 3 participants 6 messages Jun 12, 2013 – Jun 18, 2013 Locking primitives

lore

Share & tools

lore

Kirill Tkhai

author

2013-06-12 12:06 UTC #0 lore raw

PATCH 1/1 spin_unlock*_no_resched()

There are many constructions like:

	spin_unlock_irq(lock);
	schedule();

In case of preemptible kernel we check if task needs reschedule at the end of spin_unlock(). So if TIF_NEED_RESCHED is set we call schedule() twice and we have a little overhead here. Add primitives to avoid these situations.

Signed-off-by Kirill Tkhai <[email protected]>

CC Steven Rostedt <[email protected]>

CC Ingo Molnar <[email protected]>

CC Peter Zijlstra <[email protected]>

---
 include/linux/spinlock.h         |   27 +++++++++++++++++++++++++++
 include/linux/spinlock_api_smp.h |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/spinlock_api_up.h  |   13 +++++++++++++
 kernel/spinlock.c                |   20 ++++++++++++++++++++
 4 files changed, 97 insertions(+), 0 deletions(-)

include/linux/spinlock.h +27 -0

index 7d537ce..35caa32 100644
@@ -221,13 +221,24 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 #define raw_spin_lock_irq(lock)		_raw_spin_lock_irq(lock)
 #define raw_spin_lock_bh(lock)		_raw_spin_lock_bh(lock)
 #define raw_spin_unlock(lock)		_raw_spin_unlock(lock)
+#define raw_spin_unlock_no_resched(lock)	\
+	_raw_spin_unlock_no_resched(lock)
+
 #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)
 
 #define raw_spin_unlock_irqrestore(lock, flags)		\
 	do {							\
 		typecheck(unsigned long, flags);		\
 		_raw_spin_unlock_irqrestore(lock, flags);	\
 	} while (0)
+#define raw_spin_unlock_irqrestore_no_resched(lock, flags)	\
+	do {							\
+		typecheck(unsigned long, flags);		\
+		_raw_spin_unlock_irqrestore_no_resched(lock, flags);	\
+	} while (0)
+
 #define raw_spin_unlock_bh(lock)	_raw_spin_unlock_bh(lock)
 
 #define raw_spin_trylock_bh(lock) \

@@ -325,6 +336,11 @@ static inline void spin_unlock(spinlock_t *lock)
 	raw_spin_unlock(&lock->rlock);
 }
 
+static inline void spin_unlock_no_resched(spinlock_t *lock)
+{
+	raw_spin_unlock_no_resched(&lock->rlock);
+}
+
 static inline void spin_unlock_bh(spinlock_t *lock)
 {
 	raw_spin_unlock_bh(&lock->rlock);

@@ -335,11 +351,22 @@ static inline void spin_unlock_irq(spinlock_t *lock)
 	raw_spin_unlock_irq(&lock->rlock);
 }
 
+static inline void spin_unlock_irq_no_resched(spinlock_t *lock)
+{
+	raw_spin_unlock_irq_no_resched(&lock->rlock);
+}
+
 static inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
 {
 	raw_spin_unlock_irqrestore(&lock->rlock, flags);
 }
 
+static inline void spin_unlock_irqrestore_no_resched(spinlock_t *lock,
+						     unsigned long flags)
+{
+	raw_spin_unlock_irqrestore_no_resched(&lock->rlock, flags);
+}
+
 static inline int spin_trylock_bh(spinlock_t *lock)
 {
 	return raw_spin_trylock_bh(&lock->rlock);

include/linux/spinlock_api_smp.h +37 -0

index 51df117..541d371 100644
@@ -37,11 +37,18 @@ _raw_spin_lock_irqsave_nested(raw_spinlock_t *lock, int subclass)
 int __lockfunc _raw_spin_trylock(raw_spinlock_t *lock);
 int __lockfunc _raw_spin_trylock_bh(raw_spinlock_t *lock);
 void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock)		__releases(lock);
+void __lockfunc
+_raw_spin_unlock_no_resched(raw_spinlock_t *lock)		__releases(lock);
 void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock)	__releases(lock);
 void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock)	__releases(lock);
 void __lockfunc
+_raw_spin_unlock_irq_no_resched(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_irqrestore_no_resched(raw_spinlock_t *lock, unsigned long flags)
+								__releases(lock);
 
 #ifdef CONFIG_INLINE_SPIN_LOCK
 #define _raw_spin_lock(lock) __raw_spin_lock(lock)

@@ -69,6 +76,8 @@ _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,10 +86,14 @@ _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
 #define _raw_spin_unlock_irqrestore(lock, flags) __raw_spin_unlock_irqrestore(lock, flags)
+#define _raw_spin_unlock_irqrestore_no_resched(lock, flags)	\
+	__raw_spin_unlock_irqrestore_no_resched(lock, flags)
 #endif
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)

@@ -153,6 +166,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)
 {

@@ -162,6 +182,15 @@ static inline void __raw_spin_unlock_irqrestore(raw_spinlock_t *lock,
 	preempt_enable();
 }
 
+static inline void __raw_spin_unlock_irqrestore_no_resched(raw_spinlock_t *lock,
+							   unsigned long flags)
+{
+	spin_release(&lock->dep_map, 1, _RET_IP_);
+	do_raw_spin_unlock(lock);
+	local_irq_restore(flags);
+	preempt_enable_no_resched();
+}
+
 static inline void __raw_spin_unlock_irq(raw_spinlock_t *lock)
 {
 	spin_release(&lock->dep_map, 1, _RET_IP_);

@@ -170,6 +199,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 +13 -0

index af1f472..6f78980 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,9 +49,15 @@
 #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)
 
+#define __UNLOCK_IRQRESTORE_NO_RESCHED(lock, flags) \
+  do { local_irq_restore(flags); __UNLOCK_NO_RESCHED(lock); } while (0)
+
 #define _raw_spin_lock(lock)			__LOCK(lock)
 #define _raw_spin_lock_nested(lock, subclass)	__LOCK(lock)
 #define _raw_read_lock(lock)			__LOCK(lock)

@@ -67,16 +76,20 @@
 #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) \
 					__UNLOCK_IRQRESTORE(lock, flags)
+#define _raw_spin_unlock_irqrestore_no_resched(lock, flags) \
+				__UNLOCK_IRQRESTORE_NO_RESCHED(lock, flags)
 #define _raw_read_unlock_irqrestore(lock, flags) \
 					__UNLOCK_IRQRESTORE(lock, flags)
 #define _raw_write_unlock_irqrestore(lock, flags) \

kernel/spinlock.c +20 -0

index 5cdd806..c52bdf7 100644
@@ -169,6 +169,12 @@ 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

@@ -177,6 +183,13 @@ void __lockfunc _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long
 	__raw_spin_unlock_irqrestore(lock, flags);
 }
 EXPORT_SYMBOL(_raw_spin_unlock_irqrestore);
+
+void __lockfunc _raw_spin_unlock_irqrestore_no_resched(raw_spinlock_t *lock,
+						       unsigned long flags)
+{
+	__raw_spin_unlock_irqrestore_no_resched(lock, flags);
+}
+EXPORT_SYMBOL(_raw_spin_unlock_irqrestore_no_resched);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ

@@ -185,6 +198,13 @@ 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

Peter Zijlstra

maintainer

2013-06-12 12:15 UTC #1 lore raw

Quoted 35 lines · Kirill Tkhai

There are many constructions like:
	spin_unlock_irq(lock);
	schedule();
In case of preemptible kernel we check if task needs reschedule at the end of spin_unlock(). So if TIF_NEED_RESCHED is set we call schedule() twice and we have a little overhead here. Add primitives to avoid these situations.
Signed-off-by Kirill Tkhai <[email protected]>
CC Steven Rostedt <[email protected]>
CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
---
 include/linux/spinlock.h         |   27 +++++++++++++++++++++++++++
 include/linux/spinlock_api_smp.h |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/spinlock_api_up.h  |   13 +++++++++++++
 kernel/spinlock.c                |   20 ++++++++++++++++++++
 4 files changed, 97 insertions(+), 0 deletions(-)
include/linux/spinlock.h +5 -0
index 7d537ce..35caa32 100644
@@ -221,13 +221,24 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 #define raw_spin_lock_irq(lock)		_raw_spin_lock_irq(lock)
 #define raw_spin_lock_bh(lock)		_raw_spin_lock_bh(lock)
 #define raw_spin_unlock(lock)		_raw_spin_unlock(lock)
+#define raw_spin_unlock_no_resched(lock)	\
+	_raw_spin_unlock_no_resched(lock)
+
 #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)
 

include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 240

Quoted 10 lines

include/linux/spinlock.h +5 -0

 #define raw_spin_unlock_irqrestore(lock, flags)		\
 	do {							\
 		typecheck(unsigned long, flags);		\
 		_raw_spin_unlock_irqrestore(lock, flags);	\
 	} while (0)
+#define raw_spin_unlock_irqrestore_no_resched(lock, flags)	\
+	do {							\
+		typecheck(unsigned long, flags);		\
+		_raw_spin_unlock_irqrestore_no_resched(lock, flags);	\
+	} while (0)

So I absolutely hate this API because people can (and invariably will) abuse it; much like they did/do preempt_enable_no_resched().

IIRC Thomas even maps preempt_enable_no_resched() to preempt_enable() in -rt to make sure we don't miss preemption points due to stupidity.

He converted the 'few' sane sites to use schedule_preempt_disabled(). In that vein, does it make sense to introduce schedule_spin_locked()?

Also, your patch 'fails' to make use of the new API.

Steven Rostedt

2013-06-12 13:07 UTC #2 lore raw

Quoted 2 lines · Peter Zijlstra

So I absolutely hate this API because people can (and invariably will) abuse it; much like they did/do preempt_enable_no_resched().

Me too.

Quoted 7 lines

IIRC Thomas even maps preempt_enable_no_resched() to preempt_enable() in -rt to make sure we don't miss preemption points due to stupidity.
He converted the 'few' sane sites to use schedule_preempt_disabled(). In that vein, does it make sense to introduce schedule_spin_locked()?

I was thinking the exact same thing when I read this patch. This is a strict policy that we should enforce and not let individual developers implement. Yes, a schedule_spin_unlock() would work nicely. The API will enforce the two to be used together. Otherwise, I can envision seeing things like:

	preempt_disable();
	[...]

	spin_lock(x);

	spin_unlock_no_resched(x);

	[...]

	preempt_enable();

And developers having no idea why the above is broken. Although, I would say the above is broken for other reasons, but I was just using that to show the craziness such an API would give to us.

-- Steve

Kirill Tkhai

author

2013-06-12 23:05 UTC #3 lore raw

Quoted 11 lines · Steven Rostedt

Quoted 2 lines · Peter Zijlstra
So I absolutely hate this API because people can (and invariably will) abuse it; much like they did/do preempt_enable_no_resched().
Me too.
Quoted 4 lines
IIRC Thomas even maps preempt_enable_no_resched() to preempt_enable() in -rt to make sure we don't miss preemption points due to stupidity.

Quoted 8 lines

Quoted 3 lines
He converted the 'few' sane sites to use schedule_preempt_disabled(). In that vein, does it make sense to introduce schedule_spin_locked()?
I was thinking the exact same thing when I read this patch. This is a strict policy that we should enforce and not let individual developers implement. Yes, a schedule_spin_unlock() would work nicely. The API will enforce the two to be used together.

Steven thanks for your explanation and Peter's, now I looked to this from another side.

If we speak about combined primitive does it have to be a special variant of schedule_spin_unlock_* for every irq state? The simplest way is to do local_irq_enable() always before schedule() call, but I'm not sure that this is good for all platforms.

For -rt everything of this is completely useless, because number of raw_spin_locks is small. Maybe changes for some another types of locks will applicable.

Kirill

Quoted 22 lines

Otherwise, I can envision seeing
things like:
	preempt_disable();
	[...]

	spin_lock(x);

	spin_unlock_no_resched(x);

	[...]

	preempt_enable();
And developers having no idea why the above is broken. Although, I would say the above is broken for other reasons, but I was just using that to show the craziness such an API would give to us.
-- Steve

Kirill Tkhai

author

2013-06-13 00:00 UTC #4 lore raw

In reply to Steven Rostedt

Quoted 32 lines · Steven Rostedt

Quoted 2 lines · Peter Zijlstra
So I absolutely hate this API because people can (and invariably will) abuse it; much like they did/do preempt_enable_no_resched().
Me too.
Quoted 7 lines
IIRC Thomas even maps preempt_enable_no_resched() to preempt_enable() in -rt to make sure we don't miss preemption points due to stupidity.
He converted the 'few' sane sites to use schedule_preempt_disabled(). In that vein, does it make sense to introduce schedule_spin_locked()?
I was thinking the exact same thing when I read this patch. This is a strict policy that we should enforce and not let individual developers implement. Yes, a schedule_spin_unlock() would work nicely. The API will enforce the two to be used together. Otherwise, I can envision seeing things like:
	preempt_disable();
	[...]

	spin_lock(x);

	spin_unlock_no_resched(x);

	[...]

	preempt_enable();

Quoted 8 lines

And developers having no idea why the above is broken. Although, I would say the above is broken for other reasons, but I was just using that to show the craziness such an API would give to us.
-- Steve

In additional to my previous letter. If spin_lock is locked then irqs must be disabled. So sorry for the noise.

Peter Zijlstra

maintainer

2013-06-18 10:05 UTC #5 lore raw

In reply to Kirill Tkhai

Quoted 10 lines · Kirill Tkhai

There are many constructions like:
	spin_unlock_irq(lock);
	schedule();
In case of preemptible kernel we check if task needs reschedule at the end of spin_unlock(). So if TIF_NEED_RESCHED is set we call schedule() twice and we have a little overhead here. Add primitives to avoid these situations.

Aside from the excellent comments you received from Steven, please also include a patch that converts some (if not all) of these many callsites you found so that there's a user of the new code in the kernel.

That way we can simply build + run the kernel and have more confidence things work as supposed.