[PATCH 2/2] drm/amdgpu: Fix use of interruptible waiting

[Christian König]

> 1. The wait is short. There is not much benefit by interruptible 
> waiting.  The BO is a frame buffer BO. Are there many competitors to 
> lock this BO? If not, the wait is very short. This is my main reason 
> to change.
The problem is that all those waits can block the MM subsystem from 
reclaiming memory in an out of memory situation, e.g. when the process 
is terminated with a SIGKILL.

That's why the usual policy used in the drivers is to wait interruptible 
unless you absolutely need the wait uninterrupted (e.g. in a cleanup 
path for example).

> 2. In this function and caller functions, the error handling for such 
> interrupt is complicated and risky. When the waiting is interrupted by 
> a signal, the callers of this function need to handle this interrupt 
> error. I traced the calling stack all the way back to function 
> drm_mode_page_flip_ioctl. I could not find an issue. But I am not sure 
> about even upper layer. Is this IOCTL restartable? How about further 
> higher layer? Since I didn't see benefit in point 1. So I thought it 
> was a good idea to change.

The page flip IOCTL should be restartable. I think all drm IOCTLs with 
driver callbacks are restartable, but I'm not 100% sure, Michel probably 
knows that better.

There is even the CONFIG_DEBUG_WW_MUTEX_SLOWPATH option to throw random 
backoff cases into reserving the BO for testing. But I think that only 
applies to when multiple BOs are reserved at the same time.

Might be a good idea to create something similar for all interruptible 
lock acquires to test if they are really restartable. That will most 
likely yield quite a bunch of cases where the handling isn't correct.

Regards,
Christian.

Am 26.04.2017 um 21:19 schrieb Alex Xie:
> Hi,
>
> I knew this is part of ioctl. And I intended to fix this interruptible 
> waiting in an ioctl.
>
> ioctl is one of driver interfaces, where interruptible waiting is good 
> in some scenarios. For example, if ioctl performs IO operations in 
> atomic way, we don't want ioctl to be interrupted by a signal.
>
> Interruptible waiting is good when we need to wait for longer time, 
> for example, waiting for an input data for indefinite time. We don't 
> want the process not responding to signals. Interruptible waitings can 
> be good in read/write/ioctl interfaces.
>
> For this patch,
>
> 1. The wait is short. There is not much benefit by interruptible 
> waiting.  The BO is a frame buffer BO. Are there many competitors to 
> lock this BO? If not, the wait is very short. This is my main reason 
> to change.
> 2. In this function and caller functions, the error handling for such 
> interrupt is complicated and risky. When the waiting is interrupted by 
> a signal, the callers of this function need to handle this interrupt 
> error. I traced the calling stack all the way back to function 
> drm_mode_page_flip_ioctl. I could not find an issue. But I am not sure 
> about even upper layer. Is this IOCTL restartable? How about further 
> higher layer? Since I didn't see benefit in point 1. So I thought it 
> was a good idea to change.
>
> Anyway, it is up to you. A change might bring other unseen risk. If 
> you still have concern, I will drop this patch. This IOCTL is used by 
> graphic operation. There may not be a lot of signals to a GUI process 
> which uses this IOCTL.
>
> Thanks,
> Alex Bin
>
>
> On 17-04-26 04:34 AM, Christian König wrote:
>> Am 26.04.2017 um 03:17 schrieb Michel Dänzer:
>>> On 26/04/17 06:25 AM, Alex Xie wrote:
>>>> 1. The wait is short. There is not much benefit by
>>>> interruptible waiting.
>>>> 2. In this function and caller functions, the error
>>>> handling for such interrupt is complicated and risky.
>>>>
>>>> Change-Id: I289674ecd3f5ef20c93fe63e33df6d668b3c2edc
>>>> Signed-off-by: Alex Xie <AlexBin.Xie at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>> index 43082bf..c006cc4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>> @@ -227,7 +227,7 @@ int amdgpu_crtc_prepare_flip(
>>>>       new_abo = gem_to_amdgpu_bo(obj);
>>>>         /* pin the new buffer */
>>>> -    r = amdgpu_bo_reserve(new_abo, false);
>>>> +    r = amdgpu_bo_reserve(new_abo, true);
>>>>       if (unlikely(r != 0)) {
>>>>           DRM_ERROR("failed to reserve new abo buffer before flip\n");
>>>>           goto cleanup;
>>>>
>>> I'm afraid we have no choice but to use interruptible here, because 
>>> this
>>> code runs as part of an ioctl (DRM_IOCTL_MODE_PAGE_FLIP).
>>
>> Yes, agree. But the error message is incorrect here and should be 
>> removed.
>>
>> Christian.
>