diff mbox series

[RFC,v2,03/10] locking/local_lock: Introduce localtry_lock_t

Message ID 20250214-slub-percpu-caches-v2-3-88592ee0966a@suse.cz (mailing list archive)
State New
Headers show
Series SLUB percpu sheaves | expand

Commit Message

Vlastimil Babka Feb. 14, 2025, 4:27 p.m. UTC
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

In !PREEMPT_RT local_lock_irqsave() disables interrupts to protect
critical section, but it doesn't prevent NMI, so the fully reentrant
code cannot use local_lock_irqsave() for exclusive access.

Introduce localtry_lock_t and localtry_lock_irqsave() that
disables interrupts and sets acquired=1, so localtry_lock_irqsave()
from NMI attempting to acquire the same lock will return false.

In PREEMPT_RT local_lock_irqsave() maps to preemptible spin_lock().
Map localtry_lock_irqsave() to preemptible spin_trylock().
When in hard IRQ or NMI return false right away, since
spin_trylock() is not safe due to PI issues.

Note there is no need to use local_inc for acquired variable,
since it's a percpu variable with strict nesting scopes.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/local_lock.h          |  59 +++++++++++++++++
 include/linux/local_lock_internal.h | 123 ++++++++++++++++++++++++++++++++++++
 2 files changed, 182 insertions(+)

Comments

Sebastian Andrzej Siewior Feb. 17, 2025, 2:19 p.m. UTC | #1
On 2025-02-14 17:27:39 [+0100], Vlastimil Babka wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> In !PREEMPT_RT local_lock_irqsave() disables interrupts to protect
> critical section, but it doesn't prevent NMI, so the fully reentrant
> code cannot use local_lock_irqsave() for exclusive access.
> 
> Introduce localtry_lock_t and localtry_lock_irqsave() that
> disables interrupts and sets acquired=1, so localtry_lock_irqsave()
> from NMI attempting to acquire the same lock will return false.
> 
> In PREEMPT_RT local_lock_irqsave() maps to preemptible spin_lock().
> Map localtry_lock_irqsave() to preemptible spin_trylock().
> When in hard IRQ or NMI return false right away, since
> spin_trylock() is not safe due to PI issues.

spin_trylock() is not safe due to explicit locking in the underneath
rt_spin_trylock() implementation. Removing this explicit locking and
attempting only "trylock" is undesired due to PI implications.

> Note there is no need to use local_inc for acquired variable,
> since it's a percpu variable with strict nesting scopes.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Other than that, thank you two ;)

Sebastian
Vlastimil Babka Feb. 17, 2025, 2:35 p.m. UTC | #2
On 2/17/25 15:19, Sebastian Andrzej Siewior wrote:
> On 2025-02-14 17:27:39 [+0100], Vlastimil Babka wrote:
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> 
>> In !PREEMPT_RT local_lock_irqsave() disables interrupts to protect
>> critical section, but it doesn't prevent NMI, so the fully reentrant
>> code cannot use local_lock_irqsave() for exclusive access.
>> 
>> Introduce localtry_lock_t and localtry_lock_irqsave() that
>> disables interrupts and sets acquired=1, so localtry_lock_irqsave()
>> from NMI attempting to acquire the same lock will return false.
>> 
>> In PREEMPT_RT local_lock_irqsave() maps to preemptible spin_lock().
>> Map localtry_lock_irqsave() to preemptible spin_trylock().
>> When in hard IRQ or NMI return false right away, since
>> spin_trylock() is not safe due to PI issues.
> 
> spin_trylock() is not safe due to explicit locking in the underneath
> rt_spin_trylock() implementation. Removing this explicit locking and
> attempting only "trylock" is undesired due to PI implications.

Just to be sure, you're suggesting how to reword that sentence in the
changelog to make it more precise right?
Alexei will you incorporate that in your version?

>> Note there is no need to use local_inc for acquired variable,
>> since it's a percpu variable with strict nesting scopes.
>> 
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Other than that, thank you two ;)

Thank you too :)

Do you agree with my fixups and addition here?
https://lore.kernel.org/all/efc30cf9-8351-4889-8245-cc4a6893ebf4@suse.cz/

> Sebastian
Sebastian Andrzej Siewior Feb. 17, 2025, 3:07 p.m. UTC | #3
On 2025-02-17 15:35:11 [+0100], Vlastimil Babka wrote:
> > spin_trylock() is not safe due to explicit locking in the underneath
> > rt_spin_trylock() implementation. Removing this explicit locking and
> > attempting only "trylock" is undesired due to PI implications.
> 
> Just to be sure, you're suggesting how to reword that sentence in the
> changelog to make it more precise right?

Yes, just a reword. Everything else is fine by me. It just feels odd ack
my own patch.

> Alexei will you incorporate that in your version?
> 
> >> Note there is no need to use local_inc for acquired variable,
> >> since it's a percpu variable with strict nesting scopes.
> >> 
> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> > 
> > Other than that, thank you two ;)
> 
> Thank you too :)
> 
> Do you agree with my fixups and addition here?
> https://lore.kernel.org/all/efc30cf9-8351-4889-8245-cc4a6893ebf4@suse.cz/

Yes, looks good.

Sebastian
Alexei Starovoitov Feb. 18, 2025, 6:41 p.m. UTC | #4
On Mon, Feb 17, 2025 at 6:35 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 2/17/25 15:19, Sebastian Andrzej Siewior wrote:
> > On 2025-02-14 17:27:39 [+0100], Vlastimil Babka wrote:
> >> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >>
> >> In !PREEMPT_RT local_lock_irqsave() disables interrupts to protect
> >> critical section, but it doesn't prevent NMI, so the fully reentrant
> >> code cannot use local_lock_irqsave() for exclusive access.
> >>
> >> Introduce localtry_lock_t and localtry_lock_irqsave() that
> >> disables interrupts and sets acquired=1, so localtry_lock_irqsave()
> >> from NMI attempting to acquire the same lock will return false.
> >>
> >> In PREEMPT_RT local_lock_irqsave() maps to preemptible spin_lock().
> >> Map localtry_lock_irqsave() to preemptible spin_trylock().
> >> When in hard IRQ or NMI return false right away, since
> >> spin_trylock() is not safe due to PI issues.
> >
> > spin_trylock() is not safe due to explicit locking in the underneath
> > rt_spin_trylock() implementation. Removing this explicit locking and
> > attempting only "trylock" is undesired due to PI implications.

Makes sense.

> Just to be sure, you're suggesting how to reword that sentence in the
> changelog to make it more precise right?
> Alexei will you incorporate that in your version?

Sure. Let's squash patches 3 and 4 and add above
commit log clarification.
Whoever respins first can do it.
diff mbox series

Patch

diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
index 091dc0b6bdfb9f4721f94d19828a38fbfa59346c..05c254a5d7d3e6db64d7f81a3a4a10f5a942c29e 100644
--- a/include/linux/local_lock.h
+++ b/include/linux/local_lock.h
@@ -51,6 +51,65 @@ 
 #define local_unlock_irqrestore(lock, flags)			\
 	__local_unlock_irqrestore(lock, flags)
 
