[PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity
Thomas Hellström
thomas.hellstrom at linux.intel.com
Thu Sep 7 08:59:57 UTC 2023
Hi,
On 9/6/23 10:34, Christian König wrote:
> Am 05.09.23 um 16:29 schrieb Thomas Hellström:
>> Hi, Christian
>>
>> On 9/5/23 15:14, Christian König wrote:
>>> Am 05.09.23 um 10:58 schrieb Thomas Hellström:
>>>> If *any* object of a certain WW mutex class is locked, lockdep will
>>>> consider *all* mutexes of that class as locked. Also the lock
>>>> allocation
>>>> tracking code will apparently register only the address of the first
>>>> mutex locked in a sequence.
>>>> This has the odd consequence that if that first mutex is unlocked and
>>>> its memory then freed, the lock alloc tracking code will assume
>>>> that memory
>>>> is freed with a held lock in there.
>>>>
>>>> For now, work around that for drm_exec by releasing the first grabbed
>>>> object lock last.
>>>>
>>>> Related lock alloc tracking warning:
>>>> [ 322.660067] =========================
>>>> [ 322.660070] WARNING: held lock freed!
>>>> [ 322.660074] 6.5.0-rc7+ #155 Tainted: G U N
>>>> [ 322.660078] -------------------------
>>>> [ 322.660081] kunit_try_catch/4981 is freeing memory
>>>> ffff888112adc000-ffff888112adc3ff, with a lock still held there!
>>>> [ 322.660089] ffff888112adc1a0
>>>> (reservation_ww_class_mutex){+.+.}-{3:3}, at:
>>>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>>>> [ 322.660104] 2 locks held by kunit_try_catch/4981:
>>>> [ 322.660108] #0: ffffc9000343fe18
>>>> (reservation_ww_class_acquire){+.+.}-{0:0}, at:
>>>> test_early_put+0x22f/0x490 [drm_exec_test]
>>>> [ 322.660123] #1: ffff888112adc1a0
>>>> (reservation_ww_class_mutex){+.+.}-{3:3}, at:
>>>> drm_exec_lock_obj+0x11a/0x600 [drm_exec]
>>>> [ 322.660135]
>>>> stack backtrace:
>>>> [ 322.660139] CPU: 7 PID: 4981 Comm: kunit_try_catch Tainted:
>>>> G U N 6.5.0-rc7+ #155
>>>> [ 322.660146] Hardware name: ASUS System Product Name/PRIME
>>>> B560M-A AC, BIOS 0403 01/26/2021
>>>> [ 322.660152] Call Trace:
>>>> [ 322.660155] <TASK>
>>>> [ 322.660158] dump_stack_lvl+0x57/0x90
>>>> [ 322.660164] debug_check_no_locks_freed+0x20b/0x2b0
>>>> [ 322.660172] slab_free_freelist_hook+0xa1/0x160
>>>> [ 322.660179] ? drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>>>> [ 322.660186] __kmem_cache_free+0xb2/0x290
>>>> [ 322.660192] drm_exec_unlock_all+0x168/0x2a0 [drm_exec]
>>>> [ 322.660200] drm_exec_fini+0xf/0x1c0 [drm_exec]
>>>> [ 322.660206] test_early_put+0x289/0x490 [drm_exec_test]
>>>> [ 322.660215] ? __pfx_test_early_put+0x10/0x10 [drm_exec_test]
>>>> [ 322.660222] ? __kasan_check_byte+0xf/0x40
>>>> [ 322.660227] ? __ksize+0x63/0x140
>>>> [ 322.660233] ? drmm_add_final_kfree+0x3e/0xa0 [drm]
>>>> [ 322.660289] ? _raw_spin_unlock_irqrestore+0x30/0x60
>>>> [ 322.660294] ? lockdep_hardirqs_on+0x7d/0x100
>>>> [ 322.660301] ? __pfx_kunit_try_run_case+0x10/0x10 [kunit]
>>>> [ 322.660310] ?
>>>> __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
>>>> [ 322.660319] kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>>>> [ 322.660328] kthread+0x2e7/0x3c0
>>>> [ 322.660334] ? __pfx_kthread+0x10/0x10
>>>> [ 322.660339] ret_from_fork+0x2d/0x70
>>>> [ 322.660345] ? __pfx_kthread+0x10/0x10
>>>> [ 322.660349] ret_from_fork_asm+0x1b/0x30
>>>> [ 322.660358] </TASK>
>>>> [ 322.660818] ok 8 test_early_put
>>>>
>>>> Cc: Christian König <christian.koenig at amd.com>
>>>> Cc: Boris Brezillon <boris.brezillon at collabora.com>
>>>> Cc: Danilo Krummrich <dakr at redhat.com>
>>>> Cc: dri-devel at lists.freedesktop.org
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/drm_exec.c | 2 +-
>>>> include/drm/drm_exec.h | 35 +++++++++++++++++++++++++++++++----
>>>> 2 files changed, 32 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
>>>> index ff69cf0fb42a..5d2809de4517 100644
>>>> --- a/drivers/gpu/drm/drm_exec.c
>>>> +++ b/drivers/gpu/drm/drm_exec.c
>>>> @@ -56,7 +56,7 @@ static void drm_exec_unlock_all(struct drm_exec
>>>> *exec)
>>>> struct drm_gem_object *obj;
>>>> unsigned long index;
>>>> - drm_exec_for_each_locked_object(exec, index, obj) {
>>>> + drm_exec_for_each_locked_object_reverse(exec, index, obj) {
>>>
>>> Well that's a really good catch, just one more additional thought
>>> below.
>>>
>>>> dma_resv_unlock(obj->resv);
>>>> drm_gem_object_put(obj);
>>>> }
>>>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>>>> index e0462361adf9..55764cf7c374 100644
>>>> --- a/include/drm/drm_exec.h
>>>> +++ b/include/drm/drm_exec.h
>>>> @@ -51,6 +51,20 @@ struct drm_exec {
>>>> struct drm_gem_object *prelocked;
>>>> };
>>>> +/**
>>>> + * drm_exec_obj() - Return the object for a give drm_exec index
>>>> + * @exec: Pointer to the drm_exec context
>>>> + * @index: The index.
>>>> + *
>>>> + * Return: Pointer to the locked object corresponding to @index if
>>>> + * index is within the number of locked objects. NULL otherwise.
>>>> + */
>>>> +static inline struct drm_gem_object *
>>>> +drm_exec_obj(struct drm_exec *exec, unsigned long index)
>>>> +{
>>>> + return index < exec->num_objects ? exec->objects[index] : NULL;
>>>> +}
>>>> +
>>>> /**
>>>> * drm_exec_for_each_locked_object - iterate over all the locked
>>>> objects
>>>> * @exec: drm_exec object
>>>> @@ -59,10 +73,23 @@ struct drm_exec {
>>>> *
>>>> * Iterate over all the locked GEM objects inside the drm_exec
>>>> object.
>>>> */
>>>> -#define drm_exec_for_each_locked_object(exec, index, obj) \
>>>> - for (index = 0, obj = (exec)->objects[0]; \
>>>> - index < (exec)->num_objects; \
>>>> - ++index, obj = (exec)->objects[index])
>>>> +#define drm_exec_for_each_locked_object(exec, index, obj) \
>>>> + for ((index) = 0; ((obj) = drm_exec_obj(exec, index)); ++(index))
>>>
>>> Mhm, that makes it possible to modify the number of objects while
>>> inside the loop, doesn't it?
>>
>> Sorry, you lost me a bit there. Isn't that possible with the previous
>> code as well?
>
> Yeah, indeed. Reviewed-by: Christian König <christian.koenig at amd.com>
>
> Regards,
> Christian.
Thanks Boris, Danilo and Christian for review. I pushed this one to
drm-misc-next-fixes.
/Thomas
>
>>
>> /Thanks,
>>
>> Thomas
>>
>>
>>
>>>
>>> I'm not sure if that's a good idea or not.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +
>>>> +/**
>>>> + * drm_exec_for_each_locked_object_reverse - iterate over all the
>>>> locked
>>>> + * objects in reverse locking order
>>>> + * @exec: drm_exec object
>>>> + * @index: unsigned long index for the iteration
>>>> + * @obj: the current GEM object
>>>> + *
>>>> + * Iterate over all the locked GEM objects inside the drm_exec
>>>> object in
>>>> + * reverse locking order. Note that @index may go below zero and
>>>> wrap,
>>>> + * but that will be caught by drm_exec_object(), returning a NULL
>>>> object.
>>>> + */
>>>> +#define drm_exec_for_each_locked_object_reverse(exec, index,
>>>> obj) \
>>>> + for ((index) = (exec)->num_objects - 1; \
>>>> + ((obj) = drm_exec_obj(exec, index)); --(index))
>>>> /**
>>>> * drm_exec_until_all_locked - loop until all GEM objects are locked
>>>
>
More information about the dri-devel
mailing list