From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 04CF3EE14D4 for ; Thu, 7 Sep 2023 09:00:45 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6743110E090; Thu, 7 Sep 2023 09:00:44 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 20AEE10E793; Thu, 7 Sep 2023 09:00:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1694077213; x=1725613213; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=Wyw87rnJuGLXTnu/ma+xIoxzB8yhxJz3TqBc76xzgQA=; b=MA6ADBsxPDRFNgywhQDVbjuYAqkr0c23xtXrKVNYG72Y/BaYSjPhJ94L xBR6Z+TEqc1sm2TzlFq2j5BhHhO1AO8+C1xEkYqveuKEtgIiPrePT6y6n vpXC+fmi4OoVjL2yw3zMKGlJnrHKxLgWUKij6uofU5vCKFxhfwFggku93 uXBiXtWhdDwn+kRjceCB9jw4StQvQTQQb75c3eg+lHmMavP7GQcy6iZWm UxLv08L77Hge5oLG1gXx/H2jMIYQ+NKb547OjQZVVs5l1j8YXnOItkW4x CtK+90EojDEGBVpPThyFFckzbLd89Q2loB8ccnCoUXs0TpBQI+XcmhKg6 Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10825"; a="374691931" X-IronPort-AV: E=Sophos;i="6.02,234,1688454000"; d="scan'208";a="374691931" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Sep 2023 02:00:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10825"; a="1072805238" X-IronPort-AV: E=Sophos;i="6.02,234,1688454000"; d="scan'208";a="1072805238" Received: from lsilistr-mobl.ger.corp.intel.com (HELO [10.249.254.23]) ([10.249.254.23]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Sep 2023 01:59:59 -0700 Message-ID: <9a9bff2c-a6d8-6821-9180-6e33a4b577a6@linux.intel.com> Date: Thu, 7 Sep 2023 10:59:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH 3/3] drm/drm_exec: Work around a WW mutex lockdep oddity To: =?UTF-8?Q?Christian_K=c3=b6nig?= , intel-xe@lists.freedesktop.org, dri-devel@lists.freedesktop.org References: <20230905085832.2103-1-thomas.hellstrom@linux.intel.com> <20230905085832.2103-4-thomas.hellstrom@linux.intel.com> <2457e9ba-ecdb-b427-692b-a5c85f9e8828@linux.intel.com> <13c39e75-2ba7-2a1c-ab9b-acf10ad23a3f@gmail.com> Content-Language: en-US From: =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= In-Reply-To: <13c39e75-2ba7-2a1c-ab9b-acf10ad23a3f@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Boris Brezillon , Danilo Krummrich Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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]  >>>> [  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]  >>>> [  322.660818]     ok 8 test_early_put >>>> >>>> Cc: Christian König >>>> Cc: Boris Brezillon >>>> Cc: Danilo Krummrich >>>> Cc: dri-devel@lists.freedesktop.org >>>> Signed-off-by: Thomas Hellström >>>> --- >>>>   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 > > 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 >>> >