Op 28-05-13 21:18, Daniel Vetter schreef:
> On Tue, May 28, 2013 at 04:48:45PM +0200, Maarten Lankhorst wrote:
>> This stresses the lockdep code in some ways specifically useful to
>> ww_mutexes. It adds checks for most of the common locking errors.
>>
>> Changes since v1:
>> - Add tests to verify reservation_id is untouched.
>> - Use L() and U() macros where possible.
>>
>> Changes since v2:
>> - Use the ww_mutex api directly.
>> - Use macros for most of the code.
>> Changes since v3:
>> - Rework tests for the api changes.
>>
>> <snip>
>>
>> +static void ww_test_normal(void)
>> +{
>> + int ret;
>> +
>> + WWAI(&t);
>> +
>> + /*
>> + * test if ww_id is kept identical if not
>> + * called with any of the ww_* locking calls
>> + */
>> +
>> + /* mutex_lock (and indirectly, mutex_lock_nested) */
>> + o.ctx = (void *)~0UL;
>> + mutex_lock(&o.base);
>> + mutex_unlock(&o.base);
>> + WARN_ON(o.ctx != (void *)~0UL);
>> +
>> + /* mutex_lock_interruptible (and *_nested) */
>> + o.ctx = (void *)~0UL;
>> + ret = mutex_lock_interruptible(&o.base);
>> + if (!ret)
>> + mutex_unlock(&o.base);
>> + else
>> + WARN_ON(1);
>> + WARN_ON(o.ctx != (void *)~0UL);
>> +
>> + /* mutex_lock_killable (and *_nested) */
>> + o.ctx = (void *)~0UL;
>> + ret = mutex_lock_killable(&o.base);
>> + if (!ret)
>> + mutex_unlock(&o.base);
>> + else
>> + WARN_ON(1);
>> + WARN_ON(o.ctx != (void *)~0UL);
>> +
>> + /* trylock, succeeding */
>> + o.ctx = (void *)~0UL;
>> + ret = mutex_trylock(&o.base);
>> + WARN_ON(!ret);
>> + if (ret)
>> + mutex_unlock(&o.base);
>> + else
>> + WARN_ON(1);
>> + WARN_ON(o.ctx != (void *)~0UL);
>> +
>> + /* trylock, failing */
>> + o.ctx = (void *)~0UL;
>> + mutex_lock(&o.base);
>> + ret = mutex_trylock(&o.base);
>> + WARN_ON(ret);
>> + mutex_unlock(&o.base);
>> + WARN_ON(o.ctx != (void *)~0UL);
>> +
>> + /* nest_lock */
>> + o.ctx = (void *)~0UL;
>> + mutex_lock_nest_lock(&o.base, &t);
>> + mutex_unlock(&o.base);
>> + WARN_ON(o.ctx != (void *)~0UL);
>> +}
> Since we don't really allow this any more (instead allow ww_mutex_lock
> without context) do we need this test here really?
Yes. This test verifies all the normal locking paths are not affected by the ww_ctx changes.
>> +
>> +static void ww_test_two_contexts(void)
>> +{
>> + WWAI(&t);
>> + WWAI(&t2);
>> +}
>> +
>> +static void ww_test_context_unlock_twice(void)
>> +{
>> + WWAI(&t);
>> + WWAD(&t);
>> + WWAF(&t);
>> + WWAF(&t);
>> +}
>> +
>> +static void ww_test_object_unlock_twice(void)
>> +{
>> + WWL1(&o);
>> + WWU(&o);
>> + WWU(&o);
>> +}
>> +
>> +static void ww_test_spin_nest_unlocked(void)
>> +{
>> + raw_spin_lock_nest_lock(&lock_A, &o.base);
>> + U(A);
>> +}
> I don't quite see the point of this one here ...
It's a lockdep test that was missing. o.base is not locked. So lock_A is being nested into an unlocked lock, resulting in a lockdep error.
>> +
>> +static void ww_test_unneeded_slow(void)
>> +{
>> + int ret;
>> +
>> + WWAI(&t);
>> +
>> + ww_mutex_lock_slow(&o, &t);
>> +}
> I think checking the _slow debug stuff would be neat, i.e.
> - fail/success tests for properly unlocking all held locks
> - fail/success tests for lock_slow acquiring the right lock.
>
> Otherwise I didn't spot anything that seems missing in these self-tests
> here.
>
Yes it would be nice, doing so is left as an excercise for the reviewer, who failed to raise this point sooner. ;-)
~Maarten
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
