Re: [PATCH v4 3/4] mutex: Add ww tests to lib/locking-selftest.c. v4

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

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