Re: [Intel-xe] [PATCH] drm/sched: Eliminate drm_sched_run_job_queue_if_ready()

[Luben Tuikov <ltuikov89@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2128 bytes --]

Hi Matt, :-)

On 2023-11-03 11:13, Matthew Brost wrote:
> On Thu, Nov 02, 2023 at 06:46:54PM -0400, Luben Tuikov wrote:
>> Eliminate drm_sched_run_job_queue_if_ready() and instead just call
>> drm_sched_run_job_queue() in drm_sched_free_job_work(). The problem is that
>> the former function uses drm_sched_select_entity() to determine if the
>> scheduler had an entity ready in one of its run-queues, and in the case of the
>> Round-Robin (RR) scheduling, the function drm_sched_rq_select_entity_rr() does
>> just that, selects the _next_ entity which is ready, sets up the run-queue and
>> completion and returns that entity. The FIFO scheduling algorithm is unaffected.
>>
>> Now, since drm_sched_run_job_work() also calls drm_sched_select_entity(), then
>> in the case of RR scheduling, that would result in calling select_entity()
>> twice, which may result in skipping a ready entity if more than one entity is
>> ready. This commit fixes this by eliminating the if_ready() variant.
>>
> 
> Ah, yes I guess we both missed this. What about reviving the peek
> argument [1]? This would avoid unnecessary re-queues.

So, I really am not too fond of "peek-then-get-and-do" (scheduling) organizations,
because they don't scale. As we've seen in our case, the RR has a side effect,
as Tvrtko pointed out (thanks!), and in the future this
"peek-first, then go-again, to go"-type of organization would only prevent us
from doing more interesting things.

Also, with the GPU scheduler organization, mixing in the "peek", we just get
to carry it around through many a function, only to be used in a leaf function,
and exported way back up (because we don't know the rq at that level).

I'd much rather we just did "consume-until-empty", and if we have one last
empty check (or first), then that's not a breaker. (I mean, we have a
drm_sched_pick_best() which has time complexity O(n), and we execute it every time
we arm a job, so it's not that big of a deal.) Plus, it makes the code concise
and compact.

Let me reconstitute the patch and I'll send it for yours review.
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]