lkml.org 
[lkml]   [2012]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 11/11] checkpatch: Check for spin_is_locked
    On Fri, Mar 16, 2012 at 12:22:35PM -0700, Joe Perches wrote:
    > On Fri, 2012-03-16 at 12:01 -0700, Andi Kleen wrote:
    > > From: Andi Kleen <ak@linux.intel.com>
    > > spin_is_locked is usually misused. In checkpatch.pl
    > > - warn when it is used at all
    > > - error out when it is asserted on free, because that's usually broken
    > []
    > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
    > []
    > > +# spin_is_locked is usually misused. warn about it.
    > > + if ($line =~ /\bspin_is_locked\s*\(/) {
    > > + # BUG_ON/WARN_ON(!spin_is_locked() is generally a bug
    > > + if ($line =~ /(BUG_ON|WARN_ON|ASSERT)\s*\(!spin_is_locked/) {
    > > + ERROR("ASSERT_SPIN_IS_LOCKED",
    > > + "Use lockdep_assert_held() instead of asserts on !spin_is_locked\n"
    > > + . $herecurr);
    > > + } else {
    > > + WARN("SPIN_IS_LOCKED",
    > > + "spin_is_locked is very rarely correctly used. Please reconsider\n"
    > > + . $herecurr)
    > > + }
    > > + }
    > > +
    >
    > I suggest you use a single --ignore string of
    > "SPIN_IS_LOCKED" instead of different ones.

    Done.
    >
    > Grammar might be improved in the WARN. Maybe:
    > "Using spin_is_locked() is generally wrong. See [foo documentation]\n"

    Done.

    >
    > Also, like what was done for yield(), perhaps
    > some kernel-doc content would be useful.

    Added to spinlocks.txt and spinlock.h


    +/**
    + * spin_is_locked() - Check if a spinlock is being held.
    + * @lock: Lock to check.
    + *
    + * This function should normally not be used. Especially using it in
    + * WARN and BUG_ONs is usually incorrect or redundant.
    + * If you want to check if a lock is held in a function
    + * use lockdep_assert_held(). A lot of other usages are racy.
    + */


    +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))
    +
    +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.
    +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.






    --
    ak@linux.intel.com -- Speaking for myself only.


    \
     
     \ /
      Last update: 2012-03-16 21:17    [W:0.024 / U:1.940 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site