[bpf-next,v6,3/6] locking/local_lock: Introduce local_trylock_t and local_trylock_irqsave()

Commit Message

Alexei Starovoitov Jan. 24, 2025, 3:56 a.m. UTC

From: Alexei Starovoitov <[e-mail]>

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 local_trylock_t and local_trylock_irqsave() that
disables interrupts and sets active=1, so local_trylock_irqsave()
from NMI of the same lock will return false.

In PREEMPT_RT local_lock_irqsave() maps to preemptible spin_lock().
Map local_trylock_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 active variable,
since it's a percpu variable with strict nesting scopes.

Usage:

local_lock_t lock;                     // sizeof(lock) == 0 in !RT
local_lock_irqsave(&lock, ...);        // irqsave as before
if (local_trylock_irqsave(&lock, ...)) // compilation error

local_trylock_t lock;                  // sizeof(lock) == 4 in !RT
local_lock_irqsave(&lock, ...);        // irqsave and active = 1
if (local_trylock_irqsave(&lock, ...)) // if (!active) irqsave

Signed-off-by: Alexei Starovoitov <[e-mail]>
---
 include/linux/local_lock.h          |  9 ++++
 include/linux/local_lock_internal.h | 79 ++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 2 deletions(-)

Comments

Sebastian Andrzej Siewior Jan. 28, 2025, 5:21 p.m. UTC | #1

On 2025-01-23 19:56:52 [-0800], Alexei Starovoitov wrote:
> Usage:
> 
> local_lock_t lock;                     // sizeof(lock) == 0 in !RT
> local_lock_irqsave(&lock, ...);        // irqsave as before
> if (local_trylock_irqsave(&lock, ...)) // compilation error
> 
> local_trylock_t lock;                  // sizeof(lock) == 4 in !RT
> local_lock_irqsave(&lock, ...);        // irqsave and active = 1
> if (local_trylock_irqsave(&lock, ...)) // if (!active) irqsave

so I've been looking at this for a while and I don't like the part where
the type is hidden away. It is then casted back. So I tried something
with _Generics but then the existing guard implementation complained.
Then I asked myself why do we want to hide much of the implementation
and not make it obvious. So I made a few macros to hide most of the
implementation for !RT. Later I figured if the variable is saved locally
then I save one this_cpu_ptr invocation. So I wrote it out and the
snippet below is what I have.

is this anywhere near possible to accept?

diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
index 091dc0b6bdfb9..05c254a5d7d3e 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 8dd71fbbb6d2b..789b0d878e6c5 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 = {					\
@@ -50,6 +55,7 @@ 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 +124,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)) {			\
+			local_lock_acquire(&lt->llock);		\
+			WRITE_ONCE(lt->acquired, 1);		\
+			_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 +211,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 +257,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 */

Alexei Starovoitov Jan. 28, 2025, 6:50 p.m. UTC | #2

On Tue, Jan 28, 2025 at 9:21 AM Sebastian Andrzej Siewior
<[e-mail]> wrote:
>
> On 2025-01-23 19:56:52 [-0800], Alexei Starovoitov wrote:
> > Usage:
> >
> > local_lock_t lock;                     // sizeof(lock) == 0 in !RT
> > local_lock_irqsave(&lock, ...);        // irqsave as before
> > if (local_trylock_irqsave(&lock, ...)) // compilation error
> >
> > local_trylock_t lock;                  // sizeof(lock) == 4 in !RT
> > local_lock_irqsave(&lock, ...);        // irqsave and active = 1
> > if (local_trylock_irqsave(&lock, ...)) // if (!active) irqsave
>
> so I've been looking at this for a while and I don't like the part where
> the type is hidden away. It is then casted back. So I tried something
> with _Generics but then the existing guard implementation complained.
> Then I asked myself why do we want to hide much of the implementation
> and not make it obvious.

Well, the idea of hiding extra field with _Generic is to avoid
the churn:

git grep -E 'local_.*lock_irq'|wc -l
42

I think the api is clean enough and _Generic part is not exposed
to users.
Misuse or accidental usage is not possible either.
See the point:
if (local_trylock_irqsave(&lock, ...)) // compilation error

So imo it's a better tradeoff.

> is this anywhere near possible to accept?

Other than churn it's fine.
I can go with it if you insist,
but casting and _Generic() I think is cleaner.
Certainly a bit unusual pattern.
Could you sleep on it?

I can do s/local_trylock_t/localtry_lock_t/.
That part is trivial.

Sebastian Andrzej Siewior Jan. 29, 2025, 8:17 a.m. UTC | #3

PeterZ, may I summon you.

On 2025-01-28 10:50:33 [-0800], Alexei Starovoitov wrote:
> On Tue, Jan 28, 2025 at 9:21 AM Sebastian Andrzej Siewior
> <[e-mail]> wrote:
> >
> > On 2025-01-23 19:56:52 [-0800], Alexei Starovoitov wrote:
> > > Usage:
> > >
> > > local_lock_t lock;                     // sizeof(lock) == 0 in !RT
> > > local_lock_irqsave(&lock, ...);        // irqsave as before
> > > if (local_trylock_irqsave(&lock, ...)) // compilation error
> > >
> > > local_trylock_t lock;                  // sizeof(lock) == 4 in !RT
> > > local_lock_irqsave(&lock, ...);        // irqsave and active = 1
> > > if (local_trylock_irqsave(&lock, ...)) // if (!active) irqsave
> >
> > so I've been looking at this for a while and I don't like the part where
> > the type is hidden away. It is then casted back. So I tried something
> > with _Generics but then the existing guard implementation complained.
> > Then I asked myself why do we want to hide much of the implementation
> > and not make it obvious.
> 
> Well, the idea of hiding extra field with _Generic is to avoid
> the churn:
> 
> git grep -E 'local_.*lock_irq'|wc -l
> 42

This could be also hidden with a macro defining the general body and
having a place holder for "lock primitive".

> I think the api is clean enough and _Generic part is not exposed
> to users.
> Misuse or accidental usage is not possible either.
> See the point:
> if (local_trylock_irqsave(&lock, ...)) // compilation error
> 
> So imo it's a better tradeoff.
> 
> > is this anywhere near possible to accept?
> 
> Other than churn it's fine.
> I can go with it if you insist,
> but casting and _Generic() I think is cleaner.
> Certainly a bit unusual pattern.
> Could you sleep on it?

The cast there is somehow… We could have BUILD_BUG_ON() to ensure a
stable the layout of the structs… However all this is not my call.

PeterZ, do you have any preferences or an outline what you would like to
see here?

> I can do s/local_trylock_t/localtry_lock_t/.
> That part is trivial.

Sebastian

Alexei Starovoitov Jan. 30, 2025, 8:51 p.m. UTC | #4

On Wed, Jan 29, 2025 at 12:17 AM Sebastian Andrzej Siewior
<[e-mail]> wrote:
>
> PeterZ, may I summon you.
>
> On 2025-01-28 10:50:33 [-0800], Alexei Starovoitov wrote:
> > On Tue, Jan 28, 2025 at 9:21 AM Sebastian Andrzej Siewior
> > <[e-mail]> wrote:
> > >
> > > On 2025-01-23 19:56:52 [-0800], Alexei Starovoitov wrote:
> > > > Usage:
> > > >
> > > > local_lock_t lock;                     // sizeof(lock) == 0 in !RT
> > > > local_lock_irqsave(&lock, ...);        // irqsave as before
> > > > if (local_trylock_irqsave(&lock, ...)) // compilation error
> > > >
> > > > local_trylock_t lock;                  // sizeof(lock) == 4 in !RT
> > > > local_lock_irqsave(&lock, ...);        // irqsave and active = 1
> > > > if (local_trylock_irqsave(&lock, ...)) // if (!active) irqsave
> > >
> > > so I've been looking at this for a while and I don't like the part where
> > > the type is hidden away. It is then casted back. So I tried something
> > > with _Generics but then the existing guard implementation complained.
> > > Then I asked myself why do we want to hide much of the implementation
> > > and not make it obvious.
> >
> > Well, the idea of hiding extra field with _Generic is to avoid
> > the churn:
> >
> > git grep -E 'local_.*lock_irq'|wc -l
> > 42
>
> This could be also hidden with a macro defining the general body and
> having a place holder for "lock primitive".

How would that look like?

> > I think the api is clean enough and _Generic part is not exposed
> > to users.
> > Misuse or accidental usage is not possible either.
> > See the point:
> > if (local_trylock_irqsave(&lock, ...)) // compilation error
> >
> > So imo it's a better tradeoff.
> >
> > > is this anywhere near possible to accept?
> >
> > Other than churn it's fine.
> > I can go with it if you insist,
> > but casting and _Generic() I think is cleaner.
> > Certainly a bit unusual pattern.
> > Could you sleep on it?
>
> The cast there is somehow… We could have BUILD_BUG_ON() to ensure a
> stable the layout of the structs… However all this is not my call.
>
> PeterZ, do you have any preferences or an outline what you would like to
> see here?

I still don't get the objection.
This is a normal function polymorphism that is present
in many languages.
Consider spin_lock().
It's already vastly different in PREEMPT_RT vs not.
This is polymorphism. The same function has different
implementations depending on config and argument type.
This patch makes local_lock_irqsave() polymorphic
not only in PREEMPT_RT vs not,
but also depending on local_lock_t vs localtry_lock_t
argument type in !PREEMPT_RT.

Anyway, if I don't hear back from Peter or you soon
I'll just take your localtry_lock_t version of the patch
with you being an author and your SOB (ok ?) and
will make the next patch 4 to suffer all the code churn.
We cannot afford to get stuck on something as trivial
as this for days.

Sebastian Andrzej Siewior Feb. 6, 2025, 11:13 a.m. UTC | #5

On 2025-01-29 09:17:27 [+0100], To Alexei Starovoitov wrote:
> PeterZ, may I summon you.
PeterZ, may I summon you.

Sebastian

Patch

diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
index 091dc0b6bdfb..f4bc3e9b2b20 100644
--- a/include/linux/local_lock.h
+++ b/include/linux/local_lock.h
@@ -30,6 +30,15 @@ 
 #define local_lock_irqsave(lock, flags)				\
 	__local_lock_irqsave(lock, flags)
 
+/**
+ * local_trylock_irqsave - Try to acquire a per CPU local lock, save and disable
+ *			   interrupts. Fails in PREEMPT_RT when in hard IRQ or NMI.
+ * @lock:	The lock variable
+ * @flags:	Storage for interrupt flags
+ */
+#define local_trylock_irqsave(lock, flags)			\
+	__local_trylock_irqsave(lock, flags)
+
 /**
  * local_unlock - Release a per CPU local lock
  * @lock:	The lock variable
diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index 8dd71fbbb6d2..14757b7aea99 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -15,6 +15,19 @@  typedef struct {
 #endif
 } local_lock_t;
 
+typedef struct {
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct lockdep_map	dep_map;
+	struct task_struct	*owner;
+#endif
+	/*
+	 * Same layout as local_lock_t with 'active' field
+	 * at the end, since (local_trylock_t *) will be
+	 * casted to (local_lock_t *).
+	 */
+	int active;
+} local_trylock_t;
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # define LOCAL_LOCK_DEBUG_INIT(lockname)		\
 	.dep_map = {					\
@@ -31,6 +44,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,6 +65,7 @@  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 */
@@ -87,10 +108,37 @@  do {								\
 
 #define __local_lock_irqsave(lock, flags)			\
 	do {							\
+		local_trylock_t *tl;				\
+		local_lock_t *l;				\
 		local_irq_save(flags);				\
-		local_lock_acquire(this_cpu_ptr(lock));		\
+		l = (local_lock_t *)this_cpu_ptr(lock);		\
+		tl = (local_trylock_t *)l;			\
+		_Generic((lock),				\
+			local_trylock_t *: ({			\
+				lockdep_assert(tl->active == 0);\
+				WRITE_ONCE(tl->active, 1);	\
+			}),					\
+			default:(void)0);			\
+		local_lock_acquire(l);				\
 	} while (0)
 
+
+#define __local_trylock_irqsave(lock, flags)			\
+	({							\
+		local_trylock_t *tl;				\
+		local_irq_save(flags);				\
+		tl = this_cpu_ptr(lock);			\
+		if (READ_ONCE(tl->active) == 1) {		\
+			local_irq_restore(flags);		\
+			tl = NULL;				\
+		} else {					\
+			WRITE_ONCE(tl->active, 1);		\
+			local_trylock_acquire(			\
+				(local_lock_t *)tl);		\
+		}						\
+		!!tl;						\
+	})
+
 #define __local_unlock(lock)					\
 	do {							\
 		local_lock_release(this_cpu_ptr(lock));		\
@@ -105,7 +153,17 @@  do {								\
 
 #define __local_unlock_irqrestore(lock, flags)			\
 	do {							\
-		local_lock_release(this_cpu_ptr(lock));		\
+		local_trylock_t *tl;				\
+		local_lock_t *l;				\
+		l = (local_lock_t *)this_cpu_ptr(lock);		\
+		tl = (local_trylock_t *)l;			\
+		_Generic((lock),				\
+			local_trylock_t *: ({			\
+				lockdep_assert(tl->active == 1);\
+				WRITE_ONCE(tl->active, 0);	\
+			}),					\
+			default:(void)0);			\
+		local_lock_release(l);				\
 		local_irq_restore(flags);			\
 	} while (0)
 
@@ -125,6 +183,7 @@  do {								\
  * critical section while staying preemptible.
  */
 typedef spinlock_t local_lock_t;
+typedef spinlock_t local_trylock_t;
 
 #define INIT_LOCAL_LOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
 
@@ -148,6 +207,22 @@  typedef spinlock_t local_lock_t;
 		__local_lock(lock);				\
 	} while (0)
 
+#define __local_trylock_irqsave(lock, flags)			\
+	({							\
+		__label__ out;					\
+		int ret = 0;					\
+		typecheck(unsigned long, flags);		\
+		flags = 0;					\
+		if (in_nmi() || in_hardirq())			\
+			goto out;				\
+		migrate_disable();				\
+		ret = spin_trylock(this_cpu_ptr((lock)));	\
+		if (!ret)					\
+			migrate_enable();			\
+	out:							\
+		ret;						\
+	})
+
 #define __local_unlock(__lock)					\
 	do {							\
 		spin_unlock(this_cpu_ptr((__lock)));		\