Re: [PATCH] drm/amdgpu: don't increase UMC RAS UE count if no new bad page

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 2/14/2023 11:42 AM, Zhou1, Tao wrote:
> 
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Tuesday, February 14, 2023 12:55 PM
>> To: Zhou1, Tao <Tao.Zhou1@amd.com>; amd-gfx@lists.freedesktop.org; Zhang,
>> Hawking <Hawking.Zhang@amd.com>; Yang, Stanley
>> <Stanley.Yang@amd.com>; Chai, Thomas <YiPeng.Chai@amd.com>; Li, Candice
>> <Candice.Li@amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: don't increase UMC RAS UE count if no new
>> bad page
>>
>>
>>
>> On 2/14/2023 7:56 AM, Zhou1, Tao wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>> Sent: Monday, February 13, 2023 8:38 PM
>>>> To: Zhou1, Tao <Tao.Zhou1@amd.com>; amd-gfx@lists.freedesktop.org;
>>>> Zhang, Hawking <Hawking.Zhang@amd.com>; Yang, Stanley
>>>> <Stanley.Yang@amd.com>; Chai, Thomas <YiPeng.Chai@amd.com>; Li,
>>>> Candice <Candice.Li@amd.com>
>>>> Subject: Re: [PATCH] drm/amdgpu: don't increase UMC RAS UE count if
>>>> no new bad page
>>>>
>>>>
>>>>
>>>> On 2/10/2023 2:15 PM, Tao Zhou wrote:
>>>>> If a UMC bad page is reserved but not freed by an application, the
>>>>> application may trigger uncorrectable error repeatly by accessing the page.
>>>>>
>>>>
>>>> There is amdgpu_ras_check_bad_page which checks if address is already
>>>> part of an existing bad page. Can't that be used?
>>>>
>>>> Thanks,
>>>> Lijo
>>>
>>> [Tao] amdgpu_ras_check_bad_page is already called in
>> amdgpu_ras_add_bad_pages, this patch just makes use of the result of
>> amdgpu_ras_check_bad_page.
>>>
>>
>> In the patch, below two are called after error count is set to 0.
>> 	amdgpu_ras_save_bad_pages
>> 	amdgpu_dpm_send_hbm_bad_pages_num
>>
>> Instead of that, just check if it's an existing badpage which is repeatedly
>> accessed and proceed directly to the next step (reset if
>> specified)
>>
>> 	if (amdgpu_ras_check_bad_page(adev, address))
>> 		set error count to 0;
>> 		goto reset_logic;
>>
>> Thanks,
>> Lijo
> 
> [Tao] 1. amdgpu_ras_check_bad_page checks only one page, but one ue can generate 16 or more pages.

In this case, it makes sense to do as per the patch. Thanks for the 
explanation.

Thanks,
Lijo

> 2. if no new bad page is found, amdgpu_ras_save_bad_pages will do nothing and ras_num_recs won't increase.
> 3. gpu reset logic should follow the old way even ue count is set to 0.
> 
> This patch only set ue count to 0 if there is no new bad page, but other logic has no change.
> 
>>
>>>>
>>>>> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 9 ++++++++-
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c | 6 +++++-
>>>>>     2 files changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> index e85c4689ce2c..eafe01a24349 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> @@ -2049,7 +2049,7 @@ int amdgpu_ras_add_bad_pages(struct
>>>> amdgpu_device *adev,
>>>>>     {
>>>>>     	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
>>>>>     	struct ras_err_handler_data *data;
>>>>> -	int ret = 0;
>>>>> +	int ret = 0, old_cnt;
>>>>>     	uint32_t i;
>>>>>
>>>>>     	if (!con || !con->eh_data || !bps || pages <= 0) @@ -2060,6
>>>>> +2060,8 @@ int amdgpu_ras_add_bad_pages(struct amdgpu_device *adev,
>>>>>     	if (!data)
>>>>>     		goto out;
>>>>>
>>>>> +	old_cnt = data->count;
>>>>> +
>>>>>     	for (i = 0; i < pages; i++) {
>>>>>     		if (amdgpu_ras_check_bad_page_unlock(con,
>>>>>     			bps[i].retired_page << AMDGPU_GPU_PAGE_SHIFT))
>>>> @@ -2079,6
>>>>> +2081,11 @@ int amdgpu_ras_add_bad_pages(struct amdgpu_device
>> *adev,
>>>>>     		data->count++;
>>>>>     		data->space_left--;
>>>>>     	}
>>>>> +
>>>>> +	/* all pages have been reserved before, no new bad page */
>>>>> +	if (old_cnt == data->count)
>>>>> +		ret = -EEXIST;
>>>>> +
>>>>>     out:
>>>>>     	mutex_unlock(&con->recovery_lock);
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>>>> index 1c7fcb4f2380..772c431e4065 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>>>> @@ -145,8 +145,12 @@ static int
>> amdgpu_umc_do_page_retirement(struct
>>>>> amdgpu_device *adev,
>>>>>
>>>>>     		if ((amdgpu_bad_page_threshold != 0) &&
>>>>>     			err_data->err_addr_cnt) {
>>>>> -			amdgpu_ras_add_bad_pages(adev, err_data->err_addr,
>>>>> +			ret = amdgpu_ras_add_bad_pages(adev, err_data-
>>>>> err_addr,
>>>>>     						err_data->err_addr_cnt);
>>>>> +			/* if no new bad page is found, no need to increase ue
>>>> count */
>>>>> +			if (ret == -EEXIST)
>>>>> +				err_data->ue_count = 0;
>>>>> +
>>>>>     			amdgpu_ras_save_bad_pages(adev);
>>>>>
>>>>>     			amdgpu_dpm_send_hbm_bad_pages_num(adev,
>>>>> con->eeprom_control.ras_num_recs);