On 1/15/25 03:17, Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@xxxxxxxxxx> > > Similar to local_lock_irqsave() introduce local_trylock_irqsave(). > This is inspired by 'struct local_tryirq_lock' in: > https://lore.kernel.org/all/20241112-slub-percpu-caches-v1-5-ddc0bdc27e05@xxxxxxx/ Let's see what locking maintainers say about adding the flag to every local_lock even if it doesn't use the trylock operation. > Use spin_trylock in PREEMPT_RT when not in hard IRQ and not in NMI > and fail instantly otherwise, since spin_trylock is not safe from IRQ > due to PI issues. > > In !PREEMPT_RT use simple active flag to prevent IRQs or NMIs > reentering locked region. > > Note there is no need to use local_inc for active flag. > If IRQ handler grabs the same local_lock after READ_ONCE(lock->active) IRQ handler AFAICS can't do that since __local_trylock_irqsave() is the only trylock operation and it still does local_irq_save(). Could you have added a __local_trylock() operation instead? Guess not for your use case because I see in Patch 4 you want to use the local_unlock_irqrestore() universally for sections that are earlier locked either by local_trylock_irqsave() or local_lock_irqsave(). But I wonder if those can be changed (will reply on that patch). The motivation in my case was to avoid the overhead of irqsave/restore on !PREEMPT_RT. If there was a separate "flavor" of local_lock that would support the trylock operation, I think it would not need the _irq and _irqsave variants at all, and it would also avoid adding the "active" flag on !PREEMPT_RT. Meanwhile on PREEMPT_RT, a single implementation could likely handle both flavors with no downsides? > already completed it has to unlock it before returning. > Similar with NMI handler. So there is a strict nesting of scopes. > It's a per cpu lock. Multiple cpus do not access it in parallel. > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx> > --- > include/linux/local_lock.h | 9 ++++ > include/linux/local_lock_internal.h | 76 ++++++++++++++++++++++++++--- > 2 files changed, 78 insertions(+), 7 deletions(-) > > diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h > index 091dc0b6bdfb..84ee560c4f51 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. Always fails in RT when in_hardirq 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..93672127c73d 100644 > --- a/include/linux/local_lock_internal.h > +++ b/include/linux/local_lock_internal.h > @@ -9,6 +9,7 @@ > #ifndef CONFIG_PREEMPT_RT > > typedef struct { > + int active; > #ifdef CONFIG_DEBUG_LOCK_ALLOC > struct lockdep_map dep_map; > struct task_struct *owner; > @@ -22,7 +23,7 @@ typedef struct { > .wait_type_inner = LD_WAIT_CONFIG, \ > .lock_type = LD_LOCK_PERCPU, \ > }, \ > - .owner = NULL, > + .owner = NULL, .active = 0 > > static inline void local_lock_acquire(local_lock_t *l) > { > @@ -31,6 +32,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 +53,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 */ > @@ -60,6 +69,7 @@ do { \ > 0, LD_WAIT_CONFIG, LD_WAIT_INV, \ > LD_LOCK_PERCPU); \ > local_lock_debug_init(lock); \ > + (lock)->active = 0; \ > } while (0) > > #define __spinlock_nested_bh_init(lock) \ > @@ -75,37 +85,73 @@ do { \ > > #define __local_lock(lock) \ > do { \ > + local_lock_t *l; \ > preempt_disable(); \ > - local_lock_acquire(this_cpu_ptr(lock)); \ > + l = this_cpu_ptr(lock); \ > + lockdep_assert(l->active == 0); \ > + WRITE_ONCE(l->active, 1); \ > + local_lock_acquire(l); \ > } while (0) > > #define __local_lock_irq(lock) \ > do { \ > + local_lock_t *l; \ > local_irq_disable(); \ > - local_lock_acquire(this_cpu_ptr(lock)); \ > + l = this_cpu_ptr(lock); \ > + lockdep_assert(l->active == 0); \ > + WRITE_ONCE(l->active, 1); \ > + local_lock_acquire(l); \ > } while (0) > > #define __local_lock_irqsave(lock, flags) \ > do { \ > + local_lock_t *l; \ > local_irq_save(flags); \ > - local_lock_acquire(this_cpu_ptr(lock)); \ > + l = this_cpu_ptr(lock); \ > + lockdep_assert(l->active == 0); \ > + WRITE_ONCE(l->active, 1); \ > + local_lock_acquire(l); \ > } while (0) > > +#define __local_trylock_irqsave(lock, flags) \ > + ({ \ > + local_lock_t *l; \ > + local_irq_save(flags); \ > + l = this_cpu_ptr(lock); \ > + if (READ_ONCE(l->active) == 1) { \ > + local_irq_restore(flags); \ > + l = NULL; \ > + } else { \ > + WRITE_ONCE(l->active, 1); \ > + local_trylock_acquire(l); \ > + } \ > + !!l; \ > + }) > + > #define __local_unlock(lock) \ > do { \ > - local_lock_release(this_cpu_ptr(lock)); \ > + local_lock_t *l = this_cpu_ptr(lock); \ > + lockdep_assert(l->active == 1); \ > + WRITE_ONCE(l->active, 0); \ > + local_lock_release(l); \ > preempt_enable(); \ > } while (0) > > #define __local_unlock_irq(lock) \ > do { \ > - local_lock_release(this_cpu_ptr(lock)); \ > + local_lock_t *l = this_cpu_ptr(lock); \ > + lockdep_assert(l->active == 1); \ > + WRITE_ONCE(l->active, 0); \ > + local_lock_release(l); \ > local_irq_enable(); \ > } while (0) > > #define __local_unlock_irqrestore(lock, flags) \ > do { \ > - local_lock_release(this_cpu_ptr(lock)); \ > + local_lock_t *l = this_cpu_ptr(lock); \ > + lockdep_assert(l->active == 1); \ > + WRITE_ONCE(l->active, 0); \ > + local_lock_release(l); \ > local_irq_restore(flags); \ > } while (0) > > @@ -148,6 +194,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))); \
