Messages in this thread | | | Date | Wed, 28 Mar 2012 10:48:24 +0200 | From | Wolfram Sang <> | Subject | Re: [PATCH 11/13] Add a discussion on why spin_is_locked() is bad to spinlocks.txt |
| |
On Tue, Mar 27, 2012 at 05:47:15PM -0700, Andi Kleen wrote: > From: Andi Kleen <ak@linux.intel.com> > > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > Documentation/spinlocks.txt | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/Documentation/spinlocks.txt b/Documentation/spinlocks.txt > index 9dbe885..1787229 100644 > --- a/Documentation/spinlocks.txt > +++ b/Documentation/spinlocks.txt > @@ -146,6 +146,49 @@ indeed), while write-locks need to protect themselves against interrupts. > > ---- > > +spin_is_locked is a bad idea > + > +spin_is_locked checks if a lock is currently hold. On uniprocessor kernels > +it always returns 0. In general this function should be avoided because most > +uses of it are either redundant or broken. > + > +People often use spin_is_locked() to check if a particular lock is hold when a function > +is called to enforce a locking discipline, like > + > + WARN_ON(!spin_is_locked(!my_lock)) > + > +or > + > + BUG_ON(!spin_is_locked(!my_lock))
'&my_lock' instead of '!my_lock' probably.
> + > +or some variant of those. > + > +This does not work on uniprocessor kernels because they will always fail. > +While there are ways around that they are ugly and not recommended. > +Better use lockdep_assert_held(). This also only checks on a lock debugging > +kernel (which you should occasionally run on your code anyways because > +it catches many more problems). > + > +In generally this would be better done with static annotation anyways > +(there's some support for it in sparse) > + > + BUG_ON(spin_is_locked(obj->lock)); > + kfree(obj); > + > +Another usage is checking whether a lock is not hold when freeing an object.
I'd suggest to move this sentence above the code example. On first read, I was confused what the code should tell me regarding annotations :)
> +However this is redundant because lock debugging supports this anyways > +without explicit code. Just delete the BUG_ON. > + > +A third usage is to check in a console function if a lock is hold, to get > +a panic crash dump out even when some other thread died in it. > +This is better implemented with spin_try_lock() et.al. and a timeout. > + > +Other usages are usually simply races. > + > +In summary just don't use it.
At this point, I was wondering when it actually can be used? Otherwise it probably would have been removed from the kernel or marked deprecated, I'd think?
Regards,
Wolfram
-- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |