| Message ID | 20250206181711.1902989-3-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Compiler-Based Capability- and Locking-Analysis | expand |
On Thu, Feb 06, 2025 at 07:09:56PM +0100, Marco Elver wrote: > Just like the pairing of attribute __acquires() with a matching > function-like macro __acquire(), the attribute __cond_acquires() should > have a matching function-like macro __cond_acquire(). > > To be consistent, rename __cond_lock() to __cond_acquire(). So I hate this __cond_lock() thing we have with a passion. I think it is one of the very worst annotations possible since it makes a trainwreck of the trylock code. It is a major reason why mutex is not annotated with this nonsense. Also, I think very dim of sparse in general -- I don't think I've ever managed to get a useful warning from between all the noise it generates.
On Fri, Feb 07, 2025 at 09:28AM +0100, Peter Zijlstra wrote: > On Thu, Feb 06, 2025 at 07:09:56PM +0100, Marco Elver wrote: > > Just like the pairing of attribute __acquires() with a matching > > function-like macro __acquire(), the attribute __cond_acquires() should > > have a matching function-like macro __cond_acquire(). > > > > To be consistent, rename __cond_lock() to __cond_acquire(). > > So I hate this __cond_lock() thing we have with a passion. I think it is > one of the very worst annotations possible since it makes a trainwreck > of the trylock code. > > It is a major reason why mutex is not annotated with this nonsense. > > Also, I think very dim of sparse in general -- I don't think I've ever > managed to get a useful warning from between all the noise it generates. Happy to reduce the use of __cond_lock(). :-) Though one problem I found is it's still needed for those complex statement-expression *_trylock that spinlock.h/rwlock.h has, where we e.g. have (with my changes): #define raw_spin_trylock_irqsave(lock, flags) \ __cond_acquire(lock, ({ \ local_irq_save(flags); \ _raw_spin_trylock(lock) ? \ 1 : ({ local_irq_restore(flags); 0; }); \ })) Because there's an inner condition using _raw_spin_trylock() and the result of _raw_spin_trylock() is no longer directly used in a branch that also does the unlock, Clang becomes unhappy and complains. I.e. annotating _raw_spin_trylock with __cond_acquires(1, lock) doesn't work for this case because it's in a complex statement-expression. The only way to make it work was to wrap it into a function that has attribute __cond_acquires(1, lock) which is what I made __cond_lock/acquire do. For some of the trivial uses, like e.g. #define raw_spin_trylock(lock) __cond_acquire(lock, _raw_spin_trylock(lock)) it's easy enough to remove the outer __cond_lock/acquire if e.g. the _raw_spin_trylock has the attribute __cond_acquires. I kept these around for Sparse compatibility, but if we want to get rid of Sparse compatibility, some of those can be simplified.
On Fri, Feb 07, 2025 at 10:32:25AM +0100, Marco Elver wrote: > On Fri, Feb 07, 2025 at 09:28AM +0100, Peter Zijlstra wrote: > > On Thu, Feb 06, 2025 at 07:09:56PM +0100, Marco Elver wrote: > > > Just like the pairing of attribute __acquires() with a matching > > > function-like macro __acquire(), the attribute __cond_acquires() should > > > have a matching function-like macro __cond_acquire(). > > > > > > To be consistent, rename __cond_lock() to __cond_acquire(). > > > > So I hate this __cond_lock() thing we have with a passion. I think it is > > one of the very worst annotations possible since it makes a trainwreck > > of the trylock code. > > > > It is a major reason why mutex is not annotated with this nonsense. > > > > Also, I think very dim of sparse in general -- I don't think I've ever > > managed to get a useful warning from between all the noise it generates. > > Happy to reduce the use of __cond_lock(). :-) > Though one problem I found is it's still needed for those complex > statement-expression *_trylock that spinlock.h/rwlock.h has, where we > e.g. have (with my changes): > > #define raw_spin_trylock_irqsave(lock, flags) \ > __cond_acquire(lock, ({ \ > local_irq_save(flags); \ > _raw_spin_trylock(lock) ? \ > 1 : ({ local_irq_restore(flags); 0; }); \ > })) > > Because there's an inner condition using _raw_spin_trylock() and the > result of _raw_spin_trylock() is no longer directly used in a branch > that also does the unlock, Clang becomes unhappy and complains. I.e. > annotating _raw_spin_trylock with __cond_acquires(1, lock) doesn't work > for this case because it's in a complex statement-expression. The only > way to make it work was to wrap it into a function that has attribute > __cond_acquires(1, lock) which is what I made __cond_lock/acquire do. Does something like: static inline bool _raw_spin_trylock_irqsave(raw_spinlock_t *lock, unsigned long *flags) __cond_acquire(1, lock) { local_irq_save(*flags); if (_raw_spin_trylock(lock)) return true; local_irq_restore(*flags); return false; } #define raw_spin_trylock_irqsave(lock, flags) \ _raw_spin_trylock_irqsave((lock), &(flags)) work?
On Fri, 7 Feb 2025 at 10:41, Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Feb 07, 2025 at 10:32:25AM +0100, Marco Elver wrote: > > On Fri, Feb 07, 2025 at 09:28AM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 06, 2025 at 07:09:56PM +0100, Marco Elver wrote: > > > > Just like the pairing of attribute __acquires() with a matching > > > > function-like macro __acquire(), the attribute __cond_acquires() should > > > > have a matching function-like macro __cond_acquire(). > > > > > > > > To be consistent, rename __cond_lock() to __cond_acquire(). > > > > > > So I hate this __cond_lock() thing we have with a passion. I think it is > > > one of the very worst annotations possible since it makes a trainwreck > > > of the trylock code. > > > > > > It is a major reason why mutex is not annotated with this nonsense. > > > > > > Also, I think very dim of sparse in general -- I don't think I've ever > > > managed to get a useful warning from between all the noise it generates. > > > > Happy to reduce the use of __cond_lock(). :-) > > Though one problem I found is it's still needed for those complex > > statement-expression *_trylock that spinlock.h/rwlock.h has, where we > > e.g. have (with my changes): > > > > #define raw_spin_trylock_irqsave(lock, flags) \ > > __cond_acquire(lock, ({ \ > > local_irq_save(flags); \ > > _raw_spin_trylock(lock) ? \ > > 1 : ({ local_irq_restore(flags); 0; }); \ > > })) > > > > Because there's an inner condition using _raw_spin_trylock() and the > > result of _raw_spin_trylock() is no longer directly used in a branch > > that also does the unlock, Clang becomes unhappy and complains. I.e. > > annotating _raw_spin_trylock with __cond_acquires(1, lock) doesn't work > > for this case because it's in a complex statement-expression. The only > > way to make it work was to wrap it into a function that has attribute > > __cond_acquires(1, lock) which is what I made __cond_lock/acquire do. > > Does something like: > > static inline bool > _raw_spin_trylock_irqsave(raw_spinlock_t *lock, unsigned long *flags) > __cond_acquire(1, lock) > { > local_irq_save(*flags); > if (_raw_spin_trylock(lock)) > return true; > local_irq_restore(*flags); > return false; > } > > #define raw_spin_trylock_irqsave(lock, flags) \ > _raw_spin_trylock_irqsave((lock), &(flags)) > > work? Yup it does (tested). Ok, so getting rid of __cond_lock should be doable. :-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-trans.h b/drivers/net/wireless/intel/iwlwifi/iwl-trans.h index f6234065dbdd..560a5a899d1f 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-trans.h +++ b/drivers/net/wireless/intel/iwlwifi/iwl-trans.h @@ -1136,7 +1136,7 @@ void iwl_trans_set_bits_mask(struct iwl_trans *trans, u32 reg, bool _iwl_trans_grab_nic_access(struct iwl_trans *trans); #define iwl_trans_grab_nic_access(trans) \ - __cond_lock(nic_access, \ + __cond_acquire(nic_access, \ likely(_iwl_trans_grab_nic_access(trans))) void __releases(nic_access) diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h index 856b7e9f717d..a1becf833dc5 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h +++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h @@ -560,7 +560,7 @@ void iwl_trans_pcie_free_pnvm_dram_regions(struct iwl_dram_regions *dram_regions bool __iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans); #define _iwl_trans_pcie_grab_nic_access(trans) \ - __cond_lock(nic_access_nobh, \ + __cond_acquire(nic_access_nobh, \ likely(__iwl_trans_pcie_grab_nic_access(trans))) void iwl_trans_pcie_check_product_reset_status(struct pci_dev *pdev); diff --git a/include/linux/compiler-capability-analysis.h b/include/linux/compiler-capability-analysis.h index 7546ddb83f86..dfed4e7e6ab8 100644 --- a/include/linux/compiler-capability-analysis.h +++ b/include/linux/compiler-capability-analysis.h @@ -15,7 +15,7 @@ # define __releases(x) __attribute__((context(x,1,0))) # define __acquire(x) __context__(x,1) # define __release(x) __context__(x,-1) -# define __cond_lock(x, c) ((c) ? ({ __acquire(x); 1; }) : 0) +# define __cond_acquire(x, c) ((c) ? ({ __acquire(x); 1; }) : 0) #else /* !__CHECKER__ */ @@ -25,7 +25,7 @@ # define __releases(x) # define __acquire(x) (void)0 # define __release(x) (void)0 -# define __cond_lock(x, c) (c) +# define __cond_acquire(x, c) (c) #endif /* __CHECKER__ */ diff --git a/include/linux/mm.h b/include/linux/mm.h index 7b1068ddcbb7..a2365f4d6826 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2738,7 +2738,7 @@ static inline pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr, spinlock_t **ptl) { pte_t *ptep; - __cond_lock(*ptl, ptep = __get_locked_pte(mm, addr, ptl)); + __cond_acquire(*ptl, ptep = __get_locked_pte(mm, addr, ptl)); return ptep; } @@ -3029,7 +3029,7 @@ static inline pte_t *__pte_offset_map(pmd_t *pmd, unsigned long addr, { pte_t *pte; - __cond_lock(RCU, pte = ___pte_offset_map(pmd, addr, pmdvalp)); + __cond_acquire(RCU, pte = ___pte_offset_map(pmd, addr, pmdvalp)); return pte; } static inline pte_t *pte_offset_map(pmd_t *pmd, unsigned long addr) @@ -3044,7 +3044,7 @@ static inline pte_t *pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd, { pte_t *pte; - __cond_lock(RCU, __cond_lock(*ptlp, + __cond_acquire(RCU, __cond_acquire(*ptlp, pte = __pte_offset_map_lock(mm, pmd, addr, ptlp))); return pte; } diff --git a/include/linux/rwlock.h b/include/linux/rwlock.h index 5b87c6f4a243..58c346947aa2 100644 --- a/include/linux/rwlock.h +++ b/include/linux/rwlock.h @@ -49,8 +49,8 @@ do { \ * regardless of whether CONFIG_SMP or CONFIG_PREEMPT are set. The various * methods are defined as nops in the case they are not required. */ -#define read_trylock(lock) __cond_lock(lock, _raw_read_trylock(lock)) -#define write_trylock(lock) __cond_lock(lock, _raw_write_trylock(lock)) +#define read_trylock(lock) __cond_acquire(lock, _raw_read_trylock(lock)) +#define write_trylock(lock) __cond_acquire(lock, _raw_write_trylock(lock)) #define write_lock(lock) _raw_write_lock(lock) #define read_lock(lock) _raw_read_lock(lock) diff --git a/include/linux/rwlock_rt.h b/include/linux/rwlock_rt.h index 7d81fc6918ee..5320b4b66405 100644 --- a/include/linux/rwlock_rt.h +++ b/include/linux/rwlock_rt.h @@ -55,7 +55,7 @@ static __always_inline void read_lock_irq(rwlock_t *rwlock) flags = 0; \ } while (0) -#define read_trylock(lock) __cond_lock(lock, rt_read_trylock(lock)) +#define read_trylock(lock) __cond_acquire(lock, rt_read_trylock(lock)) static __always_inline void read_unlock(rwlock_t *rwlock) { @@ -111,7 +111,7 @@ static __always_inline void write_lock_irq(rwlock_t *rwlock) flags = 0; \ } while (0) -#define write_trylock(lock) __cond_lock(lock, rt_write_trylock(lock)) +#define write_trylock(lock) __cond_acquire(lock, rt_write_trylock(lock)) #define write_trylock_irqsave(lock, flags) \ ({ \ diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index d5d03d919df8..3304cce4b1bf 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -741,7 +741,7 @@ static inline struct sighand_struct *lock_task_sighand(struct task_struct *task, struct sighand_struct *ret; ret = __lock_task_sighand(task, flags); - (void)__cond_lock(&task->sighand->siglock, ret); + (void)__cond_acquire(&task->sighand->siglock, ret); return ret; } diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h index 63dd8cf3c3c2..678e6f0679a1 100644 --- a/include/linux/spinlock.h +++ b/include/linux/spinlock.h @@ -212,7 +212,7 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) * various methods are defined as nops in the case they are not * required. */ -#define raw_spin_trylock(lock) __cond_lock(lock, _raw_spin_trylock(lock)) +#define raw_spin_trylock(lock) __cond_acquire(lock, _raw_spin_trylock(lock)) #define raw_spin_lock(lock) _raw_spin_lock(lock) @@ -284,7 +284,7 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) #define raw_spin_unlock_bh(lock) _raw_spin_unlock_bh(lock) #define raw_spin_trylock_bh(lock) \ - __cond_lock(lock, _raw_spin_trylock_bh(lock)) + __cond_acquire(lock, _raw_spin_trylock_bh(lock)) #define raw_spin_trylock_irq(lock) \ ({ \ @@ -499,21 +499,21 @@ static inline int rwlock_needbreak(rwlock_t *lock) */ extern int _atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock); #define atomic_dec_and_lock(atomic, lock) \ - __cond_lock(lock, _atomic_dec_and_lock(atomic, lock)) + __cond_acquire(lock, _atomic_dec_and_lock(atomic, lock)) extern int _atomic_dec_and_lock_irqsave(atomic_t *atomic, spinlock_t *lock, unsigned long *flags); #define atomic_dec_and_lock_irqsave(atomic, lock, flags) \ - __cond_lock(lock, _atomic_dec_and_lock_irqsave(atomic, lock, &(flags))) + __cond_acquire(lock, _atomic_dec_and_lock_irqsave(atomic, lock, &(flags))) extern int _atomic_dec_and_raw_lock(atomic_t *atomic, raw_spinlock_t *lock); #define atomic_dec_and_raw_lock(atomic, lock) \ - __cond_lock(lock, _atomic_dec_and_raw_lock(atomic, lock)) + __cond_acquire(lock, _atomic_dec_and_raw_lock(atomic, lock)) extern int _atomic_dec_and_raw_lock_irqsave(atomic_t *atomic, raw_spinlock_t *lock, unsigned long *flags); #define atomic_dec_and_raw_lock_irqsave(atomic, lock, flags) \ - __cond_lock(lock, _atomic_dec_and_raw_lock_irqsave(atomic, lock, &(flags))) + __cond_acquire(lock, _atomic_dec_and_raw_lock_irqsave(atomic, lock, &(flags))) int __alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *lock_mask, size_t max_size, unsigned int cpu_mult, diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h index f6499c37157d..eaad4dd2baac 100644 --- a/include/linux/spinlock_rt.h +++ b/include/linux/spinlock_rt.h @@ -123,13 +123,13 @@ static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, } #define spin_trylock(lock) \ - __cond_lock(lock, rt_spin_trylock(lock)) + __cond_acquire(lock, rt_spin_trylock(lock)) #define spin_trylock_bh(lock) \ - __cond_lock(lock, rt_spin_trylock_bh(lock)) + __cond_acquire(lock, rt_spin_trylock_bh(lock)) #define spin_trylock_irq(lock) \ - __cond_lock(lock, rt_spin_trylock(lock)) + __cond_acquire(lock, rt_spin_trylock(lock)) #define spin_trylock_irqsave(lock, flags) \ ({ \ diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 1b675aee99a9..dbada41c10ad 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -63,7 +63,7 @@ static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags); #define lock_timer(tid, flags) \ ({ struct k_itimer *__timr; \ - __cond_lock(&__timr->it_lock, __timr = __lock_timer(tid, flags)); \ + __cond_acquire(&__timr->it_lock, __timr = __lock_timer(tid, flags)); \ __timr; \ }) diff --git a/tools/include/linux/compiler_types.h b/tools/include/linux/compiler_types.h index d09f9dc172a4..b1db30e510d0 100644 --- a/tools/include/linux/compiler_types.h +++ b/tools/include/linux/compiler_types.h @@ -20,7 +20,7 @@ # define __releases(x) __attribute__((context(x,1,0))) # define __acquire(x) __context__(x,1) # define __release(x) __context__(x,-1) -# define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) +# define __cond_acquire(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) #else /* __CHECKER__ */ /* context/locking */ # define __must_hold(x) @@ -28,7 +28,7 @@ # define __releases(x) # define __acquire(x) (void)0 # define __release(x) (void)0 -# define __cond_lock(x,c) (c) +# define __cond_acquire(x,c) (c) #endif /* __CHECKER__ */ /* Compiler specific macros. */
Just like the pairing of attribute __acquires() with a matching function-like macro __acquire(), the attribute __cond_acquires() should have a matching function-like macro __cond_acquire(). To be consistent, rename __cond_lock() to __cond_acquire(). Signed-off-by: Marco Elver <elver@google.com> --- drivers/net/wireless/intel/iwlwifi/iwl-trans.h | 2 +- drivers/net/wireless/intel/iwlwifi/pcie/internal.h | 2 +- include/linux/compiler-capability-analysis.h | 4 ++-- include/linux/mm.h | 6 +++--- include/linux/rwlock.h | 4 ++-- include/linux/rwlock_rt.h | 4 ++-- include/linux/sched/signal.h | 2 +- include/linux/spinlock.h | 12 ++++++------ include/linux/spinlock_rt.h | 6 +++--- kernel/time/posix-timers.c | 2 +- tools/include/linux/compiler_types.h | 4 ++-- 11 files changed, 24 insertions(+), 24 deletions(-)