Re: [PATCH 1/2] drm/xe/sa: make suballocator waits uninterruptible

[Matthew Auld <matthew.auld@intel.com>]

On 23/10/2026 10:32, Matthew Auld wrote:
> Hi,
> 
> On 22/10/2026 19:29, Thomas Hellström wrote:
>> Hi, Matt
>>
>> On Thu, 2026-10-22 at 18:41 +0100, Matthew Auld wrote:
>>> Large BO clears and copies in xe_migrate_clear() and
>>> xe_migrate_copy()
>>> are split into multiple passes, each needing a batchbuffer from the
>>> fixed size kernel bb pool. Once the pool fills up, __xe_sa_bo_new()
>>> waits interruptibly for space, so a pending signal (e.g. Xorg's 5ms
>>> SIGALRM smart-scheduler timer) makes it fail mid-loop with -
>>> ERESTARTSYS.
>>> The partial progress is then thrown away and the restarted ioctl
>>> starts
>>> again from byte 0, including the memory allocation, which can
>>> livelock
>>> if the signal arrives faster than the whole operation completes. In
>>> the
>>> shrinker path the same error also aborts the shrinker walk, even
>>> though
>>> ctx->interruptible is false.
>>>
>>> Being interruptible here doesn't seem to buy much anyway: on error
>>> the loop already waits uninterruptibly for all submitted passes
>>> before
>>> returning. So just make the suballocator waits uninterruptible,
>>> matching
>>> amdgpu and radeon. The other users (page reclaim from a kworker, and
>>> the
>>> GuC buffer cache) shouldn't really see any meaningful change.
>>
>> This is a deviation from the "make all waits possible interruptible".
>> One might argue that user-space that interrupts kernel work with a 5ms
>> time only to restart it again and again is not well-behaving and
>> continously shoots itself in the foot.
> 
> Maybe. This is the xserver, so I assume this is very old and not some 
> new thing. I feel like even in this situation the kernel should be able 
> to make some forward progress in the ioctl.
> 
>>
>> Given our pipelining abilities, isn't there a risk that we delay signal
>> delivery with up to 10s?
> 
> At least for the copy/clear, it looks like there is already an 
> uninterruptible wait on the err path if fence != NULL, which we trigger 
> on a signal.
> 
>>
>> In the worst case, is there a way we can periodically at least check
>> for pending signals or perhaps increase the sa bo size so that it
>> doesn't block on a single large buffer?
> 
> Other idea is to expose the intr, and then maybe plumb down the ctx- 
>  >interruptible somehow, which would then cover the shrinker case and 
> maybe other places like that which explicitly asked for that? For the 
> clear case (gem_create) maybe we check if the fence != NULL first and 
> then set intr accordingly? If we have a pending signal it would bail if 
> we have not started GPU clearing yet, but if we have started then we try 
> to finish? The error path will currently do an uninterruptible wait 
> anyway, and that one could be a lot worse for a big buffer, since it 
> drains ALL prior jobs, instead of just waiting for the oldest bb. And 
> here it might be that we got right to the tail end of the clears but we 
> got a signal, so we throw away all of that work.
> 
> Increasing the sa bo is also an option (maybe both). Although I guess 
> there will likely still be something too big, and I guess we might still 
> see issues if the pool is still busy with other pending work?

Some more thoughts. Since Matt nuked the vm_bind GPU path, I don't think 
there is much contention on the reserved bcs any more since there is 
just one migrate queue (?), so maybe we can also:

1. Bump the VRAM clear + identity path. 8M is maybe very pessimistic 
now? Maybe we could push a lot more work per MEM_SET + page mode here? 
This could also give more perf, with a lot fewer jobs for 1G+ buffers. I 
have seen llama.py with 5G+ gemma model use 1G+ buffer on my BMG, for 
example.

2. Maybe also pack more work per-bb, at least for identity? Each new bb 
eats up a bunch of padding for pre-fetch etc.

> 
>>
>> /Thomas
>>
>>>
>>> Assisted-by: LLM
>>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/9644
>>> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
>>> GPUs")
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: <stable@vger.kernel.org> # v6.12+
>>> ---
>>>   drivers/gpu/drm/xe/xe_sa.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_sa.c b/drivers/gpu/drm/xe/xe_sa.c
>>> index f32045f40b7a..d9c2b28b08c2 100644
>>> --- a/drivers/gpu/drm/xe/xe_sa.c
>>> +++ b/drivers/gpu/drm/xe/xe_sa.c
>>> @@ -165,7 +165,8 @@ void xe_sa_bo_sync_shadow(struct drm_suballoc
>>> *sa_bo)
>>>    * @size: number of bytes we want to suballocate
>>>    * @gfp: gfp flags used for memory allocation. Typically GFP_KERNEL.
>>>    *
>>> - * Try to make a suballocation of size @size.
>>> + * Try to make a suballocation of size @size. Any waits for space
>>> are
>>> + * uninterruptible.
>>>    *
>>>    * Return: a &drm_suballoc, or an ERR_PTR.
>>>    */
>>> @@ -178,7 +179,7 @@ struct drm_suballoc *__xe_sa_bo_new(struct
>>> xe_sa_manager *sa_manager, u32 size,
>>>       if (size > sa_manager->base.size)
>>>           return ERR_PTR(-ENOBUFS);
>>> -    return drm_suballoc_new(&sa_manager->base, size, gfp, true,
>>> 0);
>>> +    return drm_suballoc_new(&sa_manager->base, size, gfp, false,
>>> 0);
>>>   }
>>>   /**
>