Re: [Intel-xe] [PATCH 1/3] drm/xe: Disable interruptable wait on xe_bo_move()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 5/29/23 16:40, Souza, Jose wrote:
> On Mon, 2023-05-29 at 12:31 +0200, Thomas Hellström wrote:
>> On Mon, 2023-05-29 at 10:59 +0200, Thomas Hellström wrote:
>>> On Fri, 2023-05-26 at 12:06 -0700, José Roberto de Souza wrote:
>>>> All the Xe users of dma_resv_wait_timeout() also disable the
>>>> interruptable wait.
>>>> And doing so this avoids DRM_IOCTL_XE_EXEC fails when under memory
>>>> pressure.
>>> Interruptible waits are crucial for signal delivery latency and for
>>> user experience (being able to ctrl-c away from somtething that is
>>> stuck).
>>>
>>> Uninterruptible waits should only ever be used in situations where
>>> it's
>>> impossible to recover from an error, like in error or possibly
>>> destruction paths.
>>>
>>> In this case execbuf should be able to handle the -EINTR returned
>>> from
>>> the ioctl and restart. If it doesn't there's an error elsewhere.
>>>
>>> But Maarten had a patch to return the real error code rather than
>>> rewriting it to -ETIME.
> This patch? https://patchwork.freedesktop.org/patch/539633/?series=118428&rev=1
>
> This would cause it to return ERESTARTSYS to user-space.

ERESTARTSYS is a special error code that is never forwarded to 
user-space, the kernel's signal handling functionality converts it to 
EINTR iff there is a signal pending, and if not, just restarts the 
system call internally without returning to user-space. So user-space 
should just see the EINTR.

/Thomas


>
>>> /Thomas
>> Upon closer inspection of the code, that bool now set to true should
>> really be ctx->interruptible. Still Maarten's patch would be needed to
>> avoid the -ETIME rewrite, though.
>>
>> /Thomas
>>
>>
>>>
>>>
>>>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/239
>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/xe_bo.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_bo.c
>>>> b/drivers/gpu/drm/xe/xe_bo.c
>>>> index 0db9c05097d07..6ed7e08269e82 100644
>>>> --- a/drivers/gpu/drm/xe/xe_bo.c
>>>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>>>> @@ -609,7 +609,7 @@ static int xe_bo_move(struct ttm_buffer_object
>>>> *ttm_bo, bool evict,
>>>>              new_mem->mem_type == XE_PL_SYSTEM) {
>>>>                  long timeout = dma_resv_wait_timeout(ttm_bo-
>>>>> base.resv,
>>>>                                                      
>>>> DMA_RESV_USAGE_BOOKKEEP,
>>>> -                                                    true,
>>>> +                                                    false,
>>>>                                                      
>>>> MAX_SCHEDULE_TIMEOUT);
>>>>                  if (timeout <= 0) {
>>>>                          ret = -ETIME;