Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <e26e6ef8-6665-0b9d-804f-cf107f1788d4@amd.com>
Date:   Tue, 26 Apr 2022 10:55:34 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] gpu: drm: remove redundant dma_fence_put() when
 drm_sched_job_add_dependency() fails
Content-Language: en-US
To:     Hangyu Hua <hbh25y@gmail.com>, yuq825@gmail.com, airlied@linux.ie,
        daniel@ffwll.ch
Cc:     dri-devel@lists.freedesktop.org, lima@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20220425083645.25922-1-hbh25y@gmail.com>
 <b7d8956e-5ac6-8200-d8b1-4ce828f04665@amd.com>
 <79b198d0-eff2-d658-4b5e-9084a834fc93@gmail.com>
From:   Andrey Grodzovsky <andrey.grodzovsky@amd.com>
In-Reply-To: <79b198d0-eff2-d658-4b5e-9084a834fc93@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 2
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T3BZbVRJTlpNcTF6ZnVUSCsvYkR5cDRhc1NKVEYyUll4UFFVRUFteTNkOHBR?=
 =?utf-8?B?V0ZFR1FCeE9id2Z2SEg1b0ZxdFdGMnJEWFlLVjExZVlCMDB3ME93cmdrNjBt?=
 =?utf-8?B?Q2xoWU5uMC9peHduMFNFbTVISlJqZ1hrNUttMVN1NU52RFVjMmhNQmROU2g2?=
 =?utf-8?B?a1MvTkUzZHlXdU0xSmcwcVA1cDdSOW9FakRzc2xPRGJCbkVwZzZRNGlQbmxR?=
 =?utf-8?B?YVR0RWxJSHFjMC9QSXordXM2ZnpxSGRYNFFJU2g5SVBiMHo2ZXZmNE10M2Nt?=
 =?utf-8?B?ZnJIRDc4VnNVNFd2Zk9nRnlrMXRvTG4xV1BFc1FOUC9HQ0VwQXRIV21WYThZ?=
 =?utf-8?B?cXRuQlVRS1VRa2VqVUtuMnd5Y0VQSVdiWVVuZzdtRDUyRzEwRjdjbVdubFJs?=
 =?utf-8?B?bmpJd0lGbSs0aTdpZVR5ejVnWWI0dTZpcVlZT25xR3dabnNtOExFb2FhRnVS?=
 =?utf-8?B?T3U3dEZEUHlyN2o1c2ZMOGVzZGY0N1dkR1hsVGFibExhRVE1NWJHZURHTEtX?=
 =?utf-8?B?TlQ0TDNlK2VaS1dSY1ZxOU0xT1FmWG9Hem8xRHVZTGl2UEJQUHp3Q3AzV3Fr?=
 =?utf-8?B?R3VhRlltdEVEdy9NZWtNU0t2aWtJQXhuaUttcHZieDlGRCtxSjlOUkYvejE1?=
 =?utf-8?B?eWZ4M0hUNmw2c2RxUXlQV1g3VXZFNTNDclFvN3hTcUxLRUI0VS91SWlNbENo?=
 =?utf-8?B?QjJOcVA1OS8xMjhXQzNLUjVxRU1tZDdNVm9JZlVJY2p5Vmh1SHpqYkIxdHJp?=
 =?utf-8?B?NU03aTRRamcwK0J4ajJ2cmt1cTZ1TFRFV3FsWXQxZ1B1Q3ZVeTZVUWdzMUZa?=
 =?utf-8?B?ekF0WkdTby91UjFWeDJ6dEpIbWhhLzZ1eGk3M2hVQlVNcG40ZEhpRHVSQS9E?=
 =?utf-8?B?SmVZeVJMS0NkcWVYNXNoZ1hGU1NoaDdOcy9OcCtRNUJKaElkVlhuUXhGajZO?=
 =?utf-8?B?Nmw0UTBoZ3NWeEkxNVdobHI4QVFiTSs5N1c1Tkd4Y2VYZmZpNitBYld0STZJ?=
 =?utf-8?B?UWthQUJ1TWxoWmlRNXN1eGRSTnM2UXcyZXd1OFpydlVIZG5CSTU3cGkrVVZV?=
 =?utf-8?B?Yld2QzNubTFHTWg5UmpCakRUTzZhMFhMK0E5ajdkMzFLbCtoSmlBcHU3emE2?=
 =?utf-8?B?Ri9EdEM0UzNoaFUrUXVNK1gycnZCeDl3QWVDVCsvMUpjb0h6bjRYM2hJcm85?=
 =?utf-8?B?Y2RJTjI1V0t2a2ZOc1N1TjZFbkUrRFJ5bXV4d1ZTK0hSalhLMnJZS0V2L042?=
 =?utf-8?B?WGliNk83NHVHcTZtczBselYyRDJEUnlSS1pCWFNUbklreDNTaExYL01TQ3Vy?=
 =?utf-8?B?dkFHUDRaMmVPVUM2RkVrVFpVZi85cWgvNWxuTzhBa0JPclkrenlKNkpoYkNC?=
 =?utf-8?B?eFZGMDkwdHpJTkdvV0d3aXpac3oxb3RDTEswOG9oaFkyUllPOGxmdVptK3dh?=
 =?utf-8?B?VkgyZGxWaHl4Z29NdytQUmcyMVFOZzJqMHg5cGVpdFlJaFM5Mm1MR3hUbml1?=
 =?utf-8?B?ZzBmemRBbGhSbVordGx3eFE5QlZRNkJ0c29EWUFJRlo3aFBzOURoV3FSSzZW?=
 =?utf-8?B?RFIwQWtRMldkMVVjM1o3YmZ4MkQ5aHlaRUN4VHA2SDg4TFVYTjJCQjZWYjFt?=
 =?utf-8?B?NzlBSTlKb1FONGc4ZWx4bE5NOTlDMkdOdkh2Z211aGQ4Q0UzSlN2a1pUSUNK?=
 =?utf-8?B?ZFJiajZPbmU4R3RqUU5ya0hIb1ZvSXdadnNad1dlaEtHeHdwTzNPNVpIWXlq?=
 =?utf-8?B?Yjg5Uk82YkYramtIN1J2YTM2RUp5STRmdjZpSnNGdFBZRlBJMVVWZVRsNlUw?=
 =?utf-8?B?Mm5uVDhEOXZrUVJJRGpaem15Qlg2enhiM1kxajJiZmFBLzBDdUpSckVXZk9F?=
 =?utf-8?B?VE9jTURleHd6N2xVQmN1OW9BNnhiYzk3WEJuR3dlRUlOYXFRQXNGTmMyUmZ2?=
 =?utf-8?B?ZXVkSW9Wa2dRM1VQWThiYlh2QTEzcjZiQnB5NnRDK1kyRjdDeGF6S2poTkQ3?=
 =?utf-8?B?WXgwWWg0NkE4aVVXWXppc1M1akpiaHhDT21ybyt4MHNEbjJXRm5ObGZXZWw3?=
 =?utf-8?B?LzEvRWxJK2Q0bUJocFIwVkhxYXk1Z3RuN1BNZjVYeHhjd3RjY08zaThobE9S?=
 =?utf-8?B?aWVmOTUxdGNKSHVrNE82NUhSeGZMSkF6VjB6WjlidlRlWWp6WnRkbE1Xa1Bx?=
 =?utf-8?B?NjhOMjYyLzFEbTgrTmJ6QmxuTTh3MWsybG12QmZ6SDg2RjdIRnNhcVlNSUdW?=
 =?utf-8?B?NGxqUW1zcG1SWHB3SHMwUkg0NEhQZ25LUEZJNk40MWNxZW1zSmJlcUZLcE9G?=
 =?utf-8?B?Y05vTHVHQnhmVHIwcUJ3bG9scXNZMXB6a1gwTXlsNWZLTVlrc3ZjQnR0aVRR?=
 =?utf-8?Q?LWNZdpsNfrLjepIcStJLRkaoRJKBDq9C7hD0rAgDSDjpH?=
X-MS-Exchange-AntiSpam-MessageData-1: Q1NWsJ5XvLA9GA==
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55a930f3-6965-4054-bd83-08da2794d320
X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1947.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2022 14:55:37.9783
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EIAwtq2I16vSUtrepKukoa6d/BdAyKPUeajNVWmwcMtb/jXxG5joDsOhf9SlE+H0bovkIWsHSJuoEUVkAAOMjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5309
X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2022-04-25 22:54, Hangyu Hua wrote:
> On 2022/4/25 23:42, Andrey Grodzovsky wrote:
>> On 2022-04-25 04:36, Hangyu Hua wrote:
>>
>>> When drm_sched_job_add_dependency() fails, dma_fence_put() will be 
>>> called
>>> internally. Calling it again after drm_sched_job_add_dependency() 
>>> finishes
>>> may result in a dangling pointer.
>>>
>>> Fix this by removing redundant dma_fence_put().
>>>
>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>> ---
>>>   drivers/gpu/drm/lima/lima_gem.c        | 1 -
>>>   drivers/gpu/drm/scheduler/sched_main.c | 1 -
>>>   2 files changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/lima/lima_gem.c 
>>> b/drivers/gpu/drm/lima/lima_gem.c
>>> index 55bb1ec3c4f7..99c8e7f6bb1c 100644
>>> --- a/drivers/gpu/drm/lima/lima_gem.c
>>> +++ b/drivers/gpu/drm/lima/lima_gem.c
>>> @@ -291,7 +291,6 @@ static int lima_gem_add_deps(struct drm_file 
>>> *file, struct lima_submit *submit)
>>>           err = drm_sched_job_add_dependency(&submit->task->base, 
>>> fence);
>>>           if (err) {
>>> -            dma_fence_put(fence);
>>>               return err;
>>
>>
>> Makes sense here
>>
>>
>>>           }
>>>       }
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index b81fceb0b8a2..ebab9eca37a8 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -708,7 +708,6 @@ int 
>>> drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>>>           dma_fence_get(fence);
>>>           ret = drm_sched_job_add_dependency(job, fence);
>>>           if (ret) {
>>> -            dma_fence_put(fence);
>>
>>
>>
>> Not sure about this one since if you look at the relevant commits -
>> 'drm/scheduler: fix drm_sched_job_add_implicit_dependencies' and
>> 'drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder'
>> You will see that the dma_fence_put here balances the extra 
>> dma_fence_get
>> above
>>
>> Andrey
>>
>
> I don't think so. I checked the call chain and found no additional 
> dma_fence_get(). But dma_fence_get() needs to be called before 
> drm_sched_job_add_dependency() to keep the counter balanced. 


I don't say there is an additional get, I just say that 
drm_sched_job_add_dependency doesn't grab an extra reference to the 
fences it stores so this needs to be done outside and for that
drm_sched_job_add_implicit_dependencies->dma_fence_get is called and, if 
this addition fails you just call dma_fence_put to keep the counter 
balanced.


> On the other hand, dma_fence_get() and dma_fence_put() are meaningless 
> here if threre is an extra dma_fence_get() beacause counter will not 
> decrease to 0 during drm_sched_job_add_dependency().
>
> I check the call chain as follows:
>
> msm_ioctl_gem_submit()
> -> submit_fence_sync()
> -> drm_sched_job_add_implicit_dependencies()


Can you maybe trace or print one such example of problematic refcount 
that you are trying to fix ? I still don't see where is the problem.

Andrey


>
> Thanks,
> Hangyu
>
>>
>>>               return ret;
>>>           }
>>>       }