+/**
+ * localtry_lock_init - Runtime initialize a lock instance
+ */
+#define localtry_lock_init(lock)		__localtry_lock_init(lock)
+
+/**
+ * localtry_lock - Acquire a per CPU local lock
+ * @lock:	The lock variable
+ */
+#define localtry_lock(lock)		__localtry_lock(lock)
+
+/**
+ * localtry_lock_irq - Acquire a per CPU local lock and disable interrupts
+ * @lock:	The lock variable
+ */
+#define localtry_lock_irq(lock)		__localtry_lock_irq(lock)
+
+/**
+ * localtry_lock_irqsave - Acquire a per CPU local lock, save and disable
+ *			 interrupts
+ * @lock:	The lock variable
+ * @flags:	Storage for interrupt flags
+ */
+#define localtry_lock_irqsave(lock, flags)				\
+	__localtry_lock_irqsave(lock, flags)
+
+/**
+ * localtry_trylock_irqsave - Try to acquire a per CPU local lock, save and disable
+ *			      interrupts if acquired
+ * @lock:	The lock variable
+ * @flags:	Storage for interrupt flags
+ *
+ * The function can be used in any context such as NMI or HARDIRQ. Due to
+ * locking constrains it will _always_ fail to acquire the lock on PREEMPT_RT.
+ */
+#define localtry_trylock_irqsave(lock, flags)				\
+	__localtry_trylock_irqsave(lock, flags)
+
+/**
+ * local_unlock - Release a per CPU local lock
+ * @lock:	The lock variable
+ */
+#define localtry_unlock(lock)		__localtry_unlock(lock)
+
+/**
+ * local_unlock_irq - Release a per CPU local lock and enable interrupts
+ * @lock:	The lock variable
+ */
+#define localtry_unlock_irq(lock)		__localtry_unlock_irq(lock)
+
+/**
+ * localtry_unlock_irqrestore - Release a per CPU local lock and restore
+ *			      interrupt flags
+ * @lock:	The lock variable
+ * @flags:      Interrupt flags to restore
+ */
+#define localtry_unlock_irqrestore(lock, flags)			\
+	__localtry_unlock_irqrestore(lock, flags)
+
 DEFINE_GUARD(local_lock, local_lock_t __percpu*,
 	     local_lock(_T),
 	     local_unlock(_T))
diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index 8dd71fbbb6d2b6748969438c4642f7d970834871..c1369b300777d3ff3700cfd8bd4de8186124f036 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -15,6 +15,11 @@  typedef struct {
 #endif
 } local_lock_t;
 
+typedef struct {
+	local_lock_t	llock;
+	unsigned int	acquired;
+} localtry_lock_t;
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # define LOCAL_LOCK_DEBUG_INIT(lockname)		\
 	.dep_map = {					\
@@ -31,6 +36,13 @@  static inline void local_lock_acquire(local_lock_t *l)
 	l->owner = current;
 }
 
+static inline void local_trylock_acquire(local_lock_t *l)
+{
+	lock_map_acquire_try(&l->dep_map);
+	DEBUG_LOCKS_WARN_ON(l->owner);
+	l->owner = current;
+}
+
 static inline void local_lock_release(local_lock_t *l)
 {
 	DEBUG_LOCKS_WARN_ON(l->owner != current);
@@ -45,11 +57,13 @@  static inline void local_lock_debug_init(local_lock_t *l)
 #else /* CONFIG_DEBUG_LOCK_ALLOC */
 # define LOCAL_LOCK_DEBUG_INIT(lockname)
 static inline void local_lock_acquire(local_lock_t *l) { }
+static inline void local_trylock_acquire(local_lock_t *l) { }
 static inline void local_lock_release(local_lock_t *l) { }
 static inline void local_lock_debug_init(local_lock_t *l) { }
 #endif /* !CONFIG_DEBUG_LOCK_ALLOC */
 
 #define INIT_LOCAL_LOCK(lockname)	{ LOCAL_LOCK_DEBUG_INIT(lockname) }
+#define INIT_LOCALTRY_LOCK(lockname)	{ .llock = { LOCAL_LOCK_DEBUG_INIT(lockname.llock) }}
 
 #define __local_lock_init(lock)					\
 do {								\
@@ -118,6 +132,86 @@  do {								\
 #define __local_unlock_nested_bh(lock)				\
 	local_lock_release(this_cpu_ptr(lock))
 
+/* localtry_lock_t variants */
+
+#define __localtry_lock_init(lock)				\
+do {								\
+	__local_lock_init(&(lock)->llock);			\
+	WRITE_ONCE(&(lock)->acquired, 0);			\
+} while (0)
+
+#define __localtry_lock(lock)					\
+	do {							\
+		localtry_lock_t *lt;				\
+		preempt_disable();				\
+		lt = this_cpu_ptr(lock);			\
+		local_lock_acquire(&lt->llock);			\
+		WRITE_ONCE(lt->acquired, 1);			\
+	} while (0)
+
+#define __localtry_lock_irq(lock)				\
+	do {							\
+		localtry_lock_t *lt;				\
+		local_irq_disable();				\
+		lt = this_cpu_ptr(lock);			\
+		local_lock_acquire(&lt->llock);			\
+		WRITE_ONCE(lt->acquired, 1);			\
+	} while (0)
+
+#define __localtry_lock_irqsave(lock, flags)			\
+	do {							\
+		localtry_lock_t *lt;				\
+		local_irq_save(flags);				\
+		lt = this_cpu_ptr(lock);			\
+		local_lock_acquire(&lt->llock);			\
+		WRITE_ONCE(lt->acquired, 1);			\
+	} while (0)
+
+#define __localtry_trylock_irqsave(lock, flags)			\
+	({							\
+		localtry_lock_t *lt;				\
+		bool _ret;					\
+								\
+		local_irq_save(flags);				\
+		lt = this_cpu_ptr(lock);			\
+		if (!READ_ONCE(lt->acquired)) {			\
+			WRITE_ONCE(lt->acquired, 1);		\
+			local_trylock_acquire(&lt->llock);	\
+			_ret = true;				\
+		} else {					\
+			_ret = false;				\
+			local_irq_restore(flags);		\
+		}						\
+		_ret;						\
+	})
+
+#define __localtry_unlock(lock)					\
+	do {							\
+		localtry_lock_t *lt;				\
+		lt = this_cpu_ptr(lock);			\
+		WRITE_ONCE(lt->acquired, 0);			\
+		local_lock_release(&lt->llock);			\
+		preempt_enable();				\
+	} while (0)
+
+#define __localtry_unlock_irq(lock)				\
+	do {							\
+		localtry_lock_t *lt;				\
+		lt = this_cpu_ptr(lock);			\
+		WRITE_ONCE(lt->acquired, 0);			\
+		local_lock_release(&lt->llock);			\
+		local_irq_enable();				\
+	} while (0)
+
+#define __localtry_unlock_irqrestore(lock, flags)		\
+	do {							\
+		localtry_lock_t *lt;				\
+		lt = this_cpu_ptr(lock);			\
+		WRITE_ONCE(lt->acquired, 0);			\
+		local_lock_release(&lt->llock);			\
+		local_irq_restore(flags);			\
+	} while (0)
+
 #else /* !CONFIG_PREEMPT_RT */
 
 /*
@@ -125,8 +219,10 @@  do {								\
  * critical section while staying preemptible.
  */
 typedef spinlock_t local_lock_t;
+typedef spinlock_t localtry_lock_t;
 
 #define INIT_LOCAL_LOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
+#define INIT_LOCALTRY_LOCK(lockname) INIT_LOCAL_LOCK(lockname)
 
 #define __local_lock_init(l)					\
 	do {							\
@@ -169,4 +265,31 @@  do {								\
 	spin_unlock(this_cpu_ptr((lock)));			\
 } while (0)
 
+/* localtry_lock_t variants */
+
+#define __localtry_lock_init(lock)			__local_lock_init(lock)
+#define __localtry_lock(lock)				__local_lock(lock)
+#define __localtry_lock_irq(lock)			__local_lock(lock)
+#define __localtry_lock_irqsave(lock, flags)		__local_lock_irqsave(lock, flags)
+#define __localtry_unlock(lock)				__local_unlock(lock)
+#define __localtry_unlock_irq(lock)			__local_unlock(lock)
+#define __localtry_unlock_irqrestore(lock, flags)	__local_unlock_irqrestore(lock, flags)
+
+#define __localtry_trylock_irqsave(lock, flags)			\
+	({							\
+		int __locked;					\
+								\
+		typecheck(unsigned long, flags);		\
+		flags = 0;					\
+		if (in_nmi() | in_hardirq()) {			\
+			__locked = 0;				\
+		} else {					\
+			migrate_disable();			\
+			__locked = spin_trylock(this_cpu_ptr((lock)));	\
+			if (!__locked)				\
+				migrate_enable();		\
+		}						\
+		__locked;					\
+	})
+
 #endif /* CONFIG_PREEMPT_RT */